Skip to content

Commit 6957dee

Browse files
alexander-schranzdbu
alexander-schranz
authored and
dbu
committed
Improve CustomTtlListener, #575
* Only call store when response still is cacheable * Add flag to disable fallback to s-maxage to changelog
1 parent cc511e5 commit 6957dee

File tree

6 files changed

+111
-12
lines changed

6 files changed

+111
-12
lines changed

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,12 @@ Changelog
33

44
See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpCache/releases).
55

6+
2.15.4
7+
------
8+
9+
* Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling fallback to s-maxage if custom TTL header is not defined on the response.
10+
* Fix for Symfony HttpCache: Do not call store if Response object is not longer cacheable after event listeners. If you use the custom TTL system, this is only a performance improvement, because the TTL of the response would still be 0. With a custom listener that changes the response explicitly to not be cached but does not change s-maxage, this bug might have led to caching responses that should not have been cached
11+
612
2.15.3
713
------
814

‎doc/symfony-cache-configuration.rst‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,14 @@ but you can customize that in the listener constructor::
356356
new CustomTtlListener('My-TTL-Header');
357357

358358
The custom header is removed before sending the response to the client.
359+
You can enable keeping the custom header with the `keepTtlHeader` parameter::
360+
361+
new CustomTtlListener('My-TTL-Header', keepTtlHeader: true);
362+
363+
By default if the custom ttl header is not present, the listener falls back to the s-maxage cache-control directive.
364+
To disable this behavior, you can set the `fallbackToSmaxage` parameter to false::
365+
366+
new CustomTtlListener('My-TTL-Header', fallbackToSmaxage: false);
359367

360368
.. _symfony-cache x-debugging:
361369

‎src/SymfonyCache/CustomTtlListener.php‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ class CustomTtlListener implements EventSubscriberInterface
3333
*/
3434
private $keepTtlHeader;
3535

36+
/**
37+
* @var bool
38+
*/
39+
private $fallbackToSmaxage;
40+
3641
/**
3742
* Header used for backing up the s-maxage.
3843
*
@@ -41,13 +46,15 @@ class CustomTtlListener implements EventSubscriberInterface
4146
public const SMAXAGE_BACKUP = 'FOS-Smaxage-Backup';
4247

4348
/**
44-
* @param string $ttlHeader The header name that is used to specify the time to live
45-
* @param bool $keepTtlHeader Keep the custom TTL header on the response for later usage (e.g. debugging)
49+
* @param string $ttlHeader The header name that is used to specify the time to live
50+
* @param bool $keepTtlHeader Keep the custom TTL header on the response for later usage (e.g. debugging)
51+
* @param bool $fallbackToSmaxage If the custom TTL header is not set, should s-maxage be used?
4652
*/
47-
public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL', $keepTtlHeader = false)
53+
public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL', $keepTtlHeader = false, $fallbackToSmaxage = true)
4854
{
4955
$this->ttlHeader = $ttlHeader;
5056
$this->keepTtlHeader = $keepTtlHeader;
57+
$this->fallbackToSmaxage = $fallbackToSmaxage;
5158
}
5259

