8000 Fix two edge cases in ResponseCacheStrategy · symfony/symfony@c6e8c07 · GitHub
[go: up one dir, main page]
Skip to content

Commit c6e8c07

Browse files
mpdudefabpot
mpdude
authored and
fabpot
committed
Fix two edge cases in ResponseCacheStrategy
1 parent 53a9111 commit c6e8c07

File tree

2 files changed

+42
-1
lines changed

2 files changed

+42
-1
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class ResponseCacheStrategy implements ResponseCacheStrategyInterface
3939
*/
4040
public function add(Response $response)
4141
{
42-
if ($response->isValidateable()) {
42+
if ($response->isValidateable() || !$response->isCacheable()) {
4343
$this->cacheable = false;
4444
} else {
4545
$maxAge = $response->getMaxAge();

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,4 +137,45 @@ public function testMasterResponseWithExpirationIsUnchangedWhenThereIsNoEmbedded
137137

138138
$this->assertTrue($masterResponse->isFresh());
139139
}
140+
141+
public function testMasterResponseIsNotCacheableWhenEmbeddedResponseIsNotCacheable()
142+
{
143+
$cacheStrategy = new ResponseCacheStrategy();
144+
145+
$masterResponse = new Response();
146+
$masterResponse->setSharedMaxAge(3600); // Public, cacheable
147+
148+
/* This response has no validation or expiration information.
149+
That makes it uncacheable, it is always stale.
150+
(It does *not* make this private, though.) */
151+
$embeddedResponse = new Response();
152+
$this->assertFalse($embeddedResponse->isFresh()); // not fresh, as no lifetime is provided
153+
154+
$cacheStrategy->add($embeddedResponse);
155+
$cacheStrategy->update($masterResponse);
156+
157+
$this->assertTrue($masterResponse->headers->hasCacheControlDirective('no-cache'));
158+
$this->assertTrue($masterResponse->headers->hasCacheControlDirective('must-revalidate'));
159+
$this->assertFalse($masterResponse->isFresh());
160+
}
161+
162+
public function testEmbeddingPrivateResponseMakesMainResponsePrivate()
163+
{
164+
$cacheStrategy = new ResponseCacheStrategy();
165+
166+
$masterResponse = new Response();
167+
$masterResponse->setSharedMaxAge(3600); // public, cacheable
168+
169+
// The embedded response might for example contain per-user data that remains valid for 60 seconds
170+
$embeddedResponse = new Response();
171+
$embeddedResponse->setPrivate();
172+
$embeddedResponse->setMaxAge(60); // this would implicitly set "private" as well, but let's be explicit
173+
174+
$cacheStrategy->add($embeddedResponse);
175+
$cacheStrategy->update($masterResponse);
176+
177+
$this->assertTrue($masterResponse->headers->hasCacheControlDirective('private'));
178+
// Not sure if we should pass "max-age: 60" in this case, as long as the response is private and
179+
// that's the more conservative of both the master and embedded response...?
180+
}
140181
}

0 commit comments

Comments
 (0)
0