From 6957deed6a2190159f35466ccb2c1702b0a80803 Mon Sep 17 00:00:00 2001 From: Alexander Schranz Date: Mon, 22 Jul 2024 16:22:31 +0200 Subject: [PATCH 1/6] Improve CustomTtlListener, https://github.com/FriendsOfSymfony/FOSHttpCache/pull/575 * Only call store when response still is cacheable * Add flag to disable fallback to s-maxage to changelog --- CHANGELOG.md | 6 +++ doc/symfony-cache-configuration.rst | 8 ++++ src/SymfonyCache/CustomTtlListener.php | 33 +++++++++++------ .../EventDispatchingHttpCache.php | 6 ++- .../EventDispatchingHttpCacheTestCase.php | 33 +++++++++++++++++ .../SymfonyCache/CustomTtlListenerTest.php | 37 +++++++++++++++++++ 6 files changed, 111 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e388e6bfb..7557ab900 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ Changelog See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpCache/releases). +2.15.4 +------ + +* Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling fallback to s-maxage if custom TTL header is not defined on the response. +* 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 + 2.15.3 ------ diff --git a/doc/symfony-cache-configuration.rst b/doc/symfony-cache-configuration.rst index f08bc98aa..6d1ec43e5 100644 --- a/doc/symfony-cache-configuration.rst +++ b/doc/symfony-cache-configuration.rst @@ -356,6 +356,14 @@ but you can customize that in the listener constructor:: new CustomTtlListener('My-TTL-Header'); The custom header is removed before sending the response to the client. +You can enable keeping the custom header with the `keepTtlHeader` parameter:: + + new CustomTtlListener('My-TTL-Header', keepTtlHeader: true); + +By default if the custom ttl header is not present, the listener falls back to the s-maxage cache-control directive. +To disable this behavior, you can set the `fallbackToSmaxage` parameter to false:: + + new CustomTtlListener('My-TTL-Header', fallbackToSmaxage: false); .. _symfony-cache x-debugging: diff --git a/src/SymfonyCache/CustomTtlListener.php b/src/SymfonyCache/CustomTtlListener.php index 850fe1845..613311f7a 100644 --- a/src/SymfonyCache/CustomTtlListener.php +++ b/src/SymfonyCache/CustomTtlListener.php @@ -33,6 +33,11 @@ class CustomTtlListener implements EventSubscriberInterface */ private $keepTtlHeader; + /** + * @var bool + */ + private $fallbackToSmaxage; + /** * Header used for backing up the s-maxage. * @@ -41,13 +46,15 @@ class CustomTtlListener implements EventSubscriberInterface public const SMAXAGE_BACKUP = 'FOS-Smaxage-Backup'; /** - * @param string $ttlHeader The header name that is used to specify the time to live - * @param bool $keepTtlHeader Keep the custom TTL header on the response for later usage (e.g. debugging) + * @param string $ttlHeader The header name that is used to specify the time to live + * @param bool $keepTtlHeader Keep the custom TTL header on the response for later usage (e.g. debugging) + * @param bool $fallbackToSmaxage If the custom TTL header is not set, should s-maxage be used? */ - public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL', $keepTtlHeader = false) + public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL', $keepTtlHeader = false, $fallbackToSmaxage = true) { $this->ttlHeader = $ttlHeader; $this->keepTtlHeader = $keepTtlHeader; + $this->fallbackToSmaxage = $fallbackToSmaxage; } /** @@ -59,15 +66,23 @@ public function __construct($ttlHeader = 'X-Reverse-Proxy-TTL', $keepTtlHeader = public function useCustomTtl(CacheEvent $e) { $response = $e->getResponse(); - if (!$response->headers->has($this->ttlHeader)) { + + if (!$response->headers->has($this->ttlHeader) + && true === $this->fallbackToSmaxage + ) { return; } + $backup = $response->headers->hasCacheControlDirective('s-maxage') ? $response->headers->getCacheControlDirective('s-maxage') : 'false' ; $response->headers->set(static::SMAXAGE_BACKUP, $backup); - $response->setTtl($response->headers->get($this->ttlHeader)); + $response->setTtl( + $response->headers->has($this->ttlHeader) + ? $response->headers->get($this->ttlHeader) + : 0 + ); } /** @@ -76,11 +91,6 @@ public function useCustomTtl(CacheEvent $e) public function cleanResponse(CacheEvent $e) { $response = $e->getResponse(); - if (!$response->headers->has($this->ttlHeader) - && !$response->headers->has(static::SMAXAGE_BACKUP) - ) { - return; - } if ($response->headers->has(static::SMAXAGE_BACKUP)) { $smaxage = $response->headers->get(static::SMAXAGE_BACKUP); @@ -89,12 +99,13 @@ public function cleanResponse(CacheEvent $e) } else { $response->headers->addCacheControlDirective('s-maxage', $smaxage); } + + $response->headers->remove(static::SMAXAGE_BACKUP); } if (!$this->keepTtlHeader) { $response->headers->remove($this->ttlHeader); } - $response->headers->remove(static::SMAXAGE_BACKUP); } public static function getSubscribedEvents(): array diff --git a/src/SymfonyCache/EventDispatchingHttpCache.php b/src/SymfonyCache/EventDispatchingHttpCache.php index c531c4211..a67e1accf 100644 --- a/src/SymfonyCache/EventDispatchingHttpCache.php +++ b/src/SymfonyCache/EventDispatchingHttpCache.php @@ -109,7 +109,11 @@ protected function store(Request $request, Response $response) { $response = $this->dispatch(Events::PRE_STORE, $request, $response); - parent::store($request, $response); + // CustomTtlListener or other Listener or Subscribers might have changed the response to become non-cacheable, revalidate. + // 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 + if ($response->isCacheable()) { + parent::store($request, $response); + } } /** diff --git a/src/Test/EventDispatchingHttpCacheTestCase.php b/src/Test/EventDispatchingHttpCacheTestCase.php index 898320cc4..823d6f96f 100644 --- a/src/Test/EventDispatchingHttpCacheTestCase.php +++ b/src/Test/EventDispatchingHttpCacheTestCase.php @@ -208,6 +208,7 @@ public function testPreStoreCalled() { $request = Request::create('/foo', 'GET'); $response = new Response(); + $response->setTtl(42); $httpCache = $this->getHttpCachePartialMock(); $testListener = new TestListener($this, $httpCache, $request); @@ -230,6 +231,7 @@ public function testPreStoreResponse() $request = Request::create('/foo', 'GET'); $regularResponse = new Response(); $preStoreResponse = new Response(); + $preStoreResponse->setTtl(42); $httpCache = $this->getHttpCachePartialMock(); $testListener = new TestListener($this, $httpCache, $request); @@ -245,6 +247,37 @@ public function testPreStoreResponse() $this->assertEquals(1, $testListener->preStoreCalls); } + /** + * Assert that preStore response is used when provided. + */ + public function testPreStoreResponseNoStore() + { + $request = Request::create('/foo', 'GET'); + $regularResponse = new Response(); + $regularResponse->setTtl(42); + $preStoreResponse = new Response(); + $preStoreResponse->setTtl(0); + + $httpCache = $this->getHttpCachePartialMock(); + $testListener = new TestListener($this, $httpCache, $request); + $testListener->preStoreResponse = $preStoreResponse; + $httpCache->addSubscriber($testListener); + + $store = $this->createMock(StoreInterface::class); + $store->expects($this->never())->method('write'); + + $refHttpCache = new \ReflectionClass(HttpCache::class); + $refStore = $refHttpCache->getProperty('store'); + $refStore->setAccessible(true); + $refStore->setValue($httpCache, $store); + + $refHttpCache = new \ReflectionObject($httpCache); + $method = $refHttpCache->getMethod('store'); + $method->setAccessible(true); + $method->invokeArgs($httpCache, [$request, $regularResponse]); + $this->assertEquals(1, $testListener->preStoreCalls); + } + /** * Assert that preInvalidate is called. */ diff --git a/tests/Unit/SymfonyCache/CustomTtlListenerTest.php b/tests/Unit/SymfonyCache/CustomTtlListenerTest.php index 7c86198db..07a21bfa1 100644 --- a/tests/Unit/SymfonyCache/CustomTtlListenerTest.php +++ b/tests/Unit/SymfonyCache/CustomTtlListenerTest.php @@ -87,6 +87,23 @@ public function testNoCustomTtl() $this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP)); } + public function testNoCustomTtlNoFallback() + { + $ttlListener = new CustomTtlListener('X-Reverse-Proxy-TTL', false, false); + $request = Request::create('http://example.com/foo', 'GET'); + $response = new Response('', 200, [ + 'Cache-Control' => 'max-age=30, s-maxage=33', + ]); + $event = new CacheEvent($this->kernel, $request, $response); + + $ttlListener->useCustomTtl($event); + $response = $event->getResponse(); + + $this->assertInstanceOf(Response::class, $response); + $this->assertSame('0', $response->headers->getCacheControlDirective('s-maxage')); + $this->assertTrue($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP)); + } + public function testCleanup() { $ttlListener = new CustomTtlListener(); @@ -108,6 +125,26 @@ public function testCleanup() $this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP)); } + public function testCleanupNoReverseProxyTtl() + { + $ttlListener = new CustomTtlListener(); + $request = Request::create('http://example.com/foo', 'GET'); + $response = new Response('', 200, [ + 'Cache-Control' => 's-maxage=0, max-age=30', + CustomTtlListener::SMAXAGE_BACKUP => '60', + ]); + $event = new CacheEvent($this->kernel, $request, $response); + + $ttlListener->cleanResponse($event); + $response = $event->getResponse(); + + $this->assertInstanceOf(Response::class, $response); + $this->assertTrue($response->headers->hasCacheControlDirective('s-maxage')); + $this->assertSame('60', $response->headers->getCacheControlDirective('s-maxage')); + $this->assertFalse($response->headers->has('X-Reverse-Proxy-TTL')); + $this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP)); + } + public function testCleanupNoSmaxage() { $ttlListener = new CustomTtlListener(); From 1f955226d3b3027c3ec729a24e79bdf15d314af8 Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Tue, 23 Jul 2024 16:51:18 +0200 Subject: [PATCH 2/6] adjust changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7557ab900..cfb2f3978 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,8 @@ Changelog See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpCache/releases). -2.15.4 ------- +2.16 (unreleased) +---- * Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling fallback to s-maxage if custom TTL header is not defined on the response. * 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 From c19bdcff595675326bedd5d488f2f3f296194f88 Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Tue, 23 Jul 2024 17:24:12 +0200 Subject: [PATCH 3/6] fix custom ttl with symfony HttpCache to work regardless of s-maxage --- CHANGELOG.md | 19 ++++++++++++++++--- src/SymfonyCache/CustomTtlListener.php | 9 ++++----- .../EventDispatchingHttpCache.php | 10 ++++++++++ src/SymfonyCache/Events.php | 4 ++++ .../Symfony/EventDispatchingHttpCacheTest.php | 7 ++++--- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfb2f3978..431ea0476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,11 +3,24 @@ Changelog See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpCache/releases). -2.16 (unreleased) +2.16 ---- -* Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling fallback to s-maxage if custom TTL header is not defined on the response. -* 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 +### Symfony HttpCache + +* Add events `PRE_FORWARD` and `POST_FORWARD` to allow event listeners to alter + the request before and after it is sent to the backend. +* Changed CustomTtlListener to use the `POST_FORWARD` event instead of + `PRE_STORE`. Using PRE_STORE, requests that are not considered cacheable by + Symfony were never cached, even when they had a custom TTL header. +* Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling + fallback to s-maxage if custom TTL header is not defined on the response. +* Fix: 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. 2.15.3 ------ diff --git a/src/SymfonyCache/CustomTtlListener.php b/src/SymfonyCache/CustomTtlListener.php index 613311f7a..a0e80aba4 100644 --- a/src/SymfonyCache/CustomTtlListener.php +++ b/src/SymfonyCache/CustomTtlListener.php @@ -78,10 +78,9 @@ public function useCustomTtl(CacheEvent $e) : 'false' ; $response->headers->set(static::SMAXAGE_BACKUP, $backup); - $response->setTtl( - $response->headers->has($this->ttlHeader) - ? $response->headers->get($this->ttlHeader) - : 0 + $response->headers->addCacheControlDirective( + 's-maxage', + $response->headers->get($this->ttlHeader, 0) ); } @@ -111,7 +110,7 @@ public function cleanResponse(CacheEvent $e) public static function getSubscribedEvents(): array { return [ - Events::PRE_STORE => 'useCustomTtl', + Events::POST_FORWARD => 'useCustomTtl', Events::POST_HANDLE => 'cleanResponse', ]; } diff --git a/src/SymfonyCache/EventDispatchingHttpCache.php b/src/SymfonyCache/EventDispatchingHttpCache.php index a67e1accf..9376e7e85 100644 --- a/src/SymfonyCache/EventDispatchingHttpCache.php +++ b/src/SymfonyCache/EventDispatchingHttpCache.php @@ -130,6 +130,16 @@ protected function invalidate(Request $request, $catch = false): Response return parent::invalidate($request, $catch); } + protected function forward(Request $request, bool $catch = false, ?Response $entry = null): Response + { + // do not abort early, if $entry is set this is a validation request + $this->dispatch(Events::PRE_FORWARD, $request, $entry); + + $response = parent::forward($request, $catch, $entry); + + return $this->dispatch(Events::POST_FORWARD, $request, $response); + } + /** * Dispatch an event if there are any listeners. * diff --git a/src/SymfonyCache/Events.php b/src/SymfonyCache/Events.php index cb81d9792..a911b95c3 100644 --- a/src/SymfonyCache/Events.php +++ b/src/SymfonyCache/Events.php @@ -20,6 +20,10 @@ final class Events public const POST_HANDLE = 'fos_http_cache.post_handle'; + public const PRE_FORWARD = 'fos_http_cache.pre_forward'; + + public const POST_FORWARD = 'fos_http_cache.post_forward'; + public const PRE_INVALIDATE = 'fos_http_cache.pre_invalidate'; public const PRE_STORE = 'fos_http_cache.pre_store'; diff --git a/tests/Functional/Symfony/EventDispatchingHttpCacheTest.php b/tests/Functional/Symfony/EventDispatchingHttpCacheTest.php index f4a0e7031..f6a46ae4a 100644 --- a/tests/Functional/Symfony/EventDispatchingHttpCacheTest.php +++ b/tests/Functional/Symfony/EventDispatchingHttpCacheTest.php @@ -43,13 +43,14 @@ public function testEventListeners() $httpKernel = \Mockery::mock(HttpKernelInterface::class) ->shouldReceive('handle') - ->withArgs([$request, HttpKernelInterface::MASTER_REQUEST, true]) ->andReturn($expectedResponse) ->getMock(); $store = \Mockery::mock(StoreInterface::class) + ->shouldReceive('lookup')->andReturn(null)->times(1) + ->shouldReceive('write')->times(1) + ->shouldReceive('unlock')->times(1) // need to declare the cleanup function explicitly to avoid issue between register_shutdown_function and mockery - ->shouldReceive('cleanup') - ->atMost(1) + ->shouldReceive('cleanup')->atMost(1) ->getMock(); $kernel = new AppCache($httpKernel, $store); $kernel->addSubscriber(new CustomTtlListener()); From 383092af1dd889e965238ac2d5575c8f9a7d02bc Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Wed, 24 Jul 2024 09:31:04 +0200 Subject: [PATCH 4/6] tweak changelog --- CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 431ea0476..4fe087d1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,23 +3,23 @@ Changelog See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpCache/releases). -2.16 ----- +2.16.0 +------ ### Symfony HttpCache * Add events `PRE_FORWARD` and `POST_FORWARD` to allow event listeners to alter the request before and after it is sent to the backend. * Changed CustomTtlListener to use the `POST_FORWARD` event instead of - `PRE_STORE`. Using PRE_STORE, requests that are not considered cacheable by + `PRE_STORE`. Using `PRE_STORE`, requests that are not considered cacheable by Symfony were never cached, even when they had a custom TTL header. * Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling - fallback to s-maxage if custom TTL header is not defined on the response. + fallback to `s-maxage` if custom TTL header is not defined on the response. * Fix: 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 + change `s-maxage`, this bug might have led to caching responses that should not have been cached. 2.15.3 From 06bf3a5ca4735e0ddaf853e167c4f6d1bcfc5ddd Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Wed, 24 Jul 2024 09:32:52 +0200 Subject: [PATCH 5/6] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe087d1f..bf687fe69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ Changelog See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpCache/releases). +2.x +=== + 2.16.0 ------ From 79081bb32d93c245ef0114dd6222f97d1d37fd04 Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Wed, 24 Jul 2024 09:38:42 +0200 Subject: [PATCH 6/6] update changelog --- CHANGELOG.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 223ce6f2f..b724c548b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,25 @@ See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpC 3.x === +3.1.0 +----- + +### Symfony HttpCache + +* Add events `PRE_FORWARD` and `POST_FORWARD` to allow event listeners to alter + the request before and after it is sent to the backend. +* Changed CustomTtlListener to use the `POST_FORWARD` event instead of + `PRE_STORE`. Using `PRE_STORE`, requests that are not considered cacheable by + Symfony were never cached, even when they had a custom TTL header. +* Add flag `fallbackToSmaxage` to `CustomTtlListener` to allow controlling + fallback to `s-maxage` if custom TTL header is not defined on the response. +* Fix: 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. + 3.0.0 -----