Add fallbackToSmaxage flag to CustomTtlListener and check for isCacheable before calling store

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
 ------
 

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:
 

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

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);
+        }
     }
 
     /**

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.
      */

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();