From f613d446a463bcb8026092899d9ad85434a8b0ad Mon Sep 17 00:00:00 2001 From: Malte Wunsch Date: Tue, 19 Nov 2024 13:16:45 +0100 Subject: [PATCH] Read attributes as provided by the event object, and pass on attributes to other event handlers (Case 177753) (#15) This adapts https://github.com/webfactory/WebfactoryWfdMetaBundle/pull/53: > This PR uses the new API introduced in https://github.com/symfony/symfony/pull/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. --- composer.json | 2 +- src/NotModified/EventListener.php | 37 ++++++----------- tests/NotModified/EventListenerTest.php | 53 +++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/composer.json b/composer.json index bc07192..3a3652a 100644 --- a/composer.json +++ b/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": { diff --git a/src/NotModified/EventListener.php b/src/NotModified/EventListener.php index cd6030a..68b8951 100644 --- a/src/NotModified/EventListener.php +++ b/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; - } } diff --git a/tests/NotModified/EventListenerTest.php b/tests/NotModified/EventListenerTest.php index 38e312f..c79715c 100644 --- a/tests/NotModified/EventListenerTest.php +++ b/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