5360
/**
@@ -59,15 +66,23 @@ public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL', $keepTtlHeader =
5966
public function useCustomTtl(CacheEvent $e)
6067
{
6168
$response = $e->getResponse();
62-
if (!$response->headers->has($this->ttlHeader)) {
69+
70+
if (!$response->headers->has($this->ttlHeader)
71+
&& true === $this->fallbackToSmaxage
72+
) {
6373
return;
6474
}
75+
6576
$backup = $response->headers->hasCacheControlDirective('s-maxage')
6677
? $response->headers->getCacheControlDirective('s-maxage')
6778
: 'false'
6879
;
6980
$response->headers->set(static::SMAXAGE_BACKUP, $backup);
70-
$response->setTtl($response->headers->get($this->ttlHeader));
81+
$response->setTtl(
82+
$response->headers->has($this->ttlHeader)
83+
? $response->headers->get($this->ttlHeader)
84+
: 0
85+
);
7186
}
7287

7388
/**
@@ -76,11 +91,6 @@ public function useCustomTtl(CacheEvent $e)
7691
public function cleanResponse(CacheEvent $e)
7792
{
7893
$response = $e->getResponse();
79-
if (!$response->headers->has($this->ttlHeader)
80-
&& !$response->headers->has(static::SMAXAGE_BACKUP)
81-
) {
82-
return;
83-
}
8494

8595
if ($response->headers->has(static::SMAXAGE_BACKUP)) {
8696
$smaxage = $response->headers->get(static::SMAXAGE_BACKUP);
@@ -89,12 +99,13 @@ public function cleanResponse(CacheEvent $e)
8999
} else {
90100
$response->headers->addCacheControlDirective('s-maxage', $smaxage);
91101
}
102+
103+
$response->headers->remove(static::SMAXAGE_BACKUP);
92104
}
93105

94106
if (!$this->keepTtlHeader) {
95107
$response->headers->remove($this->ttlHeader);
96108
}
97-
$response->headers->remove(static::SMAXAGE_BACKUP);
98109
}
99110

100111
public static function getSubscribedEvents(): array

‎src/SymfonyCache/EventDispatchingHttpCache.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,11 @@ protected function store(Request $request, Response $response)
109109
{
110110
$response = $this->dispatch(Events::PRE_STORE, $request, $response);
111111

112-
parent::store($request, $response);
112+
// CustomTtlListener or other Listener or Subscribers might have changed the response to become non-cacheable, revalidate.
113+
// Only call store for a cacheable response like Symfony core does: https://github.com/symfony/symfony/blob/v7.1.2/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L409
114+
if ($response->isCacheable()) {
115+
parent::store($request, $response);
116+
}
113117
}
114118

115119
/**

‎src/Test/EventDispatchingHttpCacheTestCase.php‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ public function testPreStoreCalled()
208208
{
209209
$request = Request::create('/foo', 'GET');
210210
$response = new Response();
211+
$response->setTtl(42);
211212

212213
$httpCache = $this->getHttpCachePartialMock();
213214
$testListener = new TestListener($this, $httpCache, $request);
@@ -230,6 +231,7 @@ public function testPreStoreResponse()
230231
$request = Request::create('/foo', 'GET');
231232
$regularResponse = new Response();
232233
$preStoreResponse = new Response();
234+
$preStoreResponse->setTtl(42);
233235

234236
$httpCache = $this->getHttpCachePartialMock();
235237
$testListener = new TestListener($this, $httpCache, $request);
@@ -245,6 +247,37 @@ public function testPreStoreResponse()
245247
$this->assertEquals(1, $testListener->preStoreCalls);
246248
}
247249

250+
/**
251+
* Assert that preStore response is used when provided.
252+
*/
253+
public function testPreStoreResponseNoStore()
254+
{
255+
$request = Request::create('/foo', 'GET');
256+
$regularResponse = new Response();
257+
$regularResponse->setTtl(42);
258+
$preStoreResponse = new Response();
259+
$preStoreResponse->setTtl(0);
260+
261+
$httpCache = $this->getHttpCachePartialMock();
262+
$testListener = new TestListener($this, $httpCache, $request);
263+
$testListener->preStoreResponse = $preStoreResponse;
264+
$httpCache->addSubscriber($testListener);
265+
266+
$store = $this->createMock(StoreInterface::class);
267+
$store->expects($this->never())->method('write');
268+
269+
$refHttpCache = new \ReflectionClass(HttpCache::class);
270+
$refStore = $refHttpCache->getProperty('store');
271+
$refStore->setAccessible(true);
272+
$refStore->setValue($httpCache, $store);
273+
274+
$refHttpCache = new \ReflectionObject($httpCache);
275+
$method = $refHttpCache->getMethod('store');
276+
$method->setAccessible(true);
277+
$method->invokeArgs($httpCache, [$request, $regularResponse]);
278+
$this->assertEquals(1, $testListener->preStoreCalls);
279+
}
280+
248281
/**
249282
* Assert that preInvalidate is called.
250283
*/

‎tests/Unit/SymfonyCache/CustomTtlListenerTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,23 @@ public function testNoCustomTtl()
8787
$this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP));
8888
}
8989

90+
public function testNoCustomTtlNoFallback()
91+
{
92+
$ttlListener = new CustomTtlListener('X-Reverse-Proxy-TTL', false, false);
93+
$request = Request::create('http://example.com/foo', 'GET');
94+
$response = new Response('', 200, [
95+
'Cache-Control' => 'max-age=30, s-maxage=33',
96+
]);
97+
$event = new CacheEvent($this->kernel, $request, $response);
98+
99+
$ttlListener->useCustomTtl($event);
100+
$response = $event->getResponse();
101+
102+
$this->assertInstanceOf(Response::class, $response);
103+
$this->assertSame('0', $response->headers->getCacheControlDirective('s-maxage'));
104+
$this->assertTrue($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP));
105+
}
106+
90107
public function testCleanup()
91108
{
92109
$ttlListener = new CustomTtlListener();
@@ -108,6 +125,26 @@ public function testCleanup()
108125
$this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP));
109126
}
110127

128+
public function testCleanupNoReverseProxyTtl()
129+
{
130+
$ttlListener = new CustomTtlListener();
131+
$request = Request::create('http://example.com/foo', 'GET');
132+
$response = new Response('', 200, [
133+
'Cache-Control' => 's-maxage=0, max-age=30',
134+
CustomTtlListener::SMAXAGE_BACKUP => '60',
135+
]);
136+
$event = new CacheEvent($this->kernel, $request, $response);
137+
138+
$ttlListener->cleanResponse($event);
139+
$response = $event->getResponse();
140+
141+
$this->assertInstanceOf(Response::class, $response);
142+
$this->assertTrue($response->headers->hasCacheControlDirective('s-maxage'));
143+
$this->assertSame('60', $response->headers->getCacheControlDirective('s-maxage'));
144+
$this->assertFalse($response->headers->has('X-Reverse-Proxy-TTL'));
145+
$this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP));
146+
}
147+
111148
public function testCleanupNoSmaxage()
112149
{
113150
$ttlListener = new CustomTtlListener();

0 commit comments

Comments
 (0)