Move executing action out of the router

[samdark]

That was proposed by @roxblnfk before in the dev chat. Not sure I've got it exactly but I think this variant is interesting.

What we have now

1. Router executes handlers: https://github.com/yiisoft/router/blob/master/src/Middleware/Router.php#L43.
2. How it's done can not be customized: https://github.com/yiisoft/router/blob/master/src/Route.php#L373. We have injector there injecting services and that's it.
3. We can't use Yii-specific functionality there since router is not Yii-specific package.
4. There is a potential attribute collision issue between request and infrastructural things such as CSRF middleware. Both are using request attributes. https://github.com/yiisoft/router/blob/master/src/Middleware/Router.php#L40

Proposal

1. Split getting a matching a route and executing it.
2. Leave getting a route matched in this package, matched route will be set to an attribute of the request.
3. Move route execution into yii-web.

It fixes the following issues:

1. Handler execution is customizable.
2. We can use any Yii-specific things when executing a handler. Here's a good example of what could be done way simpler with such approach: https://github.com/romkatsu/yii-demo/pull/4
3. Attribute collision is easier to avoid (but that's separate thing).

[samdark]

Examples of 2 (not necessary a good ideas but possible after the change) are:

• A custom Request for handler that has extra functionality.
• Ability to return string or [array] from a handler.

[samdark]

Agreed that @roxblnfk would work on proof on concept.

[armpogart]

Ability to return string or [array] from a handler.

Such things are already possible to implement directly in this package and this can be helpful in many cases, see my PR.

If I understand it correctly you plan to get rid of handling route directly in this package. In that case I see no any practical usage of this package as a separate package for the community as it doesn't add any considerable value instead of using FastRoute directly for example. I haven't found any php router that is not allowing execution/handling in it. I'm not sure that Yiisoft\Router name will be appropriate for this package after the change as it will become just RouteMatcher.

What about considering ability to add custom handler to the routes (or group) specified instead of removing route execution/handling functionality altogether. I'm sure adding an ability to define custom handler for routes is very viable and good alternative. It will also add more value to the package and default functionality will remain in here. Any custom handler specified (if specified) will just replace default execution/handling process and the package will just define some interface for custom handlers.

Or maybe I don't fully understand the problem?

[roxblnfk]

What about considering ability to add custom handler to the routes (or group) specified instead of removing route execution/handling functionality altogether. I'm sure adding an ability to define custom handler for routes is very viable and good alternative. It will also add more value to the package and default functionality will remain in here. Any custom handler specified (if specified) will just replace default execution/handling process and the package will just define some interface for custom handlers.

This is roughly what I would like to do

[armpogart]

Split getting a matching a route and executing it. Leave getting a route matched in this package, matched route will be set to an attribute of the request. Move route execution into yii-web.

@roxblnfk As far as I understood from the proposal in the issue description you were planning to remove execution altogether from this package. Isn't it the case?

I've looked through other packages more and I see that there are some routers which do only route configuration, url matching, and url generation part only without any handling (laminas-router former zend, symfony routing component) and I also see value in that (considering the usage of package outside of Yii context) when you do execution/handling elsewhere.

The solution I see is working through the issues #37 and #56. Both are about separating logic in the router for each specific task to it's own class with interface. The implementation of both issues will give first the ability to use UrlMatcher as standalone matching router without any handling/execution in it when necessary. And all execution/handling logic will be encapsulated in standalone Router which when used would be able to handle the routes and would be more expandable. Separation of concepts will give obvious benefits, so we will have:

• UrlMatcher which will do the matching only (FastRoute under the hood) and will be easily expandable in the future.
• Route plain definition of the route without any dependencies, but still with definition of it's middlewares, etc and maybe added method withHandler to define custom handler to use later. Removing all execution logic will allow also routes to be easily cacheable which will help applications with many routes.
• Router which will use UrlMatcher to find match the route and do the exucution/handling or delegate the handling to custom handler if one is defined for the specific route/group.

The only problem is it would be almost total rewrite/refactoring of the package.

[roxblnfk]

@armpogart I largely support the reasoning. However, I have not yet dealt with this task closely and am not ready to provide a detailed plan. If you have a desire to translate the above into code, we would be very grateful for such a contribution. For the next week I will definitely not be doing this task.

[armpogart]

@roxblnfk I will be happy to help with this one, but it will be huge PR and BC break, I have already started working on it.

I will try to push frequently and would ask for reviews to make sure that I am going in the right direction (will also push on Slack for some consulting as I'm not sure why some things are done the way they are done now).

I will update this issue with plan for now to receive feedback (if any):

• Make Route class just a simple definition of the route without any logic in it, it will contain just the definition and support serialization/compiling with any of the underlying drivers (planning to add them, see below) with default one built-in. Any Route object will support defining middlewares and custom handler/handlers which can be used later on.
• Add serialization/compiler drivers which will be used to translate route definition to a number of formats (such as string, json, xml, yaml) and support for custom driver conforming to interface. This will be helpful in 2 cases: first to compile the routes in production for applications with many routes to improve the performance, and second to support route configuration in underlying formats also (not only in php).
• Combine RouteCollection and Group classes (not sure why they are 2 separate classes and 2 seperate interfaces RouteCollectionInterface and RouteCollectorInterface). The combined class will also be just a collection of Route objects grouped and will not contain any handling/execution logic in it. The collection will share some functionality with the Route (such as some definitions that useful to have on both, e.g. defining methods, defining handlers and middlewares..) and also the collection will again support serializer/compiler functionality.
• UrlMatcherInterface and UrlGeneratorInterface will remain more or less the same, except I want to add a possibility to use UrlMatcher also against Route (not only collection), which can be helpful when you already have Route object and want to check whether it matches current request.
• MatchingResult will remain also more or less the same, except I will get rid of execution logic in it.
• RouterInterface and new Router class will be defined which will combine all the logic of handling/execution of routes/collections and will conform to UrlMatcherInterface and UrlGeneratorInterface by delegation. This will be the only class in the package that will be dependent on ContainerInterface. Router will also conform to PSR-15 RequestHandlerInterface to be more usable as a standalone package by the community.
• Current Router will be renamed to RouterMiddleware to be usable as a PSR-15 MiddlewareInterface and will use Router under the hood.

I'm sure some things will change during the work, but the grand plan as far as I see it now is this. I think the rewrite will make the router more flexible for many use cases and will still remain usable in Yii (even more than now). Any feedback will be appreciated.

[roxblnfk]

I'll try to roughly describe what I was going to do:

• Make the router stateless. The only router state is fastroute with a route tree. Everything else will be passed through the entity of the result of its work (MatchingResult).
• The router will not execute any middlewares or actions. It will return a generic pipeline definition as a result of route comparison.
• Move all the pipeline definition execution logic into a separate package. It can be changed and adjusted for any strategy. Accordingly, the Router middleware will be renamed to RouterMiddleware and will use some kind of pipeline definition executor;
• The pipeline definition interfaces will also be in a separate package, since this is reusable code for an application pipeline.
• UrlMatcher is only part of the router API (without state like last matched route)
• MatchingResult is what will be returned as a result of the router. It will contain the last matched route and, most likely, its parameters.

[yiiliveext]

It would be better to create a separate package so in this way we can use and compare both packages.

[roxblnfk]

Or separate branch (for a start)

[armpogart]

@roxblnfk

• Make the router stateless. The only router state is fastroute with a route tree. Everything else will be passed through the entity of the result of its work (MatchingResult).
• The router will not execute any middlewares or actions. It will return a generic pipeline definition as a result of route comparison.
• Move all the pipeline definition execution logic into a separate package. It can be changed and adjusted for any strategy. Accordingly, the Router middleware will be renamed to RouterMiddleware and will use some kind of pipeline definition executor;

I am going to do more or less the same with some exceptions. UrlMatcherInterface and UrlGeneratorInterface will be defined here and the implementations will be in underlying driver (router-fastroute for now). MatchingResult can easily become interface as without execution logic (that is currently in it), it will be better to define it in underlying driver as it might have some implementation differences based on support of functionality in underlying driver. The only part containing any state would be Router class which will provide default execution logic to make this package usable as a standalone, but will be still very customizable as it will conform to RouterInterface with the ability to override it or only override handling logic through custom RequestHandlerInterface (also all the routes can have custom handlers defined on their own, which can be used by the Router for the matched Route). Router will not work with routes directly but delegate url matching, url generation and use MatchingResult to get underlying Route from it.

• The pipeline definition interfaces will also be in a separate package, since this is reusable code for an application pipeline.

I'm not sure I understand it correctly what you mean exactly under pipeline definition. We can just define simple RouteInterface which will already contain the definition you need defining getters that will be implemented in Route class. I'm not convinced that we need a separate package only for that. Anyways if all the interfaces will be strictly defined and implementation will be coded against interfaces, it would be easy to separate anything later.

• UrlMatcher is only part of the router API (without state like last matched route)

Agree. UrlMatcher will do only matching against the route or collection of routes and return MatchingResult. Last matched route can be part of RouterInterface if it is needed at all.

• MatchingResult is what will be returned as a result of the router. It will contain the last matched route and, most likely, its parameters.

It will be returned from UrlMatcher and Router (RouterInterface) can work with it and its' underlying Route with it's definition.

It would be better to create a separate package so in this way we can use and compare both packages. Or separate branch (for a start)

I will work on my separate fork, or you can create separate branch under yiisoft/router, yiisoft/router-fastroute and give me push access only to it (not sure if it's possible on github).

[armpogart]

@roxblnfk @yiiliveext You can check draft PR #60. Feedback is welcome (taking into account that it is still work in progress). I just want to be sure that the work is going in the right direction.

[samdark]

This is practically done in https://github.com/yiisoft/router/pull/63