Merge pull request #577 from FriendsOfSymfony/fix-custom-ttl

fix custom ttl with symfony HttpCache to work regardless of s-maxage

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

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',
         ];
     }

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

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

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