Adds response reason phrases test against IANA. #234

fcabralpacheco · 2017-02-19T20:18:48Z

This is a possible implementation of response reason phrases test against the registry maintained by IANA.

I chose XML because of the possibility of validating before starting the test.

The test is marked as incomplete in case of invalid XML.

I'm not sure how is the best way to update it, but I believe a pre-release script would be the best alternative.

Something like this:

curl -o test/TestAsset/http-status-codes.xml http://www.iana.org/assignments/http-status-codes/http-status-codes.xml

About getting the xml when the test runs, I believe is overkill (and adds dependency of external resource), and reminds me of cases of service misuse.

One last comment, the IANA registry is the authoritative list for HTTP status codes (RFC7231, Section 8.2.)

Ref: #230, #208

Ocramius · 2017-02-19T22:57:26Z

test/TestAsset/http-status-codes.xml

@@ -0,0 +1,368 @@
+<?xml version='1.0' encoding='UTF-8'?>


Where does this file come from? And can we instead request it via HTTPS? That's what I was asking for :-)

Specifically, the test should just download them by pinging https://www.iana.org/assignments/http-status-codes/http-status-codes.xml

@Ocramius

Changing the file name by URL in ResponseTest.php:77, works just fine.

However, this can be a problem.

The test will depend on an external online resource.

And besides this, imagine if all implementations decides to do the same thing, requesting the file when the test runs.

I believe this may be end up leading to a case of service misuse.

One solution would be to add a pre-release script, something like this in composer.json ("scripts" block).

"pre-release-tasks": [
"@update-test-asset"
],
"update-test-asset": "curl -o test/TestAsset/http-status-codes.xml https://www.iana.org/assignments/http-status-codes/http-status-codes.xml"

What do you think?

I believe this may be end up leading to a case of service misuse.

One http request a day will not trigger misuse :-)

One solution would be to add a pre-release script, something like this in composer.json ("scripts" block).

"pre-release-tasks": [
"@update-test-asset"
],
"update-test-asset": "curl -o test/TestAsset/http-status-codes.xml https://www.iana.org/assignments/http-status-codes/http-status-codes.xml"

What do you think?

If it's not automated, it doesn't exist. Let's please push it to the test suite, and run it every time.

OK. I understand.

Check new version, I may remove "last modified" stuff to make it more simpler.

Apparently IANA server don't support "If-Modified-Since" (or I made a mistake).

However I think what timeout and fallback to file is necessary.

@Ocramius

I removed unused code. And I tried to make it simpler and clear.

This file is to be removed from the test suite

samsonasik · 2017-02-22T03:45:32Z

@fcabralpacheco could you create alternate PR against zend-http also in favor of zendframework/zend-http#110 ?

fcabralpacheco · 2017-02-22T05:09:37Z

@samsonasik Yes. I will create an alternate PR.

Ocramius · 2017-02-22T13:23:03Z

