Merge pull request #578 from FriendsOfSymfony/2-to-3

2 to 3
FriendsOfSymfony · Jul 24, 2024 · dba8c46 · dba8c46
2 parents e823564 + 79081bb
commit dba8c46
Show file tree

Hide file tree

Showing 8 changed files with 158 additions and 15 deletions.
diff --git 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
 -----
 
@@ -21,6 +40,25 @@ See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpC
 2.x
 ===
 
+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
+  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/doc/symfony-cache-configuration.rst b/doc/symfony-cache-configuration.rst
@@ -351,6 +351,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
@@ -37,6 +37,10 @@ public function __construct(
          * Keep the custom TTL header on the response for later usage (e.g. debugging).
          */
         private readonly bool $keepTtlHeader = false,
+        /**
+         * If the custom TTL header is not set, should s-maxage be used?
+         */
+        private readonly bool $fallbackToSmaxage = true,
     ) {
     }
 
@@ -52,15 +56,22 @@ public function useCustomTtl(CacheEvent $e): void
         if (!$response) {
             return;
         }
-        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->headers->addCacheControlDirective(
+            's-maxage',
+            $response->headers->get($this->ttlHeader, 0)
+        );
     }
 
     /**
@@ -69,14 +80,10 @@ public function useCustomTtl(CacheEvent $e): void
     public function cleanResponse(CacheEvent $e): void
     {
         $response = $e->getResponse();
+
         if (!$response) {
             return;
         }
-        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);
@@ -85,18 +92,19 @@ public function cleanResponse(CacheEvent $e): void
             } 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
     {
         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
@@ -99,7 +99,11 @@ protected function store(Request $request, Response $response): void
     {
         $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);
+        }
     }
 
     /**
@@ -116,6 +120,16 @@ protected function invalidate(Request $request, bool $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
@@ -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/src/Test/EventDispatchingHttpCacheTestCase.php b/src/Test/EventDispatchingHttpCacheTestCase.php
@@ -198,6 +198,7 @@ public function testPreStoreCalled(): void
     {
         $request = Request::create('/foo', 'GET');
         $response = new Response();
+        $response->setTtl(42);
 
         $httpCache = $this->getHttpCachePartialMock();
         $testListener = new TestListener($this, $httpCache, $request);
@@ -220,6 +221,7 @@ public function testPreStoreResponse(): void
         $request = Request::create('/foo', 'GET');
         $regularResponse = new Response();
         $preStoreResponse = new Response();
+        $preStoreResponse->setTtl(42);
 
         $httpCache = $this->getHttpCachePartialMock();
         $testListener = new TestListener($this, $httpCache, $request);
@@ -235,6 +237,37 @@ public function testPreStoreResponse(): void
         $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/Functional/Symfony/EventDispatchingHttpCacheTest.php b/tests/Functional/Symfony/EventDispatchingHttpCacheTest.php
@@ -42,13 +42,14 @@ public function testEventListeners(): void
 
         $httpKernel = \Mockery::mock(HttpKernelInterface::class)
             ->shouldReceive('handle')
-            ->withArgs([$request, HttpKernelInterface::MAIN_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());

diff --git a/tests/Unit/SymfonyCache/CustomTtlListenerTest.php b/tests/Unit/SymfonyCache/CustomTtlListenerTest.php
@@ -84,7 +84,24 @@ public function testNoCustomTtl(): void
         $this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP));
     }
 
-    public function testCleanup(): void
+    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();
         $request = Request::create('http://example.com/foo', 'GET');
@@ -105,7 +122,27 @@ public function testCleanup(): void
         $this->assertFalse($response->headers->has(CustomTtlListener::SMAXAGE_BACKUP));
     }
 
-    public function testCleanupNoSmaxage(): void
+    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();
         $request = Request::create('http://example.com/foo', 'GET');