Header's improvements #161

snapshotpl · 2016-03-19T11:04:10Z

I remove duplicates and simplify some parts of code

weierophinney · 2016-09-07T13:45:08Z

test/MessageTraitTest.php

-     * @dataProvider testNumericHeaderValues
-     * @group 99
-     */
-    public function testFilterHeadersShouldAllowIntegersAndFloats($value)


I'm concerned about removal of this test case, particularly as it was solving a reported issue. Can you reinstate it or rewrite it to test against the current changes, please?

I have tried, but there is some exclusive cases. Look at invalidHeaderValues. There are int and float as invalid value. This removed tests uses testNumericHeaderValues where is also int and float but as valid value

This goes to another comment I've made in the review: numeric values should be allowed, which is what #99 was reporting. Internally, we can convert those to strings, so that the return values of getHeaders(), getHeader(), and getHeaderLine() are consistent with the spec. But we must allow them; requiring developers to cast those values to strings manually before calling withHeader() is a BC break, and poor for usability.

This test will likely need to be rewritten, but the behavior we're testing is the same: numeric values should be valid for header values.

weierophinney

I like the work you've done here, as it does greatly simplify much of the code. The big issue I have is with treatment of numeric values for header values. As noted, numeric values are often used (e.g., Content-Length, X-Page-Offset, etc.), and should be accepted as valid input. However, with the changes made, such values are immediately considered invalid, which is a BC break. Admittedly, we had some problematic code previously, as providing an array of values would raise an exception if any of those values were not strings; that situation, however, should be fixed.

Are you still willing to make the requested changes, @snapshotpl ?

weierophinney · 2017-01-05T19:02:33Z

src/MessageTrait.php

