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
Comment | File | Size | Author |
---|---|---|---|
#22 | routes-2092529-22.patch | 3.57 KB | tim.plunkett |
Comments
Comment #1
brianV CreditAttribution: brianV commentedAdding tag
Comment #2
larowlanRouteSubscriberBase perhaps - to be consistent with FormBase etc?
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
Comment #3
donquixote CreditAttribution: donquixote commentedI'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?
Comment #4
larowlanthe main argument for plugins is it limits exposure to symfony concepts
Its a hierarchy
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
Comment #5
dawehnerWhat about making this an abstract class and an abstract method, so people know what they should override.
We should not throw away routes without any kind of warning ... note: you might do $routes[] = new Route()...
Comment #6
brianV CreditAttribution: brianV commented> 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?
Comment #7
dawehnerI 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.
Comment #8
brianV CreditAttribution: brianV commentedOk, 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?
Comment #9
damiankloip CreditAttribution: damiankloip commentedHang 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?
Comment #10
dawehnerPersonally I think we over-architect stuff here.
Comment #11
brianV CreditAttribution: brianV commented@damiankloip - yep, I based on the same idea as your patch.
Comment #12
brianV CreditAttribution: brianV commented@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?
Comment #13
damiankloip CreditAttribution: damiankloip commentedWe did actually explore something similar for access callbacks before but it got shot down.. :/
Comment #15
tim.plunkettFrom #2091411-17: Provide an easier mechanism for a route definition wrapped by module_exists():
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.
Comment #16
donquixote CreditAttribution: donquixote commentedOne 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.
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.
Comment #17
Crell CreditAttribution: Crell commentedHiding 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.
Comment #18
tim.plunkettThere 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.
Comment #18.0
tim.plunkett.
Comment #19
catchDon'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.
Comment #20
chx CreditAttribution: chx commentedThis 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?
Comment #21
chx CreditAttribution: chx commentedBecause 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:
Comment #22
tim.plunkettAll 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...
Comment #23
dawehner@tim Please create a new issue for that, this is a meta!
Comment #24
chx CreditAttribution: chx commentedThis should block the beta.
Comment #25
dawehner22: routes-2092529-22.patch queued for re-testing.
Comment #27
tim.plunkettThat 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.
Comment #28
Crell CreditAttribution: Crell commentedI'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!
Comment #29
xjmMarking as a duplicate, then.