test/ResponseTest.php

    {
        set_error_handler(function ($errno, $errstr) {
            throw new \ErrorException($errstr, 0, $errno);
        });

-        $updateError = null;
+        $ianaHttpStatusCodes = new \DOMDocument();


requires ext-xml in the dev dependencies

Requires ext-dom.

Ocramius · 2017-02-22T13:23:36Z

test/ResponseTest.php

                }
            }
        } catch (\Exception $e) {
-            $updateError = $e->getMessage();
+            $errorMessage = $e->getMessage();


Remove this logic (the entire try/catch)

Ocramius · 2017-02-22T13:24:06Z

test/TestAsset/http-status-codes.xml

@@ -0,0 +1,368 @@
+<?xml version='1.0' encoding='UTF-8'?>


This file is to be removed from the test suite

Ocramius · 2017-02-22T13:24:34Z

test/TestAsset/iana-registry.rng

@@ -0,0 +1,198 @@
+<?xml version='1.0'?>


file to be removed from the PR

Hmm, never mind, this seems to be required for validity checks

Ocramius · 2017-02-22T13:25:51Z

test/ResponseTest.php

+        $errorMessage = null;
+        $httpStatus = 0;
+
+        try {


to be removed

Ocramius · 2017-02-22T13:36:51Z

test/ResponseTest.php

+
+        if (! $validXml) {
+            if ($errorMessage) {
+                print 'Error on IANA "http-status-codes.xml" update. Error: ' . $errorMessage . "\n";


self::fail() is sufficient :-\

Ocramius · 2017-02-22T13:37:11Z

test/ResponseTest.php

+        restore_error_handler();
+
+        if (! $validXml) {
+            $this->markTestIncomplete(


self::fail() is better here.

Ocramius · 2017-02-22T13:37:38Z

test/ResponseTest.php

+    {
+        $ianaHttpStatusCodes = $this->updateAndLoadIanaHttpStatusCodes();
+
+        if (! $ianaHttpStatusCodes) {


The method should never return null

Ocramius · 2017-02-22T13:38:25Z

test/ResponseTest.php

+            $value = $xpath->query('.//ns:value', $record)->item(0)->nodeValue;
+            $description = $xpath->query('.//ns:description', $record)->item(0)->nodeValue;
+
+            if ($description === 'Unassigned' || $description === '(Unused)') {


in_array()

Ocramius · 2017-02-22T13:39:06Z

test/ResponseTest.php

+
+            $range = preg_match('/^([0-9]+)\s*\-\s*([0-9]+)$/', $value, $matches);
+
+            if (! $range) {


Swap the conditional to avoid the negation. Also, $range variable is redundant

fcabralpacheco · 2017-02-23T05:13:20Z

@Ocramius

Removed file fallback and error handling logic.

PHPUnit handles all errors/warnings/notices nicely, just self:fail() worked very well.

fcabralpacheco · 2017-02-23T05:16:39Z

@Ocramius

I have not updated the composer.lock file. Do I need to do this?

weierophinney · 2017-04-06T15:54:32Z

test/ResponseTest.php

+        $ianaHttpStatusCodes->load('https://www.iana.org/assignments/http-status-codes/http-status-codes.xml');
+
+        if (! $ianaHttpStatusCodes->relaxNGValidate(__DIR__ . '/TestAsset/http-status-codes.rng')) {
+            self::fail();


This should have a useful message, to make it clear why the failure occurred (timeout reaching the validation file and/or invalid content in the file fetched). I can add that at merge, however.

weierophinney · 2017-04-06T15:54:53Z

test/ResponseTest.php

@@ -66,6 +66,63 @@ public function testReasonPhraseDefaultsToStandards()
        $this->assertEquals('Unprocessable Entity', $response->getReasonPhrase());
    }

+    public function ianaCodesReasonPhrasesProvider()
+    {
+        $ianaHttpStatusCodes = new \DOMDocument();


Import DOMDocument. (I can do that at merge)

weierophinney · 2017-04-06T15:55:06Z

test/ResponseTest.php

+
+        $ianaCodesReasonPhrases = [];
+
+        $xpath = new \DomXPath($ianaHttpStatusCodes);


Import DOMXPath. (I can do that at merge)

Adds response reason phrases test against IANA.

Close #234

Forward port #234

weierophinney · 2017-04-06T16:00:49Z

Thanks, @fcabralpacheco

@fcabralpacheco

…IANA's data (dunglas) This PR was merged into the 2.7 branch. Discussion ---------- [HttpFoundation] Fix and test status codes according to IANA's data | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR updates HTTP status code according to values [standardized by the IANA](https://www.iana.org/assignments/http-status-codes/http-status-codes.xml). It also add a test to check the conformance (we'll also be notified when a code is added, ~~removed~~ or modified). All credits go to @fcabralpacheco for is work in Zend Diactoros: zendframework/zend-diactoros#234 Commits ------- 72d25cc [HttpFoundation] Fix and test status codes according to IANA's data

fcabralpacheco added 3 commits February 19, 2017 16:17

Add testReasonPhraseDefaultsAgainstIana.

f65a0ea

Ignore '(Unused)' codes on IANA http-status-codes.xml.

be87948

Fixes incorrect reason phrases.

107fb39

Ocramius suggested changes Feb 19, 2017

View reviewed changes

Ocramius added the enhancement label Feb 20, 2017

Update IANA "http-status-codes.xml" when the test runs

cf80bc9

fcabralpacheco added 2 commits February 22, 2017 01:34

Makes the "http-status-codes.xml" file update process simpler and clear.

1290307

Small CS fix.

3272bf7

Ocramius suggested changes Feb 22, 2017

View reviewed changes

Ocramius assigned fcabralpacheco Feb 22, 2017

Ocramius added the Awaiting author feedback label Feb 22, 2017

fcabralpacheco added 4 commits February 23, 2017 01:36

Removes file fallback and local error handling logic.

a2fe140

Remove 'http-status-codes.xml' from test suite.

131631c

Remove redundant variables. Code cleanup.

d83e713

Adds the new dependencies for ResponseTest.php.

15c2d96

weierophinney removed the Awaiting author feedback label Apr 6, 2017

weierophinney approved these changes Apr 6, 2017

View reviewed changes

weierophinney added this to the 1.3.11 milestone Apr 6, 2017

weierophinney merged commit 15c2d96 into zendframework:master Apr 6, 2017

weierophinney added a commit that referenced this pull request Apr 6, 2017

Merge pull request #234 from fcabralpacheco/test-against-iana

d512c6c

Adds response reason phrases test against IANA.

weierophinney added a commit that referenced this pull request Apr 6, 2017

Added CHANGELOG for #234

9846822

weierophinney added a commit that referenced this pull request Apr 6, 2017

Merge branch 'hotfix/234'

59faaa0

Close #234

weierophinney added a commit that referenced this pull request Apr 6, 2017

Merge branch 'hotfix/234' into develop

8a71cfc

Forward port #234

weierophinney mentioned this pull request Apr 6, 2017

Add some useful constants php-fig/link-util#2

Closed

dunglas mentioned this pull request Apr 10, 2017

[HttpFoundation] Fix and test status codes according to IANA's data symfony/symfony#22357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds response reason phrases test against IANA. #234

Adds response reason phrases test against IANA. #234

fcabralpacheco commented Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

fcabralpacheco Feb 20, 2017 •

edited

Loading

Ocramius Feb 20, 2017 •

edited

Loading

fcabralpacheco Feb 21, 2017 •

edited

Loading

fcabralpacheco Feb 22, 2017

Ocramius Feb 22, 2017

samsonasik commented Feb 22, 2017

fcabralpacheco commented Feb 22, 2017

Ocramius Feb 22, 2017

fcabralpacheco Feb 23, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

fcabralpacheco commented Feb 23, 2017

fcabralpacheco commented Feb 23, 2017

weierophinney Apr 6, 2017

weierophinney Apr 6, 2017

weierophinney Apr 6, 2017

weierophinney commented Apr 6, 2017


		$range = preg_match('/^([0-9]+)\s\-\s([0-9]+)$/', $value, $matches);

		if (! $range) {


		$ianaCodesReasonPhrases = [];

		$xpath = new \DomXPath($ianaHttpStatusCodes);

Adds response reason phrases test against IANA. #234

Adds response reason phrases test against IANA. #234

Conversation

fcabralpacheco commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcabralpacheco Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Ocramius Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

fcabralpacheco Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik commented Feb 22, 2017

fcabralpacheco commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcabralpacheco commented Feb 23, 2017

fcabralpacheco commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Apr 6, 2017

fcabralpacheco Feb 20, 2017 •

edited

Loading

Ocramius Feb 20, 2017 •

edited

Loading

fcabralpacheco Feb 21, 2017 •

edited

Loading