Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Keep max lifetime also when part of the responses don't set it#41665
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
| yield'merge max-age and s-maxage' => [ | ||
| ['public' =>true,'s-maxage' =>'60','max-age' =>null], | ||
| ['public' =>true,'max-age' =>'60'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Short explanation what happens here:
Previously, not all responses hadmax-age, so it was dropped. But all responses were assumed to haves-maxage (because that can be substituted bymaxage), so it was kept.
With this change, thes-maxage is computed as 60, because we have the explicits-maxage andmax-age=60. And we getmax-age=60, because one response sets that explicitly, whereas the other does not give an expiration time but ispublic. So the result iss-maxage=60, maxage=60, which is merged into a singlemax-age=60.
Toflar left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These changes look correct to me too, although I'd personally never configure a response withCache-Control: public only. I don't see any use case, really. "Heuristics" sound a bit too vague for my taste and I cannot imagine a case where you'd go "oh hello reverse proxy cache, do whatever you want to do with this" 😄
But Symfony is a framework and it's not our business to judge whether something should be used or not. We should stick to the spec. Hence, 👍 on this.
Thanks for the detailed issue description, Matthias!
Although I agree that this change makes sense (for the reasons explained by@Toflar), I'm wondering whether we should change the behavior of what do you think@fabpot@nicolas-grekas ? |
@stof Yes, I've thought about this as well. I came to the conclusion that it is a real bug fix (and thus against 4.4), since it may lead to responses being kept in caches longer than you wanted. I "found" this only yesterday when a customer reported they made changes somewhere but those did not show up for the end users: The old version was kept in the CDN although it should not have been. But that's my 2e-2 💰, you decide :-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah if the current behavior indeed leads to caching the final response for too long in some cases, I agree about treating that as a bug fix
src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
53d9647 toad1f057CompareRebased and resolved merge conflicts after#41663 has been merged. |
Thank you@mpdude. |
…ires` headers (aschempp)This PR was squashed before being merged into the 5.4 branch.Discussion----------[HttpKernel] Correctly merge `max-age`/`s-maxage` and `Expires` headers| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fixcontao/contao#7494| License | MITThe `ResponseCacheStrategy` does not currently merge `Expires` and `Cache-Control: max-age/s-maxage` headers. Before#41665 this was not an issue, because if not all respones had all headers, they were not added to the final reponse. And we could assume a response itself is consistent between `Expires` and `max-age`.`@mpdude` added _heuristic caching_ of public responses in#41665. Unfortunately, it only looks at `Cache-Control: public` but if should also check if no cache information (max-age/s-maxage/Expires) is present. If that were the case, the behavior would not have changed. But it now leads to inconsistent header values because it independently keeps `Expires` and `max-age/s-maxage`.This PR does not only fix the _heuristic caching_, but also merges `Expires` and `Cache-Control` headers to make sure only the lowest value is retained across all headers. For semi-BC reasons I also made sure to only add an `Expires` header if any of the responses contains one.Commits-------d3e65d6 [HttpKernel] Correctly merge `max-age`/`s-maxage` and `Expires` headers
…ires` headers (aschempp)This PR was squashed before being merged into the 5.4 branch.Discussion----------[HttpKernel] Correctly merge `max-age`/`s-maxage` and `Expires` headers| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fixcontao/contao#7494| License | MITThe `ResponseCacheStrategy` does not currently merge `Expires` and `Cache-Control: max-age/s-maxage` headers. Beforesymfony/symfony#41665 this was not an issue, because if not all respones had all headers, they were not added to the final reponse. And we could assume a response itself is consistent between `Expires` and `max-age`.`@mpdude` added _heuristic caching_ of public responses insymfony/symfony#41665. Unfortunately, it only looks at `Cache-Control: public` but if should also check if no cache information (max-age/s-maxage/Expires) is present. If that were the case, the behavior would not have changed. But it now leads to inconsistent header values because it independently keeps `Expires` and `max-age/s-maxage`.This PR does not only fix the _heuristic caching_, but also merges `Expires` and `Cache-Control` headers to make sure only the lowest value is retained across all headers. For semi-BC reasons I also made sure to only add an `Expires` header if any of the responses contains one.Commits-------d3e65d64169 [HttpKernel] Correctly merge `max-age`/`s-maxage` and `Expires` headers
Uh oh!
There was an error while loading.Please reload this page.
https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.2 allows caches to assign a "heuristic expiration time" for responses that have no explicit expiration time set, but are explicitly marked as being cacheable by
public. We can say that such responses are "more liberal" in what is allowed than a response with an explicitmax-ageors-maxageheader.When merging responses in
ResponseCacheStrategy, suchpublicresponses without explicit expiration time should not cause themax-ageors-maxagevalues being dropped on the final response. The most restrictive settings from all responses involved should be used, and any given expiration time is more strict than not setting one when beingpublic.