diff --git a/CHANGELOG.md b/CHANGELOG.md index e388e6bf..7557ab90 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 f08bc98a..6d1ec43e 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 850fe184..613311f7 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 c531c421..a67e1acc 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 898320cc..823d6f96 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 7c86198d..07a21bfa 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();