@@ -112,7 +112,7 @@ public function getHeaders()
     */
    public function hasHeader($header)
    {
-        return array_key_exists(strtolower($header), $this->headerNames);
+        return isset($this->headerNames[strtolower($header)]);


The original was more correct.

The ABNS for header values is:

field-value = *( field-content / obs-fold )

The above means zero or more of valid field content/folded content. In other words, you can have a header with no value associated with it.

Since isset() will return false on an empty string, empty array, or null value, the above would result in a false negative in the case of a header being present with no value.

I have added test for that - still working as expected (testHeaderExistsIfWithNoValues, testHeaderWithNoValues). Do you have any other test case to fall it?

weierophinney · 2017-01-05T19:07:57Z

src/HeaderSecurity.php

@@ -129,6 +129,12 @@ public static function isValid($value)
     */
    public static function assertValid($value)
    {
+        if (! is_string($value)) {


This is somewhat problematic. Numeric values are allowed in headers — e.g., Content-Length — which means asserting that the value is a string could lead to issues for existing users (including those using the SapiStreamEmitter).

That said, we currently enforce this when an array of header values is passed to withHeader() (via the private arrayContainsOnlyStrings() method).

My feeling is that we should likely cast numeric values to strings when storing headers, vs. throwing them out outright. This would, of course, necessitate further changes from what you were already planning to introduce in this pull request.

weierophinney · 2017-01-05T19:11:55Z

src/RequestTrait.php

-        list($this->headerNames, $headers) = $this->filterHeaders($headers);
-        $this->assertHeaders($headers);
-        $this->headers = $headers;
+        $this->setHeaders($headers);


This assumes setHeaders() is in the class composing the trait; the trait should define an abstract method, to ensure that the functionality can work. (This should likely be done for the other calls on $this as well.)

Done by use MessageTrait. No magic anymore!

weierophinney · 2017-01-05T19:44:20Z

test/MessageTraitTest.php

@@ -218,7 +218,7 @@ public function testWithHeaderReplacesDifferentCapitalization()
     */
    public function testWithAddedHeaderRaisesExceptionForNonStringNonArrayValue($value)
    {
-        $this->setExpectedException('InvalidArgumentException', 'must be a string');
+        $this->setExpectedException('InvalidArgumentException', 'expected string');


Why is this exception message different?

weierophinney · 2017-01-05T19:48:14Z

test/MessageTraitTest.php

@@ -353,7 +328,7 @@ public function invalidArrayHeaderValues()
     */
    public function testFilterHeadersShouldRaiseExceptionForInvalidHeaderValuesInArrays($value)


Rename the method to s/FilterHeaders/SetHeaders/

weierophinney · 2017-01-05T19:48:21Z

test/MessageTraitTest.php

@@ -368,7 +343,7 @@ public function testFilterHeadersShouldRaiseExceptionForInvalidHeaderValuesInArr
     */
    public function testFilterHeadersShouldRaiseExceptionForInvalidHeaderScalarValues($value)


Rename the method to s/FilterHeaders/SetHeaders/

malukenho · 2017-01-12T00:06:02Z

src/HeaderSecurity.php

@@ -129,6 +129,12 @@ public static function isValid($value)
     */
    public static function assertValid($value)
    {
+        if (! is_string($value) && ! is_numeric($value)) {
+            throw new InvalidArgumentException(sprintf(
+                'Invalid header value type; must be a string, numeric or array; received %s',


array is not allowed on this current logic

Header's improvements

Close #161

Forward port #161

weierophinney · 2017-04-06T15:35:59Z

Thanks, @snapshotpl

snapshotpl added 3 commits March 19, 2016 00:41

Header improvements

15d4171

Next improvements

27742b5

Another changes

ce89945

weierophinney reviewed Sep 7, 2016
View reviewed changes

weierophinney added the Awaiting author feedback label Sep 7, 2016

Merge branch 'master' into header_improvements

2cfe204

weierophinney suggested changes Jan 5, 2017

View reviewed changes

snapshotpl added 5 commits January 11, 2017 21:38

Allow numeric header values

bd3d531

Change usage of traits

b92b0cb

Fix CS

16c8e33

Add tests to empty values of header

849e0da

Revert exception messages

ddd7178

malukenho reviewed Jan 12, 2017

View reviewed changes

snapshotpl added 2 commits January 22, 2017 11:56

Array is not allowed

d10ed53

Improve code coverage

8b94d5e

weierophinney added enhancement and removed Awaiting author feedback labels Apr 6, 2017

weierophinney added this to the 1.3.11 milestone Apr 6, 2017

weierophinney merged commit 8b94d5e into zendframework:master Apr 6, 2017

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

Merge pull request #161 from snapshotpl/header_improvements

1fa7e17

Header's improvements

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

Added CHANGELOG for #161

b33e64b

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

Merge branch 'hotfix/161'

162dd97

Close #161

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

Merge branch 'hotfix/161' into develop

6550c9f

Forward port #161

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Header's improvements #161

Header's improvements #161

snapshotpl commented Mar 19, 2016

weierophinney Sep 7, 2016

snapshotpl Sep 7, 2016

weierophinney Jan 5, 2017

weierophinney left a comment

weierophinney Jan 5, 2017

snapshotpl Jan 11, 2017

weierophinney Jan 5, 2017

weierophinney Jan 5, 2017

snapshotpl Jan 11, 2017

weierophinney Jan 5, 2017

weierophinney Jan 5, 2017

weierophinney Jan 5, 2017

malukenho Jan 12, 2017

weierophinney commented Apr 6, 2017

		@@ -353,7 +328,7 @@ public function invalidArrayHeaderValues()
		*/
		public function testFilterHeadersShouldRaiseExceptionForInvalidHeaderValuesInArrays($value)

		@@ -368,7 +343,7 @@ public function testFilterHeadersShouldRaiseExceptionForInvalidHeaderValuesInArr
		*/
		public function testFilterHeadersShouldRaiseExceptionForInvalidHeaderScalarValues($value)

Header's improvements #161

Header's improvements #161

Conversation

snapshotpl commented Mar 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a 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

weierophinney commented Apr 6, 2017