In Drupal 7, it was simple to dynamically define routes. Just wrap the item in a conditional in your hook_menu() implementation:

if ($this_variable_is_true) {
  $items['example_path'] = array(
    'title' => 'Example Page',
    'page callback' => 'example_page_callback',
    'access arguments' => array('view example page'),
    'type' => MENU_CALLBACK,
  );
}

In Drupal 8, the process is much more tedious:

1. Define a service in example.services.yml tagged as an event subscriber.
2. Create your ExampleEventSubscriber class implementing EventSubscriberInterface.
3. Register a listener in getSubscribedRequest
4. Create your listener method.
5. Fetch the route collection
6. Add your routes to the route collection.

This patch adds a new RouteSubscriber class which implements EventSubscriberInterface in an attempt to simplify the DX for adding dynamic routes. The new process is:

1. Define a service in example.services.yml tagged as an event subscriber.
2. Create your ExampleRouteSubscriber class extending RouteSubsciber.
3. Override the defineRoutes() method.

The attached patch is a proof of concept, and implements the new class and converts a module to use it. I apologize for the obscure choice of module to convert in this patch, but it's the first one that jumped to mind.

Looking for feedback first.

Related Issues

Create Base Class for RouteSubscriber Class

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

Adding tag

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteSubscriber.php
    @@ -0,0 +1,58 @@
    +class RouteSubscriber implements EventSubscriberInterface {
    

    RouteSubscriberBase perhaps - to be consistent with FormBase etc?

  2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/EventSubscriber/FormTestRouteSubscriber.php
    @@ -0,0 +1,50 @@
    +   * Definesroutes for the Form Tests.
    

    missing space

I like the concept. I think we could go one step further and make a plugin type and a plugin manager, then use the plugin manager to collect the plugins and call getDefinedRoutes on them.

That would remove the need to define a service as well, the plugin manager would register itself as an event listener and handle the events on behalf of the plugins.

So you would need to a) create a plugin implementing an interface (with one public method getDefinedRoutes) and thats it

donquixote’s picture

I'd say what brianV suggests is clearly easier to pull off, because it does not change anything fundamentally, just factor some stuff out into a base class.

If we switch this to plugins, then maybe every subscriber should be changed to a plugin?
Or the plugin manager could extend RouteSubscriber..

Do we have some general arguments of service + tags vs plugins somewhere?

larowlan’s picture

the main argument for plugins is it limits exposure to symfony concepts
Its a hierarchy

  • As it stands in HEAD you need to understand services, events and routes to create a dynamic route
  • The patch in the OP removes the need to learn events, so its just routes and services
  • The plugin approach removes the need to learn services, so its just routes

