remove annotation reader from FilteredRouteCollectionBuilder

nelmio · Oct 25, 2024 · af59f3a · af59f3a
1 parent de2984a
commit af59f3a
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 103 deletions.
diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md
@@ -18,9 +18,10 @@ public int $negative;
 ...
 ```
 
-This causes the following breaking changes in classes that used on annotations:
+This causes the following breaking changes in classes that used annotations:
 - BC BREAK: Removed 3rd parameter `?Reader $annotationReader` from `Nelmio\ApiDocBundle\Describer\OpenApiPhpDescriber::__construct()`
 - BC BREAK: Removed 2nd parameter `?Reader $annotationReader` from `Nelmio\ApiDocBundle\ModelDescriber\FormModelDescriber::__construct()`
 - BC BREAK: Removed 2nd parameter `?Reader $annotationReader` from `Nelmio\ApiDocBundle\ModelDescriber\JMSModelDescriber::__construct()`
 - BC BREAK: Removed 2nd parameter `?Reader $annotationReader` from `Nelmio\ApiDocBundle\ModelDescriber\ObjectModelDescriber::__construct()`
 - BC BREAK: Removed 1st parameter `?Reader $annotationReader` from `Nelmio\ApiDocBundle\RouteDescriber\FosRestDescriber::__construct()`
+- BC BREAK: Removed 1st parameter `?Reader $annotationReader` from `Nelmio\ApiDocBundle\Routing\FilteredRouteCollectionBuilder::__construct()`
diff --git a/src/DependencyInjection/NelmioApiDocExtension.php b/src/DependencyInjection/NelmioApiDocExtension.php
@@ -33,7 +33,6 @@
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Extension\Extension;
 use Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface;
@@ -143,7 +142,6 @@ public function load(array $configs, ContainerBuilder $container): void
                         (new Definition(FilteredRouteCollectionBuilder::class))
                             ->setArguments(
                                 [
-                                    new Reference('annotation_reader', ContainerInterface::NULL_ON_INVALID_REFERENCE), // Here we use the cached version as we don't deal with @OA annotations in this service
                                     new Reference('nelmio_api_doc.controller_reflector'),
                                     $area,
                                     $areaConfig,

diff --git a/src/Routing/FilteredRouteCollectionBuilder.php b/src/Routing/FilteredRouteCollectionBuilder.php
@@ -11,7 +11,6 @@
 
 namespace Nelmio\ApiDocBundle\Routing;
 
-use Doctrine\Common\Annotations\Reader;
 use Nelmio\ApiDocBundle\Annotation\Areas;
 use Nelmio\ApiDocBundle\Util\ControllerReflector;
 use OpenApi\Annotations\AbstractAnnotation;
@@ -21,8 +20,6 @@
 
 final class FilteredRouteCollectionBuilder
 {
-    private ?Reader $annotationReader;
-
     private ControllerReflector $controllerReflector;
 
     private string $area;
@@ -36,7 +33,6 @@ final class FilteredRouteCollectionBuilder
      * @param array<mixed> $options
      */
     public function __construct(
-        ?Reader $annotationReader,
         ControllerReflector $controllerReflector,
         string $area,
         array $options = []
@@ -64,7 +60,6 @@ public function __construct(
             $options = $normalizedOptions;
         }
 
-        $this->annotationReader = $annotationReader;
         $this->controllerReflector = $controllerReflector;
         $this->area = $area;
         $this->options = $resolver->resolve($options);
@@ -132,27 +127,11 @@ private function matchAnnotation(Route $route): bool
             return false;
         }
 
-        /** @var Areas|null $areas */
-        $areas = $this->getAttributesAsAnnotation($reflectionMethod, Areas::class)[0] ?? null;
-
-        if (null === $areas) {
-            /** @var Areas|null $areas */
-            $areas = $this->getAttributesAsAnnotation($reflectionMethod->getDeclaringClass(), Areas::class)[0] ?? null;
+        $areas = $this->getAttributesAsAnnotation($reflectionMethod, Areas::class)[0]
+            ?? $this->getAttributesAsAnnotation($reflectionMethod->getDeclaringClass(), Areas::class)[0]
+            ?? null;
 
-            if (null === $areas && null !== $this->annotationReader) {
-                /** @var Areas|null $areas */
-                $areas = $this->annotationReader->getMethodAnnotation(
-                    $reflectionMethod,
-                    Areas::class
-                );
-
-                if (null === $areas) {
-                    $areas = $this->annotationReader->getClassAnnotation($reflectionMethod->getDeclaringClass(), Areas::class);
-                }
-            }
-        }
-
-        return (null !== $areas) ? $areas->has($this->area) : false;
+        return null !== $areas && $areas->has($this->area);
     }
 
     private function defaultRouteDisabled(Route $route): bool
@@ -169,13 +148,9 @@ private function defaultRouteDisabled(Route $route): bool
             return false;
         }
 
-        $annotations = null !== $this->annotationReader
-            ? $this->annotationReader->getMethodAnnotations($method)
-            : [];
-
-        $annotations = array_merge($annotations, array_map(function (\ReflectionAttribute $attribute) {
+        $annotations = array_map(function (\ReflectionAttribute $attribute) {
             return $attribute->newInstance();
-        }, $method->getAttributes(AbstractAnnotation::class, \ReflectionAttribute::IS_INSTANCEOF)));
+        }, $method->getAttributes(AbstractAnnotation::class, \ReflectionAttribute::IS_INSTANCEOF));
 
         foreach ($annotations as $annotation) {
             if (false !== strpos(get_class($annotation), 'Nelmio\\ApiDocBundle\\Annotation')

diff --git a/tests/Routing/FilteredRouteCollectionBuilderTest.php b/tests/Routing/FilteredRouteCollectionBuilderTest.php
@@ -11,13 +11,11 @@
 
 namespace Nelmio\ApiDocBundle\Tests\Routing;
 
-use Doctrine\Common\Annotations\AnnotationReader;
-use Doctrine\Common\Annotations\Reader;
 use Nelmio\ApiDocBundle\Annotation\Areas;
 use Nelmio\ApiDocBundle\Annotation\Operation;
 use Nelmio\ApiDocBundle\Routing\FilteredRouteCollectionBuilder;
 use Nelmio\ApiDocBundle\Util\ControllerReflector;
-use OpenApi\Annotations\Parameter;
+use OpenApi\Attributes\Parameter;
 use OpenApi\Context;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
@@ -31,16 +29,6 @@
  */
 class FilteredRouteCollectionBuilderTest extends TestCase
 {
-    /**
-     * @var AnnotationReader|null
-     */
-    private $doctrineAnnotations;
-
-    protected function setUp(): void
-    {
-        $this->doctrineAnnotations = class_exists(AnnotationReader::class) ? new AnnotationReader() : null;
-    }
-
     public function testFilter(): void
     {
         $options = [
@@ -60,7 +48,6 @@ public function testFilter(): void
         }
 
         $routeBuilder = new FilteredRouteCollectionBuilder(
-            $this->doctrineAnnotations,
             $this->createControllerReflector(),
             'areaName',
             $options
@@ -88,7 +75,6 @@ public function testFilterWithDeprecatedArgument(): void
         }
 
         $routeBuilder = new FilteredRouteCollectionBuilder(
-            $this->doctrineAnnotations,
             $this->createControllerReflector(),
             'areaName',
             $pathPattern
@@ -107,7 +93,6 @@ public function testFilterWithInvalidOption(array $options): void
         $this->expectException(InvalidArgumentException::class);
 
         new FilteredRouteCollectionBuilder(
-            $this->doctrineAnnotations,
             $this->createControllerReflector(),
             'areaName',
             $options
@@ -162,7 +147,6 @@ public function testMatchingRoutes(string $name, Route $route, array $options =
         $routes->add($name, $route);
 
         $routeBuilder = new FilteredRouteCollectionBuilder(
-            $this->doctrineAnnotations,
             $this->createControllerReflector(),
             'area',
             $options
@@ -192,28 +176,16 @@ public static function getMatchingRoutes(): \Generator
      * @param array<string, mixed> $options
      */
     #[DataProvider('getMatchingRoutesWithAnnotation')]
-    public function testMatchingRoutesWithAnnotation(string $name, Route $route, array $options = []): void
+    public function testMatchingRoutesWithAnnotation(string $name, Route $route, \ReflectionMethod $reflectionMethod, array $options = []): void
     {
         $routes = new RouteCollection();
         $routes->add($name, $route);
         $area = 'area';
 
-        $reflectionMethodStub = $this->createMock(\ReflectionMethod::class);
         $controllerReflectorStub = $this->createMock(ControllerReflector::class);
-        $controllerReflectorStub->method('getReflectionMethod')->willReturn($reflectionMethodStub);
-
-        $annotationReader = null;
-        if (interface_exists(Reader::class)) {
-            $annotationReader = $this->createMock(Reader::class);
-            $annotationReader
-                ->method('getMethodAnnotation')
-                ->with($reflectionMethodStub, Areas::class)
-                ->willReturn(new Areas(['value' => [$area]]))
-            ;
-        }
+        $controllerReflectorStub->method('getReflectionMethod')->willReturn($reflectionMethod);
 
         $routeBuilder = new FilteredRouteCollectionBuilder(
-            $annotationReader,
             $controllerReflectorStub,
             $area,
             $options
@@ -225,33 +197,26 @@ public function testMatchingRoutesWithAnnotation(string $name, Route $route, arr
 
     public static function getMatchingRoutesWithAnnotation(): \Generator
     {
+        $apiController = new class {
+            #[Areas(['area'])]
+            public function fooAction(): void
+            {
+            }
+        };
         yield from [
-            'with annotation only' => [
+            'with attribute only' => [
                 'r10',
-                new Route('/api/areas/new', ['_controller' => 'ApiController::newAreaAction']),
+                new Route('/api/areas_attributes/new', ['_controller' => 'ApiController::newAreaActionAttributes']),
+                new \ReflectionMethod($apiController, 'fooAction'),
                 ['with_annotation' => true],
             ],
-            'with annotation and path patterns' => [
+            'with attribute and path patterns' => [
                 'r10',
-                new Route('/api/areas/new', ['_controller' => 'ApiController::newAreaAction']),
+                new Route('/api/areas_attributes/new', ['_controller' => 'ApiController::newAreaActionAttributes']),
+                new \ReflectionMethod($apiController, 'fooAction'),
                 ['path_patterns' => ['^/api'], 'with_annotation' => true],
             ],
         ];
-
-        if (\PHP_VERSION_ID < 80000) {
-            yield from [
-                'with attribute only' => [
-                    'r10',
-                    new Route('/api/areas_attributes/new', ['_controller' => 'ApiController::newAreaActionAttributes']),
-                    ['with_annotation' => true],
-                ],
-                'with attribute and path patterns' => [
-                    'r10',
-                    new Route('/api/areas_attributes/new', ['_controller' => 'ApiController::newAreaActionAttributes']),
-                    ['path_patterns' => ['^/api'], 'with_annotation' => true],
-                ],
-            ];
-        }
     }
 
     /**
@@ -264,7 +229,6 @@ public function testNonMatchingRoutes(string $name, Route $route, array $options
         $routes->add($name, $route);
 
         $routeBuilder = new FilteredRouteCollectionBuilder(
-            $this->doctrineAnnotations,
             $this->createControllerReflector(),
             'areaName',
             $options
@@ -285,36 +249,24 @@ public static function getNonMatchingRoutes(): \Generator
     }
 
     /**
-     * @param array<Operation|Parameter> $annotations
-     * @param array<string|bool>         $options
+     * @param array<string|bool> $options
      */
     #[DataProvider('getRoutesWithDisabledDefaultRoutes')]
     public function testRoutesWithDisabledDefaultRoutes(
         string $name,
         Route $route,
-        array $annotations,
+        \ReflectionMethod $reflectionMethod,
         array $options,
         int $expectedRoutesCount
     ): void {
         $routes = new RouteCollection();
         $routes->add($name, $route);
         $area = 'area';
 
-        $reflectionMethodStub = $this->createMock(\ReflectionMethod::class);
         $controllerReflectorStub = $this->createMock(ControllerReflector::class);
-        $controllerReflectorStub->method('getReflectionMethod')->willReturn($reflectionMethodStub);
-
-        $annotationReader = null;
-        if (interface_exists(Reader::class)) {
-            $annotationReader = $this->createMock(Reader::class);
-            $annotationReader
-                ->method('getMethodAnnotations')
-                ->willReturn($annotations)
-            ;
-        }
+        $controllerReflectorStub->method('getReflectionMethod')->willReturn($reflectionMethod);
 
         $routeBuilder = new FilteredRouteCollectionBuilder(
-            $annotationReader,
             $controllerReflectorStub,
             $area,
             $options
@@ -326,24 +278,59 @@ public function testRoutesWithDisabledDefaultRoutes(
 
     public static function getRoutesWithDisabledDefaultRoutes(): \Generator
     {
+        $apiController = new class {
+            public function fooAction(): void
+            {
+            }
+        };
+
         yield 'non matching route without Annotation' => [
             'r10',
             new Route('/api/foo', ['_controller' => 'ApiController::fooAction']),
-            [],
+            new \ReflectionMethod($apiController, 'fooAction'),
+            ['disable_default_routes' => true],
+            0,
+        ];
+
+        $apiController = new class {
+            #[Areas(['area_something_very_different'])]
+            public function fooAction(): void
+            {
+            }
+        };
+
+        yield 'non matching route with different area Annotation' => [
+            'r10',
+            new Route('/api/foo', ['_controller' => 'ApiController::fooAction']),
+            new \ReflectionMethod($apiController, 'fooAction'),
             ['disable_default_routes' => true],
             0,
         ];
+
+        $apiController = new class {
+            #[Operation(['_context' => new Context()])]
+            public function fooAction(): void
+            {
+            }
+        };
         yield 'matching route with Nelmio Annotation' => [
             'r10',
             new Route('/api/foo', ['_controller' => 'ApiController::fooAction']),
-            [new Operation(['_context' => new Context()])],
+            new \ReflectionMethod($apiController, 'fooAction'),
             ['disable_default_routes' => true],
             1,
         ];
+
+        $apiController = new class {
+            #[Parameter]
+            public function fooAction(): void
+            {
+            }
+        };
         yield 'matching route with Swagger Annotation' => [
             'r10',
             new Route('/api/foo', ['_controller' => 'ApiController::fooAction']),
-            [new Parameter(['_context' => new Context()])],
+            new \ReflectionMethod($apiController, 'fooAction'),
             ['disable_default_routes' => true],
             1,
         ];