From ee8a8127e1d77908b5f44786a1ba4d6f2500c583 Mon Sep 17 00:00:00 2001
From: mhsdesign <85400359+mhsdesign@users.noreply.github.com>
Date: Wed, 27 Mar 2024 11:52:46 +0100
Subject: [PATCH] TASK: Restrict `#[Flow\Route]` to `ActionController`'s

see https://github.com/neos/flow-development-collection/pull/3325#issuecomment-2022198044
see https://github.com/neos/flow-development-collection/issues/3335
---
 .../Mvc/Routing/AttributeRoutesProvider.php   | 50 ++++++++++++-------
 .../TheDefinitiveGuide/PartIII/Routing.rst    |  5 +-
 .../Routing/AttributeRoutesProviderTest.php   | 21 +++++---
 3 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/Neos.Flow/Classes/Mvc/Routing/AttributeRoutesProvider.php b/Neos.Flow/Classes/Mvc/Routing/AttributeRoutesProvider.php
index 0086f09c9f..df77ab0840 100644
--- a/Neos.Flow/Classes/Mvc/Routing/AttributeRoutesProvider.php
+++ b/Neos.Flow/Classes/Mvc/Routing/AttributeRoutesProvider.php
@@ -14,6 +14,7 @@
 
 namespace Neos\Flow\Mvc\Routing;
 
+use Neos\Flow\Mvc\Controller\ActionController;
 use Neos\Flow\Mvc\Exception\InvalidActionNameException;
 use Neos\Flow\Mvc\Routing\Exception\InvalidControllerException;
 use Neos\Flow\ObjectManagement\ObjectManagerInterface;
@@ -24,33 +25,43 @@
 /**
  * Allows to annotate controller methods with route configurations
  *
- * Implementation:
+ * Internal implementation:
+ * -----------------------
  *
  * Flows routing configuration is declared via \@package, \@subpackage, \@controller and \@action
  * The first three options will resolve to a fully qualified class name {@see \Neos\Flow\Mvc\ActionRequest::getControllerObjectName()}
  * which is instantiated in the dispatcher {@see \Neos\Flow\Mvc\Dispatcher::dispatch()}
  *
- * The latter \@action option will be treated internally by each controller.
- * By convention and implementation of the default ActionController inside processRequest
- * {@see \Neos\Flow\Mvc\Controller\ActionController::callActionMethod()} will be used to concatenate the "Action" suffix
- * to the action name and invoke it internally with prepared method arguments.
- * The \@action is just another routing value while the dispatcher does not really know about "actions" from the "outside".
+ * The latter \@action option will be treated internally by each controller. From the perspective of the dispatcher \@action is just another routing value.
+ * By convention during processRequest in the default ActionController {@see \ActionController::resolveActionMethodName()} will be used
+ * to concatenate the "Action" suffix to the action name
+ * and {@see ActionController::callActionMethod()} will invoke the method internally with prepared method arguments.
  *
- * Creating routes by annotation must make a few assumptions to work.
- * As not every FQ class name is representable via the routing configuration (e.g. the class has to end with "Controller"),
+ * Creating routes by annotation must make a few assumptions to work:
+ *
+ * 1. As not every FQ class name is representable via the routing configuration (e.g. the class has to end with "Controller"),
  * only classes can be annotated that reside in a correct location and have the correct suffix.
  * Otherwise, an exception will be thrown as the class is not discoverable by the dispatcher.
  *
- * The routing annotation is placed at methods.
- * It is validated that the annotated method ends with "Action" and a routing value with the suffix trimmed will be generated.
- * Using the annotations on any controller makes the assumption that the controller will delegate the request to the dedicate
- * action by depending "Action".
- * This thesis is true for the ActionController.
+ * 2. As the ActionController requires a little magic and is the main use case we currently only support this controller.
+ * For that reason it is validated that the annotation is inside an ActionController and the method ends with "Action".
+ * The routing value with the suffix trimmed will be generated:
+ *
+ *     class MyThingController extends ActionController
+ *     {
+ *         #[Flow\Route(path: 'foo')]
+ *         public function someAction()
+ *         {
+ *         }
+ *     }
+ *
+ * The example will genrate the configuration:
+ *
+ *     \@package My.Package
+ *     \@controller MyThing
+ *     \@action some
  *
- * As discussed in https://discuss.neos.io/t/rfc-future-of-routing-mvc-in-flow/6535 we want to refactor the routing values
- * to include the fully qualified controller name, so it can be easier generated without strong restrictions.
- * Additionally, the action mapping should include its full name and be guaranteed to called.
- * Either by invoking the action in the dispatcher or by documenting this feature as part of a implementation of a ControllerInterface
+ * TODO for a future scope of `Flow\Action` see {@link https://github.com/neos/flow-development-collection/issues/3335}
  */
 final class AttributeRoutesProvider implements RoutesProviderInterface
 {
@@ -80,6 +91,11 @@ public function getRoutes(): Routes
             if (!$includeClassName) {
                 continue;
             }
+
+            if (!in_array(ActionController::class, class_parents($className), true)) {
+                throw new InvalidControllerException('TODO: Currently #[Flow\Route] is only supported for ActionController. See https://github.com/neos/flow-development-collection/issues/3335.');
+            }
+
             $controllerObjectName = $this->objectManager->getCaseSensitiveObjectName($className);
             $controllerPackageKey = $this->objectManager->getPackageKeyByObjectName($controllerObjectName);
             $controllerPackageNamespace = str_replace('.', '\\', $controllerPackageKey);
diff --git a/Neos.Flow/Documentation/TheDefinitiveGuide/PartIII/Routing.rst b/Neos.Flow/Documentation/TheDefinitiveGuide/PartIII/Routing.rst
index f03a44b18b..2c8c708b45 100644
--- a/Neos.Flow/Documentation/TheDefinitiveGuide/PartIII/Routing.rst
+++ b/Neos.Flow/Documentation/TheDefinitiveGuide/PartIII/Routing.rst
@@ -753,6 +753,7 @@ Subroutes from Annotations
 --------------------------
 
 The ``Flow\Route`` attribute allows to define routes directly on the affected method.
+(Currently only ActionController are supported https://github.com/neos/flow-development-collection/issues/3335)
 
 .. code-block:: php
 
@@ -774,7 +775,7 @@ The ``Flow\Route`` attribute allows to define routes directly on the affected me
   }
 
 To find the annotation and tp specify the order of routes this has to be used together with the
-`\Neos\Flow\Mvc\Routing\AttributeRoutesProviderFactory` as `providerFactory` in  Setting `Neos.Flow.mvs.routes`
+`\Neos\Flow\Mvc\Routing\AttributeRoutesProviderFactory` as `providerFactory` in  Setting `Neos.Flow.mvc.routes`
 
 .. code-block:: yaml
 
@@ -787,7 +788,7 @@ To find the annotation and tp specify the order of routes this has to be used to
             providerFactory: \Neos\Flow\Mvc\Routing\AttributeRoutesProviderFactory
             providerOptions:
               classNames:
-                - Vendor\Example\Controller\ExampleController
+                - Vendor\Example\Controller\*
 
 Route Loading Order and the Flow Application Context
 ====================================================
diff --git a/Neos.Flow/Tests/Unit/Mvc/Routing/AttributeRoutesProviderTest.php b/Neos.Flow/Tests/Unit/Mvc/Routing/AttributeRoutesProviderTest.php
index 0a63ca722b..4ee9cd3459 100644
--- a/Neos.Flow/Tests/Unit/Mvc/Routing/AttributeRoutesProviderTest.php
+++ b/Neos.Flow/Tests/Unit/Mvc/Routing/AttributeRoutesProviderTest.php
@@ -38,7 +38,7 @@ public function setUp(): void
         $this->annotationRoutesProvider = new Routing\AttributeRoutesProvider(
             $this->mockReflectionService,
             $this->mockObjectManager,
-            ['Vendor\Example\Controller\ExampleController']
+            ['Vendor\\Example\\Controller\\*']
         );
     }
 
