leave cache-control no-cache as is #16307
Conversation
|
Hmmm... HttpCacheTest::testEsiCacheForceValidation failed, but only for 5.3 and hhvm |
|
Status: Needs work |
|
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'. |
|
I don't understand this fix. You can have |
|
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. |
|
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/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
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.