Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
leave cache-control no-cache as is#16307
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ewgRa commentedOct 21, 2015
Hmmm... HttpCacheTest::testEsiCacheForceValidation failed, but only for 5.3 and hhvm |
ewgRa commentedOct 21, 2015
Status: Needs work |
ewgRa commentedOct 22, 2015
Understand now why 5.3 and hhvm fail and others not. CI calls for others: It install HttpFoundation without my changes and pass. In another words - test HttpKernel with not changed HttpFoundation. For 5.3 and hhvm: and HttpKernel will be tested with my changes in HttpFoundation and fail, that actually right. Question1: it is not well configured CI, or there is a trick how to work with such changes? Question2: how to solve this fail? I sure that testEquals must be passed. But for this we need change default 'no-cache' to 'no-cache, private', or change broken tests in a way when they not expect private for 'no-cache' (broke bc?). As from my point of view, 'no-cache, private' - make no sense, because no-cache mean no any cache at all, for proxies, for browsers and no-cache include all cases for 'private'. |
fabpot commentedJun 15, 2016
I don't understand this fix. You can have |
ewgRa commentedJun 15, 2016
Yes, it is legitimate. And ResponseHeaderBag still can have it. Yes, it is kind of hard to find here "right" solution, but expected is when I create ResponseHeaderBag and than "clone" it by ResponseHeaderBag($originalBag->allPreserveCase()) - headers must be same. Main idea - if "no-cache" in cacheControl already setted, we can't add "private", because "no-cache" and "no-cache, private" is not a same, and it will be unexpected. |
fabpot commentedJun 22, 2016
I think we should fix the bug as I've done in ##19143. |
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/aTo 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
Fix, that do not add "private" for "no-cache" in ResponseHeaderBag. By default there is "no-cache" returned, and that mean we must follow same logic (avoid add "private") when "no-cache" present.