@@ -61,19 +61,26 @@ public function noAnnotationsYieldEmptyRoutes(): void
      */
     public function routesFromAnnotationAreCreatedWhenClassNamesMatch(): void
     {
+        $exampleFqnControllerName = 'Vendor\\Example\\Controller\\ExampleController';
+        eval('
+        namespace Vendor\Example\Controller;
+        class ExampleController extends \Neos\Flow\Mvc\Controller\ActionController {
+        }'
+        );
+
         $this->mockReflectionService->expects($this->once())
             ->method('getClassesContainingMethodsAnnotatedWith')
             ->with(Flow\Route::class)
-            ->willReturn(['Vendor\Example\Controller\ExampleController']);
+            ->willReturn([$exampleFqnControllerName]);
 
         $this->mockReflectionService->expects($this->once())
             ->method('getMethodsAnnotatedWith')
-            ->with('Vendor\Example\Controller\ExampleController', Flow\Route::class)
+            ->with($exampleFqnControllerName, Flow\Route::class)
             ->willReturn(['specialAction']);
 
         $this->mockReflectionService->expects($this->once())
             ->method('getMethodAnnotations')
-            ->with('Vendor\Example\Controller\ExampleController', 'specialAction', Flow\Route::class)
+            ->with($exampleFqnControllerName, 'specialAction', Flow\Route::class)
             ->willReturn([
                 new Flow\Route(uriPattern: 'my/path'),
                 new Flow\Route(
@@ -86,12 +93,12 @@ public function routesFromAnnotationAreCreatedWhenClassNamesMatch(): void
 
         $this->mockObjectManager->expects($this->once())
             ->method('getCaseSensitiveObjectName')
-            ->with('Vendor\Example\Controller\ExampleController')
-            ->willReturn('Vendor\Example\Controller\ExampleController');
+            ->with($exampleFqnControllerName)
+            ->willReturn($exampleFqnControllerName);
 
         $this->mockObjectManager->expects($this->once())
             ->method('getPackageKeyByObjectName')
-            ->with('Vendor\Example\Controller\ExampleController')
+            ->with($exampleFqnControllerName)
             ->willReturn('Vendor.Example');
 
         $expectedRoute1 = new Route();