New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpKernel] Fixed RequestDataCollector handling of null header values. #20747

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@gmoreira

gmoreira commented Dec 4, 2016

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This PR references this discussion.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 4, 2016

$response->headers->set('X-Foo-Bar', null); is invalid 😕 The API of $values is string|string[] (well.. string|array actually).

But it doesnt expect null at all. Hence remove().

@gmoreira

This comment has been minimized.

gmoreira commented Dec 4, 2016

@ro0NL You are correct, I should have noticed this. However, this brings up an interesting point, should the HeaderBag set() method protect against this being possible?

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 4, 2016

No, this is design by contract. We could however clarify the PHPdoc, ie. string|string[], and perhaps default an empty array to array('') for the sake of it.

Then there are also a few array_key_exists checks which shouldnt be needed imo.

@alcalyn

This comment has been minimized.

Contributor

alcalyn commented Dec 4, 2016

Personnaly I don't use $headersBag->set('X-Foo-Bar', null);, but $headersBag->set('X-Foo-Bar', $myValue);, and unfortunately $myValue was sometimes null. It doesn't affect the behaviour until this bug.

I think that if we set null, it should just not appears in headers, and let an empty array in headers bag, like before 3.2. So I agree this PR.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 4, 2016

Imo setting null equals setting an empty string; ie. a header value is a string.

and unfortunately $myValue was sometimes null

That doesnt mean you can just violate string|string[].

@gmoreira

This comment has been minimized.

gmoreira commented Dec 5, 2016

@nicolas-grekas would be interested in your opinion here.

@@ -125,7 +125,7 @@ public function collect(Request $request, Response $response, \Exception $except
continue;
}
if ('request_headers' === $key || 'response_headers' === $key) {
$value = array_map(function ($v) { return isset($v[1]) ? $v : $v[0]; }, $value);
$value = array_map(function ($v) { return isset($v[1]) ? $v : (isset($v[0]) ? $v[0] : null); }, $value);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 5, 2016

Member

To be even more defensive, I'd suggest: isset($v[0]) && !isset($v[1]) ? $v[0] : $v

This comment has been minimized.

@gmoreira

gmoreira Dec 5, 2016

Sure. With regards to the unit test however, would you omit this change to the unit test in order to not violate the current interface contract of the HeaderBag::set() method.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 5, 2016

Member

If it's green, let's do with for now.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 5, 2016

👍

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 5, 2016

Is it really something for master only?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 5, 2016

3.2!

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 5, 2016

Thank you @gmoreira.

fabpot added a commit that referenced this pull request Dec 5, 2016

bug #20747 [HttpKernel] Fixed RequestDataCollector handling of null h…
…eader values. (Gabriel Moreira)

This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes #20747).

Discussion
----------

[HttpKernel] Fixed RequestDataCollector handling of null header values.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This PR references this [discussion](symfony/http-kernel@1fee784#commitcomment-20028839).

Commits
-------

adc4a26 [HttpKernel] Fixed RequestDataCollector handling of null header values.

@fabpot fabpot closed this Dec 5, 2016

@fabpot fabpot referenced this pull request Dec 13, 2016

Merged

Release v3.2.1 #20897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment