Skip to content

[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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 24, 2021
Merged

[HttpKernel] Keep max lifetime also when part of the responses don't set it #41665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,15 @@ public function add(Response $response)
return;
}

$isHeuristicallyCacheable = $response->headers->hasCacheControlDirective('public');
$maxAge = $response->headers->hasCacheControlDirective('max-age') ? (int) $response->headers->getCacheControlDirective('max-age') : null;
$this->storeRelativeAgeDirective('max-age', $maxAge, $age);
$this->storeRelativeAgeDirective('max-age', $maxAge, $age, $isHeuristicallyCacheable);
$sharedMaxAge = $response->headers->hasCacheControlDirective('s-maxage') ? (int) $response->headers->getCacheControlDirective('s-maxage') : $maxAge;
$this->storeRelativeAgeDirective('s-maxage', $sharedMaxAge, $age);
$this->storeRelativeAgeDirective('s-maxage', $sharedMaxAge, $age, $isHeuristicallyCacheable);

$expires = $response->getExpires();
$expires = null !== $expires ? (int) $expires->format('U') - (int) $response->getDate()->format('U') : null;
$this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0);
$this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0, $isHeuristicallyCacheable);
}

/**
Expand Down Expand Up @@ -199,11 +200,29 @@ private function willMakeFinalResponseUncacheable(Response $response): bool
* we have to subtract the age so that the value is normalized for an age of 0.
*
* If the value is lower than the currently stored value, we update the value, to keep a rolling
* minimal value of each instruction. If the value is NULL, the directive will not be set on the final response.
* minimal value of each instruction.
*
* If the value is NULL and the isHeuristicallyCacheable parameter is false, the directive will
* not be set on the final response. In this case, not all responses had the directive set and no
* value can be found that satisfies the requirements of all responses. The directive will be dropped
* from the final response.
*
* If the isHeuristicallyCacheable parameter is true, however, the current response has been marked
* as cacheable in a public (shared) cache, but did not provide an explicit lifetime that would serve
* as an upper bound. In this case, we can proceed and possibly keep the directive on the final response.
*/
private function storeRelativeAgeDirective(string $directive, ?int $value, int $age)
private function storeRelativeAgeDirective(string $directive, ?int $value, int $age, bool $isHeuristicallyCacheable)
{
if (null === $value) {
if ($isHeuristicallyCacheable) {
/*
* See https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.2
* This particular response does not require maximum lifetime; heuristics might be applied.
* Other responses, however, might have more stringent requirements on maximum lifetime.
* So, return early here so that the final response can have the more limiting value set.
*/
return;
}
$this->ageDirectives[$directive] = false;
}

Expand Down
26 changes: 25 additions & 1 deletion src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ public function cacheControlMergingProvider()
];

yield 'merge max-age and s-maxage' => [
['public' => true, 's-maxage' => '60', 'max-age' => null],
['public' => true, 'max-age' => '60'],
Copy link
Contributor Author

@mpdude mpdude Jun 11, 2021 •
edited
Loading

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 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.

['public' => true, 's-maxage' => 3600],
[
['public' => true, 'max-age' => 60],
Expand All @@ -393,6 +393,30 @@ public function cacheControlMergingProvider()
],
];

yield 'public subresponse without lifetime does not remove lifetime for main response' => [
['public' => true, 's-maxage' => '30', 'max-age' => null],
['public' => true, 's-maxage' => '30'],
[
['public' => true],
],
];

yield 'lifetime for subresponse is kept when main response has no lifetime' => [
['public' => true, 'max-age' => '30'],
['public' => true],
[
['public' => true, 'max-age' => '30'],
],
];

yield 's-maxage on the subresponse implies public, so the result is public as well' => [
['public' => true, 'max-age' => '10', 's-maxage' => null],
['public' => true, 'max-age' => '10'],
[
['max-age' => '30', 's-maxage' => '20'],
],
];

yield 'result is private when combining private responses' => [
['no-cache' => false, 'must-revalidate' => false, 'private' => true],
['s-maxage' => 60, 'private' => true],
Expand Down