Closed mficzel closed 3 months ago
This one needs a little discussion:
appendExceedingArguments
cachePeriod
and cacheTags
FYI: Related discussion: https://discuss.neos.io/t/rfc-future-of-routing-mvc-in-flow/6535
@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.
@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:
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
@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
Let's talk, I have some ideas and I think that they can solve the remaining issues without a lot of work
Regarding Cache flushing i added this class to the annotationCacheFlusher not sure this actually works as i could not find prior uses.
@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
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
@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.
@bwaidelich agree and will add docs after cleaning up and fixing the tests. Ran into an issue with the partner policy pr #3324 that has to be solved first as we should release those together if possible.
@bwaidelich yes we agreed that package routes are always configured via mvc.routes
. The idea to include both routes.yaml and annotation routes via mvc.routes
was before that and is replaced by now. It totally makes sense to only include Package/Routes.yaml or annotation routes which is ensured by configuring a routes provider.
The code is imho fine but testing needs a little work now to catch up to those changes.
Flow/action is a bit misleading as we are configuring the route. A possible name would be Flow/ActionRoute
IMO Flow/Route
is fine.
You can always have a custom MyPackage/Route
Ill just write it down:
1.) ActionController
+ method annotation
This is the main usecase: We want to support annotations on an ActionController
(in my example below as Flow\Action
).
As "legacy" behaviour here, to satisfy ActionController::callActionMethod
, we require the action method to end with Action
and trim the suffix.
The expected route values are listed as comment:
class MyThingController extends ActionController
{
// @package My.Package
// @controller MyThing
// @action some
#[Flow\Action(path: 'foo')]
public function someAction()
{
}
}
2.) ControllerInterface
+ method annotation (proposed behaviour not implemented here right now)
If the Flow\Action
annotation is placed on a custom controller, i think it should behave a little less magic.
(Custom controllers are currently unattractive but via the dispatcher overhaul they will be simpler to use #3311)
In comparison when being placed onto an ActionController
we might not enforce the 'Action' suffix and pass the method name 1 by 1 to the @action
route value:
class MySimpleController implements ControllerInterface
{
// @package My.Package
// @controller MySimple
// @action myMethod
#[Flow\Action(path: 'foo')]
private function myMethod(ActionRequest $request, ActionResponse $response)
{
}
public function processRequest(ActionRequest $request, ActionResponse $response)
{
$this->{$request->getControllerActionName()}($request, $response);
}
}
3.) ControllerInterface
+ class annotation
And as part of a future scope, once @action
is truly optional: https://discuss.neos.io/t/rfc-future-of-routing-mvc-in-flow/6535/5
We can (and probably should) add a class based annotation like:
// @package My.Package
// @controller MyRest
#[Flow\Endpoint(path: 'foo')]
class MyRestController extends RestController
{
private function get()
{
}
private function post()
{
}
}
Flow\Endpoint
can only be placed once per class at its top and Flow\Action
must not be used in this case.
Other things:
In the future as proposed here https://discuss.neos.io/t/rfc-future-of-routing-mvc-in-flow/6535/5, we will have it easier as we can just generate @controller My\Package\MyCustomController
as FQN
Additionally i seem to outnumbered by bernhard and christian as they do not want to enforce action methods to be public, and i now agree ^^.
The namings Flow\Action
and Flow\Endpoint
are not super well founded, bernhard and me just though of them ;)
Flow\ActionRoute
and Flow\EndpointRoute
or whatever would also be cool, but i think i like to have those concepts separated and not mixed into one Flow\Route
... or maybe that is also fine?
Flow\Action
does not make sense to me.
I think we should just go for Flow\Route
and allow the attribute on methods that have the Action
suffix for now.
As a follow-up we could extend the behavior to allow...
__invoke(Request): Response
)@controller
to the FQN once that is supported by the routing framework)A Flow\Endpoint
is speculation at this point, it could as well come from a 3rd party – but in any case it does not conflict with Flow\Route
IMO
I think we should just go for
Flow\Route
and allow the attribute on methods that have theAction
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.
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
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()
{
}
}
So giving this until tomorrow morning, then anything else becomes a separate PR as this here will be merged.
@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 ;)
Can we get a grip on this one or what is still blocking that from being merged, @mhsdesign ?
I'll merge this as soon as tests are done, everything else can really happen in follow ups.
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?
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:
ConfigurationRoutesProvider
supports evaluating nested providersIMO that's just a matter of documentation and it's quite a low-level API anyways
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:
To use annotation routes packages have to register the
AttributeRoutesProviderFactory
inNeos.Flow.mvc.routes
with Controller classNames or patterns.Settings.yaml:
This pr also adds the general option to register
provider
andproviderOptions
in the SettingNeos.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
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions