8000 bug #58376 [HttpKernel] Correctly merge `max-age`/`s-maxage` and `Exp… · symfony/symfony@908a91f · GitHub
[go: up one dir, main page]
Skip to content

Commit 908a91f

Browse files
nicolas-grekasnicolas-grekas
committed
bug #58376 [HttpKernel] Correctly merge max-age/s-maxage and Expires 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
2 parents 7fe5bba + d3e65d6 commit 908a91f

File tree

2 files changed

+89
-33
lines changed

2 files changed

+89
-33
lines changed

‎src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php

Lines changed: 28 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ class ResponseCacheStrategy implements ResponseCacheStrategyInterface
5050
private $ageDirectives = [
5151
'max-age' => null,
5252
's-maxage' => null,
53-
'expires' => null,
53+
'expires' => false,
5454
];
5555

5656
/**
@@ -81,15 +81,30 @@ public function add(Response $response)
8181
return;
8282
}
8383

84-
$isHeuristicallyCacheable = $response->headers->hasCacheControlDirective('public');
8584
$maxAge = $response->headers->hasCacheControlDirective('max-age') ? (int) $response->headers->getCacheControlDirective('max-age') : null;
86-
$this->storeRelativeAgeDirective('max-age', $maxAge, $age, $isHeuristicallyCacheable);
8785
$sharedMaxAge = $response->headers->hasCacheControlDirective('s-maxage') ? (int) $response->headers->getCacheControlDirective('s-maxage') : $maxAge;
88-
$this->storeRelativeAgeDirective('s-maxage', $sharedMaxAge, $age, $isHeuristicallyCacheable);
89-
9086
$expires = $response->getExpires();
9187
$expires = null !== $expires ? (int) $expires->format('U') - (int) $response->getDate()->format('U') : null;
92-
$this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0, $isHeuristicallyCacheable);
88+
89+
// See https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.2
90+
// If a response is "public" but does not have maximum lifetime, heuristics might be applied.
91+
// Do not store NULL values so the final response can have more limiting value from other responses.
92+
$isHeuristicallyCacheable = $response->headers->hasCacheControlDirective('public')
93+
&& null === $maxAge
94+
&& null === $sharedMaxAge
95+
&& null === $expires;
96+
97+
if (!$isHeuristicallyCacheable || null !== $maxAge || null !== $expires) {
98+
$this->storeRelativeAgeDirective('max-age', $maxAge, $expires, $age);
99+
}
100+
101+
if (!$isHeuristicallyCacheable || null !== $sharedMaxAge || null !== $expires) {
102+
$this->storeRelativeAgeDirective('s-maxage', $sharedMaxAge, $expires, $age);
103+
}
104+
105+
if (null !== $expires) {
106+
$this->ageDirectives['expires'] = true;
107+
}
93108
}
94109

95110
/**
@@ -102,7 +117,7 @@ public function update(Response $response)
102117
return;
103118
}
104119

105-
// Remove validation related headers of the master response,
120+
// Remove validation related headers of the final response,
106121
// because some of the response content comes from at least
107122
// one embedded response (which likely has a different caching strategy).
108123
$response->setEtag(null);
@@ -145,9 +160,9 @@ public function update(Response $response)
145160
}
146161
}
147162

148-
if (is_numeric($this->ageDirectives['expires'])) {
163+
if ($this->ageDirectives['expires'] && null !== $maxAge) {
149164
$date = clone $response->getDate();
150-
$date = $date->modify('+'.($this->ageDirectives['expires'] + $this->age).' seconds');
165+
$date = $date->modify('+'.$maxAge.' seconds');
151166
$response->setExpires($date);
152167
}
153168
}
@@ -200,33 +215,16 @@ private function willMakeFinalResponseUncacheable(Response $response): bool
200215
* we have to subtract the age so that the value is normalized for an age of 0.
201216
*
202217
* If the value is lower than the currently stored value, we update the value, to keep a rolling
203-
* minimal value of each instruction.
204-
*
205-
* If the value is NULL and the isHeuristicallyCacheable parameter is false, the directive will
206-
* not be set on the final response. In this case, not all responses had the directive set and no
207-
* value can be found that satisfies the requirements of all responses. The directive will be dropped
208-
* from the final response.
209-
*
210-
* If the isHeuristicallyCacheable parameter is true, however, the current response has been marked
211-
* as cacheable in a public (shared) cache, but did not provide an explicit lifetime that would serve
212-
* as an upper bound. In this case, we can proceed and possibly keep the directive on the final response.
218+
* minimal value of each instruction. If the value is NULL, the directive will not be set on the final response.
213219
*/
214-
private function storeRelativeAgeDirective(string $directive, ?int $value, int $age, bool $isHeuristicallyCacheable)
220+
private function storeRelativeAgeDirective(string $directive, ?int $value, ?int $expires, int $age): void
215221
{
216-
if (null === $value) {
217-
if ($isHeuristicallyCacheable) {
218-
/*
219-
* See https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.2
220-
* This particular response does not require maximum lifetime; heuristics might be applied.
221-
* Other responses, however, might have more stringent requirements on maximum lifetime.
222-
* So, return early here so that the final response can have the more limiting value set.
223-
*/
224-
return;
225-
}
222+
if (null === $value && null === $expires) {
226223
$this->ageDirectives[$directive] = false;
227224
}
228225

229226
if (false !== $this->ageDirectives[$directive]) {
227+
$value = min($value ?? PHP_INT_MAX, $expires ?? PHP_INT_MAX);
230228
$value -= $age;
231229
$this->ageDirectives[$directive] = null !== $this->ageDirectives[$directive] ? min($this->ageDirectives[$directive], $value) : $value;
232230
}

‎src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php

Lines changed: 61 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,64 @@ public function testSharedMaxAgeNotSetIfNotSetInMainRequest()
7676
$this->assertFalse($response->headers->hasCacheControlDirective('s-maxage'));
7777
}
7878

79+
public function testExpiresHeaderUpdatedFromMaxAge()
80+
{
81+
$cacheStrategy = new ResponseCacheStrategy();
82+
83+
$response1 = new Response();
84+
$response1->setExpires(new \DateTime('+ 1 hour'));
85+
$response1->setPublic();
86+
$cacheStrategy->add($response1);
87+
88+
$response = new Response();
89+
$response->setMaxAge(0);
90+
$response->setSharedMaxAge(86400);
91+
$cacheStrategy->update($response);
92+
93+
$this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
94+
$this->assertSame('3600', $response->headers->getCacheControlDirective('s-maxage'));
95+
96+
// Expires header must be same as Date header because "max-age" is 0.
97+
$this->assertSame($response->headers->get('Date'), $response->headers->get('Expires'));
98+
}
99+
100+
public function testMaxAgeUpdatedFromExpiresHeader()
101+
{
102+
$cacheStrategy = new ResponseCacheStrategy();
103+
104+
$response1 = new Response();
105+
$response1->setExpires(new \DateTime('+ 1 hour'));
106+
$response1->setPublic();
107+
$cacheStrategy->add($response1);
108+
109+
$response = new Response();
110+
$response->setMaxAge(86400);
111+
$cacheStrategy->update($response);
112+
113+
$this->assertSame('3600', $response->headers->getCacheControlDirective('max-age'));
114+
$this->assertNull($response->headers->getCacheControlDirective('s-maxage'));
115+
$this->assertSame((new \DateTime('+ 1 hour'))->format('D, d M Y H:i:s').' GMT', $response->headers->get('Expires'));
116+
}
117+
118+
public function testMaxAgeAndSharedMaxAgeUpdatedFromExpiresHeader()
119+
{
120+
$cacheStrategy = new ResponseCacheStrategy();
121+
122+
$response1 = new Response();
123+
$response1->setExpires(new \DateTime('+ 1 day'));
124+
$response1->setPublic();
125+
$cacheStrategy->add($response1);
126+
127+
$response = new Response();
128+
$response->setMaxAge(3600);
129+
$response->setSharedMaxAge(86400);
130+
$cacheStrategy->update($response);
131+
132+
$this->assertSame('3600', $response->headers->getCacheControlDirective('max-age'));
133+
$this->assertSame('86400', $response->headers->getCacheControlDirective('s-maxage'));
134+
$this->assertSame((new \DateTime('+ 1 hour'))->format('D, d M Y H:i:s').' GMT', $response->headers->get('Expires'));
135+
}
136+
79137
public function testMainResponseNotCacheableWhenEmbeddedResponseRequiresValidation()
80138
{
81139
$cacheStrategy = new ResponseCacheStrategy();
@@ -243,7 +301,7 @@ public function testResponseIsExpirableButNotValidateableWhenMainResponseCombine
243301
*
244302
* @dataProvider cacheControlMergingProvider
245303
*/
246-
public function testCacheControlMerging(array $expects, array $master, array $surrogates)
304+
public function testCacheControlMerging(array $expects, array $main, array $surrogates)
247305
{
248306
$cacheStrategy = new ResponseCacheStrategy();
249307
$buildResponse = function ($config) {
@@ -289,7 +347,7 @@ public function testCacheControlMerging(array $expects, array $master, array $su
289347
$cacheStrategy->add($buildResponse($config));
290348
}
291349

292-
$response = $buildResponse($master);
350+
$response = $buildResponse($main);
293351
$cacheStrategy->update($response);
294352

295353
foreach ($expects as $key => $value) {
@@ -371,7 +429,7 @@ public static function cacheControlMergingProvider()
371429
];
372430

373431
yield 'merge max-age and s-maxage' => [
374-
['public' => true, 'max-age' => '60'],
432+
['public' => true, 'max-age' => null, 's-maxage' => '60'],
375433
['public' => true, 's-maxage' => 3600],
376434
[
377435
['public' => true, 'max-age' => 60],

0 commit comments

Comments
 (0)
0