Read attributes as provided by the event object, and pass on attributes to other event handlers (Case 177753) (#15)

This adapts
webfactory/WebfactoryWfdMetaBundle#53:

> This PR uses the new API introduced in
symfony/symfony#46001 to read controller
attributes through the `ControllerEvent`, and to make them available to
other event handlers when replacing the controller.
>
> This is necessary when using the `Send304IfNotModified` attribute in
combination with `\Symfony\Component\HttpKernel\Attribute\Cache`.
Without this change,
`\Symfony\Component\HttpKernel\EventListener\CacheAttributeListener`
will not set `Cache` headers accordingly. The result is that you may get
`304 Not Modified` responses on conditional requests with
`If-Modified-Since`, but these are treated as `stale/cache` only in the
HttpCache and have a `Cache-Control: must-revalidate, private` header.

And adds some tests.

Author: MalteWunsch

composer.json

@@ -24,7 +24,7 @@
         "symfony/dependency-injection": "^5.0 | ^6.0 | ^7.0",
         "symfony/deprecation-contracts": "^2.0|^3.0",
         "symfony/http-foundation": "^5.0 | ^6.0 | ^7.0",
-        "symfony/http-kernel": "^5.3 | ^6.0 | ^7.0"
+        "symfony/http-kernel": "^6.4 | ^7.0"
     },
 
     "require-dev": {

src/NotModified/EventListener.php

@@ -9,7 +9,6 @@
 
 namespace Webfactory\HttpCacheBundle\NotModified;
 
-use ReflectionMethod;
 use SplObjectStorage;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Response;
@@ -46,14 +45,16 @@ public function __construct(
      */
     public function onKernelController(ControllerEvent $event): void
     {
-        $annotation = $this->findAnnotation($event->getController());
-        if (!$annotation) {
+        $attributes = $event->getAttributes(ReplaceWithNotModifiedResponse::class);
+
+        if (!$attributes) {
             return;
         }
 
+        $attribute = $attributes[0];
         $request = $event->getRequest();
-        $annotation->setContainer($this->container);
-        $lastModified = $annotation->determineLastModified($request);
+        $attribute->setContainer($this->container);
+        $lastModified = $attribute->determineLastModified($request);
         if (!$lastModified) {
             return;
         }
@@ -70,9 +71,12 @@ public function onKernelController(ControllerEvent $event): void
         $response->setLastModified($lastModified);
 
         if ($response->isNotModified($request)) {
-            $event->setController(function () use ($response) {
-                return $response;
-            });
+            $event->setController(
+                function () use ($response) {
+                    return $response;
+                },
+                $event->getAttributes()
+            );
         }
     }
 
@@ -89,21 +93,4 @@ public function onKernelResponse(ResponseEvent $event): void
             $response->setLastModified($this->lastModified[$request]);
         }
     }
-
-    /**
-     * @param $controllerCallable callable PHP callback pointing to the method to reflect on.
-     */
-    private function findAnnotation(callable $controllerCallable): ?ReplaceWithNotModifiedResponse
-    {
-        if (!is_array($controllerCallable)) {
-            return null;
-        }
-
-        [$class, $methodName] = $controllerCallable;
-        $method = new ReflectionMethod($class, $methodName);
-
-        $attributes = $method->getAttributes(ReplaceWithNotModifiedResponse::class);
-
-        return $attributes ? $attributes[0]->newInstance() : null;
-    }
 }

tests/NotModified/EventListenerTest.php

@@ -11,6 +11,7 @@
 
 use Closure;
 use DateTime;
+use Error;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -159,6 +160,51 @@ public function eventListenerDifferentiatesBetweenMultipleRequests(): void
         self::assertNull($anotherResponse->getLastModified());
     }
 
+    /** @test */
+    public function onKernelControllerSearchesEventInsteadOfControllerForAttribute(): void
+    {
+        // setup an event that should lead to a NotModified response
+        $this->request->headers->set('If-Modified-Since', '-1 hour');
+        $this->filterControllerEvent = new ControllerEvent(
+            $this->kernel,
+            [DummyController::class, 'oneDayAgoModifiedLastModifiedAction'],
+            $this->request,
+            HttpKernelInterface::MAIN_REQUEST
+        );
+
+        // now simulate another EventListener has replaced the response of the controller inside the event, but has
+        // saved the original attributes in the event.
+        $this->filterControllerEvent->setController(
+            function () {
+                return new Response();
+            },
+            [ReplaceWithNotModifiedResponse::class => [new ReplaceWithNotModifiedResponse([OneDayAgoModifiedLastModifiedDeterminator::class])]]
+        );
+
+        $this->eventListener->onKernelController($this->filterControllerEvent);
+
+        self::assertNotModifiedResponse();
+    }
+
+    /** @test */
+    public function onKernelControllerSavesOriginalControllerAttributesWhenReplacingTheController(): void
+    {
+        $this->request->headers->set('If-Modified-Since', '-1 hour');
+
+        $this->exerciseOnKernelController([DummyController::class, 'oneDayAgoModifiedLastModifiedAction']);
+
+        self::assertNotEmpty($this->filterControllerEvent->getAttributes());
+    }
+
+    /** @test */
+    public function onKernelControllerThrowsExceptionIfAttributeIsFoundMoreThanOnce(): void
+    {
+        self::expectException(Error::class);
+        self::expectExceptionMessageMatches('/ReplaceWithNotModifiedResponse/');
+
+        $this->exerciseOnKernelController([DummyController::class, 'actionWithMoreThanOneAttribute']);
+    }
+
     private function exerciseOnKernelController(array $callable): void
     {
         $this->callable = $callable;
@@ -213,6 +259,13 @@ public static function fixedDateAgoModifiedLastModifiedDeterminatorAction(): Res
     {
         return new Response();
     }
+
+    #[ReplaceWithNotModifiedResponse([AbstainingLastModifiedDeterminator::class])]
+    #[ReplaceWithNotModifiedResponse([OneDayAgoModifiedLastModifiedDeterminator::class])]
+    public static function actionWithMoreThanOneAttribute(): Response
+    {
+        return new Response();
+    }
 }
 
 final class AbstainingLastModifiedDeterminator implements LastModifiedDeterminator