We could go one step further and make the getDefinedRoutes method return arrays instead of keyed Route objects but thats getting us into exposed data structures/arrays of doom territory and I for one would prefer a first class object over that

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteSubscriber.php
    @@ -0,0 +1,58 @@
    +class RouteSubscriber implements EventSubscriberInterface {
    ...
    +  protected function defineRoutes() {
    +    return array();
    +  }
    

    What about making this an abstract class and an abstract method, so people know what they should override.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteSubscriber.php
    @@ -0,0 +1,58 @@
    +        if ($route instanceOf Route && is_string($key)) {
    +          $collection->add($key, $route);
    +        }
    

    We should not throw away routes without any kind of warning ... note: you might do $routes[] = new Route()...

brianV’s picture

> What about making this an abstract class and an abstract method, so people know what they should override.

That's a good idea.

> We should not throw away routes without any kind of warning ... note: you might do $routes[] = new Route()...

Yes, that crossed my mind as I was writing the code, but I decided to keep the code simple in the initial patch to keep the idea simple generate discussion of the basic concept. I'll roll this patch in a more complete fashion if conversation suggests this is the basic idea we should follow.

In the meantime, I'd like to see more input on the plugin idea. I like the fact that moving these to plugins abstracts 'joe module developer' away from Symfony by another step, and saves having to define a service.yml.php for a lot of simple modules.

@donquixote - are there any other event subscriber types / patterns that *should* be plugins?

dawehner’s picture

I think it is a bad thing to go for plugins for a specific usecase in the routing system. Yes for sure plugins would also work
all over the routing system (for example access checkers are an example for it).

It seems to be for me that moving to plugins would cause more inconsistency in the routing subsystem itself.

brianV’s picture

Ok, how's this for a different way to go:

We add a new option for routes that specifies a callback. This callback determines whether the route should be enabled or not. This allows you to set dynamic routes directly in your module.routing.yml file.

The attached patch implements it as a proof of concept.

Pros:
You can add dynamic routes right in your module.routing.yml without needing to create any services, plugins or anything beyond a callback that is either a static method or a function in your .module file.

Shortcomings:
It's still not easy to allow dependency injection here if we do as I do in the patch and make the callback a static function in the controller.

Thoughts?

damiankloip’s picture

Hang on, that is just the patch from #2091411: Provide an easier mechanism for a route definition wrapped by module_exists() with a couple of things renamed?

dawehner’s picture

Personally I think we over-architect stuff here.

brianV’s picture

@damiankloip - yep, I based on the same idea as your patch.

brianV’s picture

@dawhener - ok, do you have a suggestion? Given that the current DX for this type of task sucks, how do *you* recommend we approach it without 'over-architecting' a solution?

damiankloip’s picture

We did actually explore something similar for access callbacks before but it got shot down.. :/

Status: Needs review » Needs work

The last submitted patch, drupal8.routing-system.2092529-8.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Postponed

From #2091411-17: Provide an easier mechanism for a route definition wrapped by module_exists():

I think we have two things here: Dynamically adding routes, and actual dynamic routes. Examples like dblog.search do not really have anything dynamic about the route, just whether we want the route to be included or not

I cannot think of any easy examples similar to module_exists. Everything else is about foreach-ing through some data.

So I think we should postpone this until we've finished converting the page callbacks, and then we can decide if there are other easy patterns we should support.

Assigning to myself so that we don't forget to unpostpone, but if anyone wants to steal it please feel free.

donquixote’s picture

One relevant example would be Drupal\views\EventSubscriber\RouteSubscriber.
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

Having an abstract base class for this would make things easier.
But even as it is now, that specific class does not look overly complex to me.
Having getSubscribedEvents() explicit could even be preferable, for the sake of transparency.

I am not in favour of plugins.
If we do anything about it, I prefer an abstract base class. But not exactly like the inital patch.

namespace Drupal\views\EventSubscriber;
[..]
class RouteSubscriber extends AbstractRouteSubscriber {

  /**
   * {@inheritdoc}
   */
  public function dynamicRoutes(RouteCollection $collection) {
    $views = views_get_applicable_views('uses_route');
    foreach ($views as $data) {
      list($view, $display_id) = $data;
      if ($view->setDisplay($display_id) && $display = $view->displayHandlers->get($display_id)) {
        if ($display instanceof DisplayRouterInterface) {
          $display->collectRoutes($collection);
        }
      }
      $view->destroy();
    }
  }
}

Imo this is preferable to passing arrays around.
One big advantage: If any component wants to register a broken route, then the method that defines the broken route will be in the stack trace.

Crell’s picture

Hiding events and services from people is a fool's errand. We should not be hiding such important concepts from them. Module developers WILL be exposed to those concepts; that's a good thing. Trying to avoid them will only make everyone's life miserable.

That said, simplifying common patterns with utility base classes is a worthwhile task, and I'm OK with that in concept.

Using plugins here is right-out. The routing system is currently plugin-free, which means it's totally decoupled from plugins. We want that. A given route subscriber may call out to the plugin system for data the way Views does; that's fine, and we couldn't stop it anyway. But the main routing system itself should be entirely decoupled from plugins.

I'm fine with Tim's suggestion of circling back to this once we have a better grip on what the common patterns to simplify are.

tim.plunkett’s picture

Title: Improve DX for defining custom routes. » [meta] Improve DX for defining custom routes

There is not one perfect way to solve this problem. We should take incremental steps. Reopening #2098795: Create Base Class for RouteSubscriber Class as a simplified and uncontroversial version of some things we had discussed here.

tim.plunkett’s picture

Issue summary: View changes

.

catch’s picture

Category: Bug report » Task
Priority: Normal » Critical
Issue summary: View changes
Status: Postponed » Active

Don't think a meta issue needs to be postponed. Also marked #2133707: Regression: routes can no longer be defined in one place as duplicate. Bumping priority because this is currently horrible and I think we at least need to make it a bit less horrible before we can release.

chx’s picture

This is just one issue to justify rolling back the router. I am sure once we actually get down to solving D8 performance , that will be a second one. How many do you need?

chx’s picture

Because webchick asked in her AMA (even without mentioning names, I can read it well enough) I am willing to put up with the status quo if the internals gets documented and the DX gets fixed:

  1. #2109035: Make access checkers (much) easier to find
  2. #2092529: [meta] Improve DX for defining custom routes
  3. #2046367: Document the internals of the router
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Active » Needs review
FileSize
3.57 KB

All of the other route subscribers have foreach() loops, except \Drupal\image\EventSubscriber\RouteSubscriber which has a dynamic path.

This was the only other that could be converted.

We added a base class, and we added this _module_dependencies key. No other suggestions have been put forward...

dawehner’s picture

@tim Please create a new issue for that, this is a meta!

chx’s picture

This should block the beta.

dawehner’s picture

22: routes-2092529-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: routes-2092529-22.patch, failed testing.

tim.plunkett’s picture

That patch is obsolete.

In fact, this whole issue might be obsolete now that #2145041: Allow dynamic routes to be defined via a callback is in.

Crell’s picture

Status: Needs work » Fixed
Related issues: +#2145041: Allow dynamic routes to be defined via a callback

I'm going to say yes. The new callback allows for fancy logic, and is only very marginally more work to setup than hook_menu was. (You need an entry in the yml file, and you may need dependent services.) From a DX perspective I think this is done.

We may find other common patterns, like _required_modules or whatever, but those would be add-ons niceties and not breaking-changes. The critical part of this issue is, IMO, resolved.

Thanks, Tim!

xjm’s picture

Status: Fixed » Closed (duplicate)

Marking as a duplicate, then.