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

Bug fix: Replace header when sending first one of each header #2441

Merged
merged 1 commit into from Aug 4, 2018

Conversation

Projects
None yet
4 participants
@akrabat
Member

akrabat commented May 16, 2018

When we call header() to send the headers in the Response, set the
replace parameter to true for the first of that header name and then
set it to false.

Don't do this if the header is Set-Cookie so we don't break sessions.

Fixes #2282
Fixes #2246

Replace header when sending first of each header
When we call `header()` to send the headers in the Response, set the
`replace` parameter to true for the first of that header name and then
set it to false.

Don't do this if the header is Set-Cookie so we don't break sessions.

Fixes #2282
Fixes #2246

@geggleto geggleto requested review from silentworks and geggleto May 16, 2018

@coveralls

This comment has been minimized.

coveralls commented May 16, 2018

Coverage Status

Coverage increased (+0.7%) to 97.803% when pulling b504149 on akrabat:override-previously-set-headers into 2be185b on slimphp:3.x.

@ncou

This comment has been minimized.

ncou commented May 17, 2018

Hi,

just my 2 cents opinion, but i believe this modification could break some applications who use multiple header. I think for example to the header Content-Security-Policy you could use multiple headers with cumulative policies. With this modification only the first header will be sent. But this case is probably rare...

Keep up the good work.

@akrabat akrabat added the Slim 3 label May 24, 2018

@akrabat akrabat removed the request for review from geggleto May 24, 2018

@@ -1742,6 +1763,80 @@ public function testResponseWithStreamReadYieldingLessBytesThanAsked()
$this->expectOutputString(str_repeat('.', Mocks\SmallChunksStream::SIZE));
}
public function testResponseReplacesPreviouslySetHeaders()

This comment has been minimized.

@damianopetrungaro

damianopetrungaro May 24, 2018

wdyt about using a dataProvider and trying to pass the test using "strange" values too?

@akrabat akrabat added this to the 3.10.1 milestone Jun 24, 2018

@akrabat

This comment has been minimized.

Member

akrabat commented Jun 24, 2018

@ncou That's not what this patch does. It will allow multiple Content-Security-Policy headers if you add them to the Request object.

This patch replaces headers set by header() if the same header name is used in a header in the Request object.

@ncou

This comment has been minimized.

ncou commented Jun 24, 2018

hummm i think i am starting to understand (or perhaps not :)). In fact this patch will replace (one time) the header if it's already present in the header() list. To sumup it will overwrite the possible headers added by the server (like for example the "Cache-Control" header). And for the Set-Cookie it will do an exception and not replace the first occurrence, because it could be added by the server (i suppose it's to handle the case with the session cookies).
Am i right ?

@akrabat

This comment has been minimized.

Member

akrabat commented Jul 15, 2018

@ncou pretty much, yes.

@ncou

This comment has been minimized.

ncou commented Jul 16, 2018

thank you it's more clear. 👍

akrabat added a commit that referenced this pull request Aug 4, 2018

@akrabat akrabat force-pushed the akrabat:override-previously-set-headers branch from b504149 to efa19d7 Aug 4, 2018

akrabat added a commit to akrabat/Slim that referenced this pull request Aug 4, 2018

@akrabat akrabat merged commit efa19d7 into slimphp:3.x Aug 4, 2018

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 97.802%
Details

@akrabat akrabat changed the title from Replace header when sending first one of each header to Bug fix: Replace header when sending first one of each header Aug 4, 2018

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