Use PHP attributes for route discovery

Created on 23 September 2022, over 1 year ago
Updated 30 January 2024, 5 months ago

Problem/Motivation

Drupal should use modern PHP attributes for route discovery ala Symfony.

This will allow us to add an attribute to a method name and use that for routing information. For example:

  /**
   * The default 401 content.
   *
   * @return array
   *   A render array containing the message to display for 401 pages.
   */
  #[Route(
    path: '/system/401',
    name: 'system.401',
    requirements: ['_access' => 'TRUE'],
    defaults: ['_title' => 'Unauthorized']
  )]
  public function on401() {
    return [
      '#markup' => $this->t('Please log in to access this page.'),
    ];
  }

will replace the following entry in system.routing.yml

system.401:
  path: '/system/401'
  defaults:
    _controller: '\Drupal\system\Controller\Http4xxController:on401'
    _title: 'Unauthorized'
  requirements:
    _access: 'TRUE'

What are the advantages of doing this?

  • The defaults._controller value is determined by the method you add the attribute to
  • The route definition and the controller code live side by side
  • No longer need route names - these can be automatically provided based on the class and method name
  • Less difference between modern Symfony and Drupal

What are the disadvantages?

  • The routing.yml file can give a good overview of what a module does and makes reviewing route access a bit simpler

Mitigations of the disadvantages:

  • Currently the code is scanning all the classes in a module's src/Controllers directory for attributed methods - therefore it's not that difficult to scan this code and you benefit from the controller and route definition being together.
  • We should and can build CLI tools to expose all the routes provided by a module. This would be good not just for routes determined by attributes but also dynamic routes

Proposed resolution

  • Remove yaml discovery from the RouteBuilder into it's own service
  • Add a new static routing event for the yaml discovery to listen to
  • Add a new service for PHP attribute discovery and listen to the same event. This reflects on all classes in a module's src/Controllers directory to find routes

Remaining tasks

  • Decide which of the Symfony route features to keep in - env, localized_paths, priority, _locale, _canonical_route stuff... not sure what to do with this.

User interface changes

None

API changes

TBD

Data model changes

None

Release notes snippet

@todo

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Routing 

Last updated 2 days ago

Created by

