Skip to content

Response headers fix #19143

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

Merged
merged 1 commit into from
Jun 23, 2016
Merged

Response headers fix #19143

merged 1 commit into from
Jun 23, 2016

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Jun 22, 2016

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? yes/no
Deprecations? no
Tests pass? yes
Fixed tickets #16171, #16307
License MIT
Doc PR n/a

To fix the inconsistency mentioned in #16171, I think the "best" solution would be to add private when cache-control is not set, which was the intention but was forgotten.

I propose to make the fix in 3.2 only as it might be a BC break.

@fabpot fabpot force-pushed the response-headers-fix branch 2 times, most recently from 8e775f3 to 429034f Compare June 22, 2016 13:18
@fabpot fabpot removed the BC Break label Jun 22, 2016
@@ -281,7 +281,7 @@ public function makeDisposition($disposition, $filename, $filenameFallback = '')
protected function computeCacheControlValue()
{
if (!$this->cacheControl && !$this->has('ETag') && !$this->has('Last-Modified') && !$this->has('Expires')) {
return 'no-cache';
return 'no-cache, private';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding private is what we are doing by default line 299 in the same situation.

@fabpot fabpot force-pushed the response-headers-fix branch from 429034f to 66afa01 Compare June 22, 2016 13:19
$bag1 = new ResponseHeaderBag($headers);
$bag2 = new ResponseHeaderBag($bag1->allPreserveCase());
//print_r($bag1->allPreserveCase());
//print_r($bag2->allPreserveCase());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These print_r() weren't removed.

@nicolas-grekas
Copy link
Member

👍 (on 3.2 or as bug fix either)

@romainneutron
Copy link
Contributor

👍

@fabpot fabpot merged commit 66afa01 into symfony:master Jun 23, 2016
fabpot added a commit that referenced this pull request Jun 23, 2016
This PR was merged into the 3.2-dev branch.

Discussion
----------

Response headers fix

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes/no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16171, #16307
| License       | MIT
| Doc PR        | n/a

To fix the inconsistency mentioned in #16171, I think the "best" solution would be to add `private` when cache-control is not set, which was the intention but was forgotten.

I propose to make the fix in 3.2 only as it might be a BC break.

Commits
-------

66afa01 [HttpFoundation] added private by default when setting Cache-Control to no-cache
@fabpot fabpot mentioned this pull request Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants