Skip to content
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

Response headers fix #19143

Merged
merged 1 commit into from Jun 23, 2016

Conversation

Projects
None yet
5 participants
@fabpot
Copy link
Member

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 fabpot:response-headers-fix branch 2 times, most recently from 8e775f3 to 429034f Jun 22, 2016

@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';

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 22, 2016

Author Member

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

@fabpot fabpot force-pushed the fabpot:response-headers-fix branch from 429034f to 66afa01 Jun 22, 2016

$bag1 = new ResponseHeaderBag($headers);
$bag2 = new ResponseHeaderBag($bag1->allPreserveCase());
//print_r($bag1->allPreserveCase());
//print_r($bag2->allPreserveCase());

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jun 22, 2016

Member

These print_r() weren't removed.

$headers = array('foo' => 'bar');
$bag1 = new ResponseHeaderBag($headers);
$bag2 = new ResponseHeaderBag($bag1->allPreserveCase());
//print_r($bag1->allPreserveCase());

This comment has been minimized.

Copy link
@nicolas-grekas
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

👍 (on 3.2 or as bug fix either)

@romainneutron

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

👍

@fabpot fabpot merged commit 66afa01 into symfony:master Jun 23, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 23, 2016

feature #19143 Response headers fix (fabpot)
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 referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.