🇬🇧United Kingdom alexpott 🇪🇺🌍

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States smustgrave

    Think the MR got altered. Seeing 900+ changes for ckeditor, composer, etc. Is that expected?

  • 🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

    @smustgrave seems ok now. I saw something similar on a different issue. I think its a gitlab problem.

  • Status changed to Needs review over 1 year ago
  • 🇺🇸United States smustgrave

    Will add to my list.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    @catch / #15 I went with an event due to consistency with the existing routing building events.

  • 🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

    I can take a look at #13:

    a unit test of AttributeRouteDiscovery would be good.

  • 🇬🇧United Kingdom joachim

    I've opened an issue to decide on where to put annotation classes, so all annotation issues can work to a common standard: 📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed .

  • 🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

    A couple of extracted comments from @alexpott and @berdir in slack:

    alexpott
      3 days ago
    OTOH we could teach POTX to extract _title from Route attributes like it does with routing yml
    
    
    alexpott
      3 days ago
    I have no idea how _title ends up in TranslatableMarkup :smile:
    
    
    alexpott
      3 days ago
    Oh that happens here… \Drupal\Core\Controller\TitleResolver::getTitle
    
    berdir
      3 days ago
    you're too fast, was just about to paste that
    
    
    berdir
      3 days ago
    didn't know we make any raw variable available as a placeholder, can't imagine that this is often used and seems pretty scary
    
    
    alexpott
      3 days ago
    Yeah so I think we’ll need to open an issue to extend POTX to support discovery of certain attributes… which given what it already does shouldn’t be too bad.
    
  • 🇺🇸United States smustgrave

    Before reviewing can the remaining tasks be crossed out for what has been completed please.

  • 🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

    I've added more test coverage so crossed that out. Not sure if it's enough test coverage though.

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Seems to have some open threads.

  • Status changed to Needs review over 1 year ago
  • 🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

    Addressed feedback.

  • 🇫🇷France andypost

    Left a review and nitpicks, not sure core need to adopt sophisticated logic from SF with automatic route name generation

    Somehow it discovery should report about routes without name

  • 🇺🇸United States smustgrave

    @andypost think there are some questions in the MR for you? There are still some open threads.

  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    not sure core need to adopt sophisticated logic from SF with automatic route name generation

    Why not? Naming thing with IDs opens the possibility of namespace conflicts. Less hard coded IDs would be a good thing.

  • 🇫🇷France andypost

    I mean if route name will be generated from class-name or whatever it will be DX-- to use such names in URL generator and making links

  • 🇬🇧United Kingdom joachim

    > Why not? Naming thing with IDs opens the possibility of namespace conflicts. Less hard coded IDs would be a good thing.

    All it means is that instead of having a code ID that shouldn't change, you have a class that you can't rename.

    Routes need to be referred to by other routes, child menu items etc etc.

    I actually think the introduction of route IDs in D8 was a really good thing -- much easier to deal with route IDs rather than paths.

  • 🇳🇿New Zealand John Pitcairn

    As a custom module developer and site builder, I think I'd be fairly averse to auto generated route names based on the controller class.

    Would it include the namespace? I might have more than one DownloadController across a suite of modules for example.

    • I shouldn't need a cli tool to discover a route name.
    • I definitely don't want a route name to change if a controller class changes.
    • I do want to be able to define a route name explicitly and search for its definition in my IDE.
  • 🇷🇴Romania claudiu.cristea Arad 🇷🇴

    What about _title_callback?

  • 🇩🇪Germany donquixote

    As a custom module developer and site builder, I think I'd be fairly averse to auto generated route names based on the controller class.

    Personally I think I like automatic route names based on the class name.
    Obviously it should include the namespace.

    But if we want to keep custom ids, one pattern that can be useful in combination with attributes is class constants.

    class MyController {
      const ROUTE_ID = 'mymodule.myroute';
    
      #[Route(id: self::ROUTE_ID, ...)]
      public function page(..) {}
    }
    

    Then other code can target that constant to refer to the route.

    $url = Url::fromRoute(MyController::ROUTE_ID);
    

    Of course, this again makes that code dependent on the class holding the constant.
    To avoid that, the constant could live in a separate class..

    A big question is also whether we want to convert the other yml files to attributes, e.g. for local tasks, menu links etc.
    And as a consequence, will we refer to routes more often from yml files or from php code?
    In yml files we cannot use constants (afaik), we cannot use class imports, and string ids generally look nicer than class names in yml.
    In php code, I prefer class constants over string literals.

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    I wonder if we could have local tasks, menu links, etc, also defined in the controller attribute. When creating a controller, and you want a local task to appear, it might be quite nice DX to just simply attach an attribute to the controller, rather than having to edit YAML files and have to manage the references. If things like local tasks can be defined in an attribute with the controller, that also simplifies it slightly because then you don't need to know or care what the route name is.

  • 🇫🇷France andypost

    @AaronMcHale great idea, ++ to add to summary and probably as follow-up issue

  • 🇷🇴Romania claudiu.cristea Arad 🇷🇴

    Missing hook_menu()? :)))

  • 🇬🇧United Kingdom joachim

    > Personally I think I like automatic route names based on the class name.
    > Obviously it should include the namespace.

    It's not explicitly stated in the BC policy, but route names have to be public API so that you can work with them to generate links, add extra pages to the UI, etc.

    Automatic route names would mean the controller class name becomes API. I'm not automatically against that, but it's not what we're used to.

    Also, it would worsen DX because how is a developer supposed to find the route name?

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    Building on my previous comment, and the concerns about automatic route naming. If we're going to use attributes anyway, then why not just define the route name in the attribute.

    That would address the BC and DX problems stated above, automatic route naming sounds nice in theory but it doesn't sound like it's worth it for the trade-offs mentioned.

    It could also make it harder for new developers to understand what's going on, if the route names are generated automatically, we're abstracting away a key piece of information that could really help someone new understand routes.

    The big DX win here is the ability to define the route (and potentially links, etc) right there in the controller class. Just that in itself is a huge win!

  • 🇬🇧United Kingdom dakala Swindon

    Not sure if this is irrelevant but a contrib module is already using PHP attributes for hook discovery - https://www.drupal.org/project/hux

  • 🇬🇧United Kingdom joachim

    > If we're going to use attributes anyway, then why not just define the route name in the attribute.

    +1

    > Missing hook_menu()? :)))

    Hmmm yes and no! Having to mess about in THREE different Yaml files to add a new admin tab is tedious. But hook_menu() wasn't simple, and there were plenty of ways that could go wrong too. I seem to remember complications around the MENU_DEFAULT_TASK value or something like that.

    So I'd cautiously say yes to some sort of additional attributes to say 'Put this route in the menu' and 'Put this route in a tab set'. But as a follow-up issue - let's keep this one reasonably simple, and allow addition of future attributes when we design it.

  • 🇬🇧United Kingdom joachim

    > The defaults._controller value is determined by the method you add the attribute to

    I don't want to add complexity to this issue, so it should be a follow-up, but we should bear in mind in the building of this that we might want to be able to also use a Route attribute on a form class, where defaults._form would be the route property that gets set to the form class, rather than defaults._controller set to the method.

  • 🇩🇪Germany donquixote

    I don't want to add complexity to this issue, so it should be a follow-up, but we should bear in mind in the building of this that we might want to be able to also use a Route attribute on a form class, where defaults._form would be the route property that gets set to the form class, rather than defaults._controller set to the method.

    I support it. But this should be a different attribute class.

    So I'd cautiously say yes to some sort of additional attributes to say 'Put this route in the menu' and 'Put this route in a tab set'. But as a follow-up issue - let's keep this one reasonably simple, and allow addition of future attributes when we design it.

    A question is do we "pollute" the route list with these meta keys, or do we collect a different kind of item that we then use to generate routes _and_ links?

  • Status changed to Needs review 7 months ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Updated MR to be from 11.x and updated all the deprecations to be for 10.3.0 as this is not going to make 10.2.0

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    POTX can read attributes - yay - so I think this is ready... going to add back the conversion of a single route. I think this issue is ready FWIW.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    Very interesting — this looks indeed … very ready. Posted a few questions on the MR :)

    We should and can build CLI tools to expose all the routes provided by a module. This would be good not just for routes determined by attributes but also dynamic routes

    This is true for not just routes, but also for all plugins of any given type: that is a frequent challenge when working on migrations (), validation (), and more.

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    I've added code to not support certain things (locale and localized paths) form the Symfony route attribute. I guess there is a question about whether we should have our own attribute... but it is kinda nice to not have our own. Not as sure as I once was :)

  • 🇫🇷France andypost

    Related may need some work too

  • First commit to issue fork.
  • @alexpott
    You created two change records:
    - https://www.drupal.org/node/3324758
    - https://www.drupal.org/node/3314769

    Neither of them had any information in it, so I started filling out some information on the first.
    Is there a reason for us to keep the second one around?

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    @duadua I think I got a bit change-record-keen :)

    I really like the idea of copying Symfony 6.4 and adding an always there FQCN route alias. That would mean getting 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work done. Definitely think it is worth a follow-up. I wonder if we should forgo the automatically generated names like 'MethodRouteLocale' and instead where no name is provide automatically name using the FQCN that Symfony would create an alias for?

  • Good question. I think I'd expect these to be declared in a modules services file - maybe an event subscriber. Yeah - this change introduces RoutingEvents::STATIC so these callbacks could be changed to an event subscriber listening to that event and then add their routes in.

    Added https://www.drupal.org/project/drupal/issues/3401172 📌 Figure out how to handle generating dynamic Active

  • @duadua I think I got a bit change-record-keen :)

    For a while I was trying to figure out whether there is a logical space in which the slightly different title might exist.
    I tried to delete the particular change record, but there are some additional permissions required. Someone will probably see the updated title .

  • I really like the idea of copying Symfony 6.4 and adding an always there FQCN route alias. That would mean getting #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name done. Definitely think it is worth a follow-up. I wonder if we should forgo the automatically generated names like 'MethodRouteLocale' and instead where no name is provide automatically name using the FQCN that Symfony would create an alias for?

    What feels very sensible for me. Looking at the current way its generated, a FQCN name would be no way longer compared to the current autogeneration.
    Looking at this it just let me realize that auto-generation has the advantage of avoiding accidental re-definitions of the same route name.

    CREATE TABLE IF NOT EXISTS "router" (
    "name" VARCHAR(255) NOT NULL DEFAULT '',
    "path" VARCHAR(255) NOT NULL DEFAULT '',
    "pattern_outline" VARCHAR(255) NOT NULL DEFAULT '',
    "fit" INTEGER NOT NULL DEFAULT 0,
    "route" BLOB DEFAULT NULL,
    "number_parts" INTEGER NOT NULL DEFAULT 0,
    "alias" VARCHAR(255) NOT NULL DEFAULT '',
     PRIMARY KEY ("name")
    );
    CREATE INDEX "router_pattern_outline_parts" ON "router" ("pattern_outline", "number_parts");
    CREATE INDEX "router_alias" ON "router" ("alias");
    

    Looking at the schema for the router, I see there is a limit of length for the route name. Looking at everything, 255 seems plenty of space though for:

    • Drupal
    • Module name
    • "Controller"
    • Controller name
    • Method name
  • Status changed to Needs work 7 months ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇮🇳India guptahemant

    Couple of scenarios which comes in my mind, which we should address:

    • Currently multiple route definition can point to a single class method, How we will define routes like that with php attributes?
    • Route names are used to define menu links, menu tabs and at various other places also, I think we are aiming to generate auto route names but it also kind of relates to above point, How we are going to address that?
    • What would be the process of overriding an existing route? Using route subscribers or something else?
Production build 0.69.0 2024