[HttpKernel] Keep max lifetime also when part of the responses don't set it #41665
Conversation
| @@ -370,13 +370,29 @@ public function cacheControlMergingProvider() | |||
| ]; | |||
|
|
|||
| 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.
Short explanation what happens here:
Previously, not all responses had max-age, so it was dropped. But all responses were assumed to have s-maxage (because that can be substituted by maxage), so it was kept.
With this change, the s-maxage is computed as 60, because we have the explicit s-maxage and max-age=60. And we get max-age=60, because one response sets that explicitly, whereas the other does not give an expiration time but is public. So the result is s-maxage=60, maxage=60, which is merged into a single max-age=60.
There was a problem hiding this comment.
These changes look correct to me too, although I'd personally never configure a response with Cache-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 :-). |
src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php
Outdated
Show resolved
Hide resolved
…resulting response
53d9647 to
ad1f057
Compare
|
Rebased 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 | Fix contao/contao#7494 | License | MIT The `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 | Fix contao/contao#7494 | License | MIT The `ResponseCacheStrategy` does not currently merge `Expires` and `Cache-Control: max-age/s-maxage` headers. Before symfony/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 in symfony/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
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.