FEATURE: Add Flow\Route Attribute/Annotation

[mficzel]

The Flow\Route attribute allows to define routes directly on the affected method. This allows to avoid dealing with Routes.yaml in projects in simple cases where is sometimes is annoying to look up the exact syntax for that.

Usage:

use Neos\Flow\Mvc\Controller\ActionController;
use Neos\Flow\Annotations as Flow;

class ExampleController extends ActionController
{
    #[Flow\Route(uriPattern:'my/path', httpMethods: ['GET'])]
    public function someAction(): void
    {
    }

    #[Flow\Route(uriPattern:'my/other/b-path', defaults: ['test' => 'b'])]
    #[Flow\Route(uriPattern:'my/other/c-path', defaults: ['test' => 'c'])]
    public function otherAction(string $test): void
    {
    }
}

To use annotation routes packages have to register the AttributeRoutesProviderFactory in Neos.Flow.mvc.routes with Controller classNames or patterns.

Settings.yaml:

Neos:
  Flow:
    mvc:
      routes:
        Vendor.Example.attributes:
          position: 'before Neos.Neos'
          providerFactory: \Neos\Flow\Mvc\Routing\AttributeRoutesProviderFactory
          providerOptions:
            classNames:
              - Vendor\Example\Controller\ExampleController

This pr also adds the general option to register provider and providerOptions in the Setting Neos.Flow.mvc.routes which was required obviously.

The package: WebSupply.RouteAnnotation by @sorenmalling implemented similar ideas earlier.

Resolves: #2059

Upgrade instructions

Review instructions

Alsow see: #3324 resolving #2060, both solutions ideally would work hand in hand

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[mficzel]

This one needs a little discussion:

1. Shall we expose appendExceedingArguments
2. Shall we expose cachePeriod and cacheTags
3. Is it problematic that defaults could overwrite @Package etc

[bwaidelich]

FYI: Related discussion: https://discuss.neos.io/t/rfc-future-of-routing-mvc-in-flow/6535

[mficzel]

@mhsdesign @bwaidelich i am a bit frustraded how this turns out. While i like the suggestions on discuss the approach "we have to rewrite routing first" basically excludes this from beeing in Neos 9 (this close to a release we should not introduce any big changes we do not strictly need) which means we are stuck with Routes.yaml and in extension Policy.yaml for 9.0

I also see no easy way to only include part 1 of the discuss post (configure route provideres in seettings mvc.routes) into the RouteConfiguration that was just refactored in #2970. If you see a clear way to include this i might work on this but i do'nt short of refactoring again which i have no motivation to atm.

Regarding binding the current implementation to Action i tend to not see this as a real problem as this is how routing works now and it makes sense to implement exactly that now. Once routing gets more smart we can adjust that but even then we will surely want to support ActionControllers even if we allow other alternatives. So i see no way the Route Annotation would become wrong in future, only the way the attributes are handled would have to change together with routing.

[bwaidelich]

@mficzel I'm sorry that you're frustrated. But I think this is due to a misunderstanding:
I'm in no way trying to block or slow down this PR – I just linked the writeup of the things we discussed yesterday because they are related.

IMO there is no reason not to proceed with this one, we can always refactor it to use a different integration mechanism lateron.

There's only two issues that we should tackle/decide upon IMO:

• from a DX perspective, I would love to have the routing cache issue fixed (i.e. that routing caches are flushed when a route attribute is changed – or is that the case already?)
• yesterday we were in favor of keeping the explicit routing activation & ordering via Settings, and this change would circumvent this in the current form AFAIS

I also see no easy way to only include part 1 of the discuss post (configure route provideres in seettings mvc.routes) into the RouteConfiguration

I think it would actually be pretty straightforward to skip the CombinedRoutesProvider and instead support a setting of

provider: '<providerClassName>'

in our default ConfigurationRoutesProvider

[mficzel]

@bwaidelich the handling of the whole mvc.routes setting currently happens before the combined settings are passed to the ConfigurationRoutesProvider ... that is why there is currently no way to get in between at a specific point.

Also specifying a provider via setting is probably not that trivial as one will have to pass at leas the package key and for that either allow options or pass the key from the configuration in any case. Imho it would make sense to just include searching for route attributes into what happens when you add a package key to mvc.routes

[bwaidelich]

Let's talk, I have some ideas and I think that they can solve the remaining issues without a lot of work

[mficzel]

Regarding Cache flushing i added this class to the annotationCacheFlusher not sure this actually works as i could not find prior uses.

[bwaidelich]

@mficzel A little bit hacky, but with the following change:

