[Routing]RouteSubscriberInterface #37786

ahundiak · 2020-08-09T18:01:06Z

Like the event subscriber, a route subscriber would be able to define their own routes. No annotations needed and developers would have the power of the route configurator.

interface RouteSubscriberInterface
{
    public static function getRoutes(RoutingConfigurator $routes);
}
class HomeIndexAction implements RouteSubscriberInterface
{
    public static function getRoutes(RoutingConfigurator $routes)
    {
        $routes->add('home_index', "/")->controller(self::class);
    }

# config/routes.php
return function (RoutingConfigurator $routes) {

    HomeIndexAction::getRoutes($routes);

A file loader like the annotation loader could provide some automation. Maybe a compiler pass as well.

The text was updated successfully, but these errors were encountered:

derrabus · 2020-08-10T06:19:49Z

That sounds like you‘re suggesting yet another way to load routes. Can you elaborate why the methods described in https://symfony.com/doc/current/routing/custom_route_loader.html don‘t work for you?

ahundiak · 2020-08-10T12:02:21Z

To the best of my knowledge, annotations are the only approach for letting controllers define their own routes. Annotations are of course not code and don't always provide useful feedback when writing. Annotations appear to be very popular so the desire to define routes within the same file as the controller seems relevant. As opposed to using external routing files.

My approach relies on the RoutingConfigurator which is basically a route builder and avoids the need for dealing with array keys and such. Basically it replaces annotations with code while staying inside of the same file. It is of course what the framework itself uses for PHP based routing configuration.

Annotations are also a bit awkward to use outside of the framework. You need to load the doctrine annotation package which always seemed a bit strange to me.

I'm just proposing a standard component level interface that could be used to move code from config/routes.php to individual controller classes with a minimum of effort. And could then potentially be used to automate the process of finding the route subscriber classes within the framework when building the route collection.

I think this approach is also consistent with current efforts to move to more PHP based configuration.

Guikingone · 2020-08-10T13:26:13Z

I think this approach is also consistent with current efforts to move to more PHP based configuration.

Actually, you can define routes in a .php file: https://symfony.com/doc/current/routing.html#creating-routes-in-yaml-xml-or-php-files.

That's probably the closest that you can get to php based configuration IMO 🤔

jvasseur · 2020-08-10T13:33:44Z

Annotations are also a bit awkward to use outside of the framework. You need to load the doctrine annotation package which always seemed a bit strange to me.

This requirement will be removed once #37474 is merged.

ahundiak · 2020-08-10T13:52:04Z

Actually, you can define routes in a .php file:

Yep.

And all I am doing is proposing a standard component level way for using PHP to define routes from inside of a class such as a controller class.

A way that might enable a bit of automation at the framework configuration level.

Guikingone · 2020-08-10T13:58:47Z

And all I am doing is proposing a standard component level way for using PHP to define routes from inside of a class such as a controller class.

Actually, thanks to the Route attribute, we will be able to declare routes in PHP classes.

A way that might enable a bit of automation at the framework configuration level.

Automation is a good idea but IMO, the current "implementation" is not what Sf should target, the proposed implementation looks and feels like ServiceSubscriber and EventSubscriber which are related to specific use cases (or edge cases if I take the ServiceSubscriber path) and not used "every time" that you define a new class in Symfony, IMO, controllers/actions should stay "out" of some kind of magic.

BTW, maybe s 8000 omething similar to the Twig functions could be IMO a better approach:

<?php

namespace App\Controller;

class HomeIndexAction implements RouteSubscriberInterface
{
    public static function getRoutes(): array
    {
        return [
            'index' => new Route('/')->setController($this, ['index']), // This code is not valid, just a example
        ];
    }

    public function index(Request $request): Response
    {
        // ...
    }
}

javiereguiluz · 2020-08-10T14:01:45Z

@derrabus as a bundle author, not having a feature like this is something that I missed more than once. As explained in the article that you linked, bundles can't really add dynamic routes to the application. The reason is this: https://symfony.com/doc/current/routing/custom_route_loader.html#using-the-custom-loader

Even if your bundle defines a route loader, the user must always add some config manually to activate it. That's why you have to copy+paste some route config when using SonataAdmin, because it includes a loader that generates admin routes for your entities dynamically.

Note that:

I don't know the implications (in performance and security) of allowing such a feature. It's probably dangerous at multiple levels.
I haven't analyzed this specific proposal, that's why I'm not mentioning it.

fabpot · 2020-08-10T14:27:07Z

Also because you almost always want to mount the bundle routes under a prefix that you (the app) decide (the bundle should not do it).

ahundiak · 2020-08-10T14:53:59Z

This proposal is geared more towards the application level as opposed to the bundle level.

In my controller code I almost always add a prefix:

    public static function getRoutes(RoutingConfigurator $routes)
    {
        $routes = $routes->collection('blog_')->prefix('blog');
        $routes->add('show', '/show/{slug}')->controller(self::class . '::show');

It's one reason I like using the configurator to define routes. All the hard work of dealing with prefixes is already done for you.

I don't see any reason why we could not add an optional prefix argument to the interface:

# config\routing.php
return function (RoutingConfigurator $routes) {
    SomeThirdPartyUserController::getRoutes($routes,'user');

That would of course become messy if the bundle had many controllers. But just like autowire is discouraged for bundles, I see no inherent reason why autoroute (to invent a term) could not also be discouraged for bundles. The current bundle routing approach works fine and could still be the recommended approach.

ahundiak · 2020-08-10T15:29:16Z

Thought a bit more about bundles. I think this interface could be used in place of importing bundle routes.

# my-bundle\Resources\MyBundleRoutes.php
class MyBundleRoutes implements RouteSubscriberInterface {
    static getRoutes($routes,$prefix) {
        MyBundleController1::getRoutes($routes,$prefix);
        MyBundleController2::getRoutes($routes,$prefix);

# application/config/routes.php
return function (RoutingConfigurator $routes) {
    MyBundleRoutes::getRoutes($routes,'mounting prefix');

I think this would make importing bundle routes completely independent of any bundle specific configuration functionality. The entry point would just be a class. Would not even have to be part of a Symfony bundle.

ahundiak · 2020-08-10T17:34:29Z

One thing that I am perfectly confident of is that the decades old debate of using annotations vs code for configuration is not going to be solved in this thread. At the end of the day, whatever the community decides is fine by me.

I do think the recent shift from using yaml to PHP for routes and services show that there is not a great deal of resistance to using PHP. I also find it somewhat interesting that Symfony never developed an annotation based system for configuring services. Plenty of other systems have one.

I think it is important that anyone offering an opinion on this proposed interface be familiar with the RoutingConfigurator. This is not a question of directly instantiating route objects and dealing with all the arrays and array keys involved. The builder gives you quite a bit of code completion and error checking and whatnot. And yes there are plugins that can help with route annotations.

I also feel that developers using the route component standalone might appreciate a more or less standard method of defining routes inside of controllers. As mentioned before, using annotations outside of the framework can a bit of a pain.

Using code also opens up the possibility of adding helpers. One could, for example, imagine a CRUD route helper that would generate all the routes needed for a CRUD controller. And yes there are existing generators which emit seemingly endless streams of annotations. Still, it could be nice to have some code based helpers.

I'll just end with the proverbial BlogController example. Anyone who has managed to read this far can make their own comparisons to annotations.

    public static function getRoutes(RoutingConfigurator $routes)
    {
        $routes = $routes->collection('blog_')->prefix('blog');
        $routes->add('show', '/show/{slug}')->controller(self::class . '::show');
        $routes->add('edit','/edit/{slug}')->controller(self::class . '::edit');
        $routes->add('list', '/')->controller(self::class . '::list');
    }

ahundiak · 2020-08-10T18:46:05Z

More than open to name changes. Subscribers sort of matches some other components. A single static method that provides additional configuration information.

No conflict with route loaders or importers. I kind of think that having an unique name might actually avoid confusing it with a loader. Autoload is doing the hard work of finding and loading the file.

I think if you squint a bit you could consider adding a route to the router as a type of subscribing. You are telling the router to provide this information when the url is matched. Sort of like the event dispatcher does when it receives an event. Of course there a few more steps between the router and the action but with a bit of hand waving, maybe people won't notice.

Defining a helper trait as well is fine but one step at a time.

derrabus · 2020-08-11T08:51:49Z

@derrabus as a bundle author, not having a feature like this is something that I missed more than once.

I think that this is in fact a separate issue. If the feature that is being discussed here gets implemented, we should make sure, it does not create a way for bundles to automatically register routes.

Even if your bundle defines a route loader, the user must always add some config manually to activate it.

And we should keep it that way. A bundle should not sneak in routes. We're talking about the public API exposed by the application. The application author should be in control here.

I agree, that the activation of routes provided by a bundle could be standardized and simplified. But in the end, the application author should decide if and where the routes exposed by a bundle are mounted.

javiereguiluz · 2020-08-11T08:58:06Z

@derrabus yes, I understand there might be some security/performance concerns. However, if I'm right, a bundle can override, delete and create any services in your application without manual intervention, it can override any Twig filter and function, etc. So, "shady bundles" can already destroy your application if they want.

nicolas-grekas · 2020-08-11T09:58:37Z

Taking over routes of the app is another level of shadiness. Especially because the public URL layout doesn't belong to bundle authors but to devs of the app.
(Well, except for the /well-known/ URLs, but that's the exception)

ahundiak · 2020-08-11T12:58:02Z

Based on some of the previous comments, I just want to emphasize that this proposal does not make any changes whatsoever to the existing route configuration processes. All it does i 8000 s provide a standard way for a class to act as a routes.php configuration file. Exactly the same functionality currently available using annotations.

It's worth discussing how to use the interface to automate the discovery of routing file. Same as the annotation processor does. But the only thing that changes is replacing configurator import statements with static method calls.

Warning: If this interface is accepted then expect a ServiceSubscriberInterface proposal as well. Same basic idea. Allow a class to configure itself as a service using a static method and the service configurator.

ahundiak · 2020-08-11T20:17:10Z

There's no real automated discovery involved with annotation routes

I was definitely unclear. I was referring to the process of searching a directory (typically the Controller directory) and then using the AnnotationClassLoader to discover which classes have the route annotation and extracting the route information.

I see no inherent reason why a PhpClassLoader could not accomplish that same task using the RouteSubscriberInterface.

Tobion · 2020-08-15T01:24:07Z

I'm 👎 on this proposal. PHP attributes will be a core php feature which will eventually also be the replacement for route annotations.

So this invalides the argument

Annotations are also a bit awkward to use outside of the framework. You need to load the doctrine annotation package which always seemed a bit strange to me.

So to me there is no point in adding an alternative to annotations/attributes when you want to define routes near the controller code. And if you are not happy with attributes, you can implement your proposal in a custom route loader (even as a standalone bundle). No need to add this to core at this point IMO.

ahundiak · 2020-08-15T11:45:01Z

So to me there is no point in adding an alternative to annotations/attributes

I understand and respect your opinion. Certainly attributes should be cleaner than annotations.

Still the RoutingConfigurator is pretty powerful and easy to use. No need to learn another syntax. Seems a shame not to encourage folks to use it outside of config/routing.php.

And a potential shame not to be able to use the framework's tagging capability to identify routing files as opposed to searching by directory.

But again, if the consensus is thumbs down then it's time to close this issue and move on.

Tobion · 2020-08-15T11:59:29Z

And a potential shame not to be able to use the framework's tagging capability to identify routing files as opposed to searching by directory.

Container services can be tagged. But routing files are not services.

I'm gonna close the issue. Thanks for sparking this discussion.

…TheCat) This PR was merged into the 6.4 branch. Discussion ---------- [Routing][SecurityBundle] Add `LogoutRouteLoader` | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #50920 | License | MIT | Doc PR | symfony/symfony-docs#19000 #50920 is about avoiding for users to create logout routes. Given [we don’t want to allow bundles registering routes](#37786), I added a `LogoutRouteLoader` service bearing the `routing.route_loader` tag to be imported by the user. Such import could be added to the SecurityBundle recipe: ```yaml # config/routes/security.yaml logout: resource: security.route_loader.logout type: service ``` To invalidate routes when logout paths change, I stored them in a parameter so that the `ContainerParametersResourceChecker` can check the collection. Not sure if it’s okay or if a better way exists. Commits ------- 0a558d0 [SecurityBundle][Routing] Add `LogoutRouteLoader`

xabbuh added Feature Routing labels Aug 10, 2020

Tobion closed this as completed Aug 15, 2020

MatTheCat mentioned this issue Jul 11, 2023

[Security] Automatically create logout route if it does not exists #50920

Closed

MatTheCat mentioned this issue Sep 17, 2023

[Routing][SecurityBundle] Add LogoutRouteLoader #50946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Routing]RouteSubscriberInterface #37786

[Routing]RouteSubscriberInterface #37786

[Routing]RouteSubscriberInterface #37786

[Routing]RouteSubscriberInterface #37786

Comments