diff --git a/Neos.Flow/Classes/Configuration/Loader/RoutesLoader.php b/Neos.Flow/Classes/Configuration/Loader/RoutesLoader.php
index a02a5c152..8b2229c31 100644
--- a/Neos.Flow/Classes/Configuration/Loader/RoutesLoader.php
+++ b/Neos.Flow/Classes/Configuration/Loader/RoutesLoader.php
@@ -96,6 +96,13 @@ class RoutesLoader implements LoaderInterface
             if (isset($routeFromSettings['suffix'])) {
                 $subRoutesConfiguration['suffix'] = $routeFromSettings['suffix'];
             }
+            if (isset($routeFromSettings['provider'])) {
+                $routeDefinitions[] = [
+                    'name' => $packageKey,
+                    'provider' => $routeFromSettings['provider'],
+                ];
+                continue;
+            }
             $routeDefinitions[] = [
                 'name' => $packageKey,
                 'uriPattern' => '<' . $subRoutesName . '>',
@@ -128,6 +135,10 @@ class RoutesLoader implements LoaderInterface
             }
             $mergedSubRoutesConfiguration = [$routeConfiguration];
             foreach ($routeConfiguration['subRoutes'] as $subRouteKey => $subRouteOptions) {
+                if (isset($subRouteOptions['provider'])) {
+                    $mergedRoutesConfiguration[] = $subRouteOptions;
+                    continue;
+                }
                 if (!isset($subRouteOptions['package'])) {
                     throw new ParseErrorException(sprintf('Missing package configuration for SubRoute in Route "%s".', ($routeConfiguration['name'] ?? 'unnamed Route')), 1318414040);
                 }
diff --git a/Neos.Flow/Classes/Mvc/Routing/ConfigurationRoutesProvider.php b/Neos.Flow/Classes/Mvc/Routing/ConfigurationRoutesProvider.php
index f876008e8..a4567c559 100644
--- a/Neos.Flow/Classes/Mvc/Routing/ConfigurationRoutesProvider.php
+++ b/Neos.Flow/Classes/Mvc/Routing/ConfigurationRoutesProvider.php
@@ -6,6 +6,8 @@ namespace Neos\Flow\Mvc\Routing;
 
 use Neos\Flow\Annotations as Flow;
 use Neos\Flow\Configuration\ConfigurationManager;
+use Neos\Flow\Configuration\Loader\RoutesLoader;
+use Neos\Flow\ObjectManagement\ObjectManagerInterface;
 
 /**
  * @Flow\Scope("singleton")
@@ -15,13 +17,26 @@ final class ConfigurationRoutesProvider implements RoutesProviderInterface
     private ConfigurationManager $configurationManager;
 
     public function __construct(
-        ConfigurationManager $configurationManager
+        ConfigurationManager $configurationManager,
+        private ObjectManagerInterface $objectManager,
     ) {
         $this->configurationManager = $configurationManager;
     }
 
     public function getRoutes(): Routes
     {
-        return Routes::fromConfiguration($this->configurationManager->getConfiguration(ConfigurationManager::CONFIGURATION_TYPE_ROUTES));
+        $routes = [];
+        foreach ($this->configurationManager->getConfiguration(ConfigurationManager::CONFIGURATION_TYPE_ROUTES) as $routeConfiguration) {
+            if (isset($routeConfiguration['provider'])) {
+                $provider = $this->objectManager->get($routeConfiguration['provider']);
+                assert($provider instanceof RoutesProviderInterface);
+                foreach ($provider->getRoutes() as $route) {
+                    $routes[] = $route;
+                }
+                continue;
+            }
+            $routes[] = Route::fromConfiguration($routeConfiguration);
+        }
+        return Routes::create(...$routes);
     }
 }

I can register custom providers via

Neos:
  Flow:
    mvc:
      routes:
        'Foo':
          provider: 'Some\Package\SomeRoutesProvider'
          position: 'start'

we could also add providerOptions

[bwaidelich]

Re your discussion points from 2 weeks ago:

Shall we expose appendExceedingArguments

I would say no because they are too easily confused with query parameters. We could always add it lateron

Shall we expose cachePeriod and cacheTags

Personally I don't really care, but IMO there is no reason not to (but it could always be added lateron)

Is it problematic that defaults could overwrite @Package etc

That would be solved with an explicit activation like suggested above

ps: if you force push changes it makes it really hard to follow the progress

[mficzel]

@bwaidelich it works now but is ugly as hell for now ... moved most of the logic of routes configuration loader into the ConfigurationRoutesProvider and integrated the annotation handling there. Have to figure out how to seperate and test this later.

Also i think that adding "pathPrefix" and "additionalRouteProviders" to the Setting mvc.routes can and should be be done separately and that flow route annotations should be used for all packages that are configured in mvc.routes without configuring an extra route provider.

[mhsdesign]

I think we should just go for Flow\Route and allow the attribute on methods that have the Action suffix for now.

Jain if we make it purely specific for the ActionController i agree. All other / custom controllers would otherwise not be supported at first.

for custom controllers i wouldnt want any suffix magic and have in getControllerActionName the whole method name without trimming.

[bwaidelich]

All other / custom controllers would otherwise not be supported at first.

at first. I don't think it's fair to block a new feature because it does not include another feature

for custom controllers i wouldnt want any suffix magic and have in getControllerActionName the whole method name without trimming.

I agree – we can just add a check for the controller base class and omit the magic if it's not a Flow ActionController. But IMO this can be done in a separate PR

[mhsdesign]

I don't think it's fair to block a new feature because it does not include another feature

Agreed.

I agree – we can just add a check for the controller base class and omit the magic if it's not a Flow ActionController.

okay so exactly as described above in point 2?

2.) ControllerInterface + method annotation (proposed behaviour not implemented here right now)

but regarding:

But IMO this can be done in a separate PR

i dont see a reason for that, a pr should IMO be self contained and complete. But dont worry i will prepare the necessary adjustments as it was my idea. Dont want to chase anyone around ^^

---

Regarding Flow\Endpoint, i was actually referring to attributes on classes and i wanted to discuss to have two separate annotations instead of one almighty.

But it seems we want to go the route (hatahahah) of allowing the annotation to be placed at some point on methods and classes?

class MyThingController extends ActionController
{
    // @package My.Package
    // @controller MyThing
    // @action some
    #[Flow\Route(path: 'foo')]
    public function someAction()
    {
    }
}

and

// @package My.Package
// @controller MyRest
#[Flow\Route(path: 'foo')]
class MyRestController extends RestController
{
    private function get()
    {
    }

    private function post()
    {
    }
}

[kitsunet]

Thanks a lot, this was a struggle, I actually would rather hit merge now than later, I think @mhsdesign concerns can and should all be tackled in a different PR, it's important to remmeber that just because we decided on a redirection it's not reality yet and the controller chnages are not fully in not even speaking about documenting them. So IMHO totally fine if this doesn't work correctly for other controllers that are not fully there yet.

[kitsunet]

So giving this until tomorrow morning, then anything else becomes a separate PR as this here will be merged.

[mhsdesign]

While i adjust the above ill also take care of these things ... lets see if i pass your constraints of tomorrow, but i have a lot going on at the moment

[mhsdesign]

ill try to add a functional test where once can see the feature 1 to 1 in action. Currently the unit tests are cool to have but really obscure :D

[mhsdesign]

@kitsunet and me agreed to just allow this feature at first on the ActionController so we can merge fast and keep the extensive followup discussion separate ;)

That being said, there will be a followup, and we are still developing Flow 9 so this must not necessary be the end of it - but even if it were and we only find a sane solution in Flow 9.1 it should be acceptable as this feature is itself a milestone.

I will try to provide the constraints and the adjustments above today unless someone else does it ;)

[bwaidelich]

Can we get a grip on this one or what is still blocking that from being merged, @mhsdesign ?

[kitsunet]

I'll merge this as soon as tests are done, everything else can really happen in follow ups.

Can be done in follow up.

[mhsdesign]

I wish you were so eager with other prs ;)
I just managed to add a functional tests and improved documentation a little. Which ill rebase now on 9.0 and make a followup.

Two questions:

1. Also i stumbled over the thing that the AttributeRoutesProvider's are NOT cached and always computed at runtime (unless the router has the route cached). Is this anticipated?

2. Its special that the AttributeRoutesProvider and ConfigurationRoutesProvider share one interface as registering the ConfigurationRoutesProvider via the settings would cause infinite recursion.

I would say we need one CentralRoutesProviderInterface implementation and possible several AttributeRoutesProvider implementations.

That means: ConfigurationRoutesProvider will be renamed to DefaultCentralRoutesProvider or sth. we add the CentralRoutesProviderInterface and the rest can stay as is.

The naming is of course discussable but i think it makes more sense. Also by that we can clearly document the one interface as api and describe how it can be used.

Does this direction make sense to you?

[bwaidelich]

I wish you were so eager with other prs ;)

Likewise :)
My concern was not to get this merged asap but to avoid this from absorbing more resources from other pressing topics that affect existing core logic.
Besides we should IMHO only block PRs from others if we have sincere concerns or want to discuss matters that couldn't easily be done in follow-ups.

Also i stumbled over the thing that the AttributeRoutesProvider's are NOT cached [...]
unless the router has the route cached [...]

But that should almost always be the case, right? I hope that the route is only evaluated if it hasn't been hit before and I'd say that's totally fine

Its special that the AttributeRoutesProvider and ConfigurationRoutesProvider share one interface

Yes, it's special and a bit weird. But I don't think that it's a problem we have to solve:

• You can globally replace the routes provider if you want to
• The provided ConfigurationRoutesProvider supports evaluating nested providers

IMO that's just a matter of documentation and it's quite a low-level API anyways

[bwaidelich]

FYI: Unfortunately this does not seem to work in production context, see #3400

[kitsunet]

#3401 is the bugfix just to have it refrenced here as well