Allow dynamic routes to be defined via a callback

I like the general idea, but this should just expect an array to be returned from the callback, exactly how you'd make static routes in the yaml file, and then they can get turned into a RouteCollection by whatever collects them.

nice

Just to express my thoughts:

░░░░░░░░░░░░░░░░░▄▄███▄▄▄░▄▄██▄░░░░░░░
░░░░░░░░░░██▀███████████████▀▀▄█░░░░░░
░░░░░░░░░█▀▄▀▀▄██████████████▄░░█░░░░░
░░░░░░░░█▀▀░▄██████████████▄█▀░░▀▄░░░░
░░░░░░▄▀░░░▀▀▄████████████████▄░░░█░░░
░░░░░░▀░░░░▄███▀░░███▄████░████░░░░▀▄░
░░░░▄▀░░░░▄████░░▀▀░▀░░░░░░██░▀▄░░░░▀▄
░░▄▀░░░░░▄▀▀██▀░░░░░▄░░▀▄░░██░░░▀▄░░░░
██░░░░░█▀░░░██▄░░░░░▀▀█▀░░░█░░░░░░█░░░
░█░░░▄▀░░░░░░██░░░░░▀██▀░░█▀▄░░░░░▀█▀▀
░▀▀▀▀█░▄▄▄▄▄▀▀░█░░░░░░░░░▄█░░█▀▀▀▀▀█░░
░░░░░█░░░▀▀░░░░░░▀▄░░░▄▄██░░░█░░░░░▀▄░
░░░░░█░░░░░░░░░░░░█▄▀▀▀▀▀█░░░█░░░░░░█░
░░░░░▀░░░░░░░░░░░░░▀░░░░▀░░░░▀░░░░░░░░

Why the hell do we so active try to hide the symfony routing system from the users.

Having a route object helps you sooooo much to know how things are called etc.

1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
   @@ -28,41 +26,23 @@ class ContentTranslationRouteSubscriber extends RouteSubscriberBase {
       */
   -  protected function routes(RouteCollection $collection) {
   +  protected function alterRoutes(RouteCollection $collection, $module) {
        foreach ($this->contentTranslationManager->getSupportedEntityTypes() as $entity_type => $entity_info) {
   -      // First try to get the route from the dynamic_routes collection.
   +      // Try to get the route from the current collection.
          if (!$entity_route = $collection->get($entity_info['links']['canonical'])) {
   -        // Then try to get the route from the route provider itself, checking
   -        // all previous collections.
   -        try {
   -          $entity_route = $this->routeProvider->getRouteByName($entity_info['links']['canonical']);
   -        }
   -        // If the route was not found, skip this entity type.
   -        catch (RouteNotFoundException $e) {
   -          continue;
   -        }
   +        continue;
          }
          $path = $entity_route->getPath() . '/translations';
    
   

   Just to be fare, this could have been done before as well. The only reason why it was not, was that other people can alter those routes easily as well.

2. +++ b/core/modules/image/lib/Drupal/image/EventSubscriber/RouteSubscriber.php
   @@ -7,34 +7,31 @@
   +    return $routes;
   

   Let's at least ensure that $routes is not empty.

I wouldn't say the route subscribers were super complex before :-)

Doing this just seems like we're regressing slightly from what we currently have with collections. If we go much further, we may as well just bring back hook_menu.

It also seems wrong to move everything into alter.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AccessRouteSubscriber.php
   @@ -53,7 +53,7 @@ public function onRoutingRouteAlterSetAccessCheck(RouteBuildEvent $event) {
   +    $events[RoutingEvents::ALTER][] = array('onRoutingRouteAlterSetAccessCheck', -1000);
   

   I am little bit worried about the need of so many "magic" priorities.

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -70,12 +78,15 @@ class RouteBuilder {
   +   * @param \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver
   +   *   The controller resolver.
   

   Just as a futurure todo: It would be cool to have a generic object in another namespaces which does that, given that using the ControllerResolver is kind of semantically wrong here. We have a specialized resolver (TitleResolver) already.

3. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -98,6 +109,17 @@ public function rebuild() {
   +          $controller = $this->controllerResolver->getControllerFromDefinition($route_callback);
   

   Can we use 'callback' maybe instead? Controller feels kind of weird.

4. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -98,6 +109,17 @@ public function rebuild() {
   +            foreach ($controller_routes as $name => $controller_route) {
   +              $collection->add($name, $controller_route);
   +            }
   

   In case we would return a route collection this code would just be $collection->addCollection($controller_collection);

5. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
   @@ -28,41 +26,23 @@ class ContentTranslationRouteSubscriber extends RouteSubscriberBase {
   -        try {
   -          $entity_route = $this->routeProvider->getRouteByName($entity_info['links']['canonical']);
   -        }
   

   That line has always been wrong.

6. +++ b/core/modules/image/lib/Drupal/image/EventSubscriber/ImageStyleRoutes.php
   @@ -7,26 +7,29 @@
   +   * Returns an array of route objects.
   +   *
   +   * @return \Symfony\Component\Routing\Route[]
   +   *   An array of route objects.
   ...
   +  public function routes() {
   

   Can't we just use an interface and specify just the class? Just wondering, what is the usecase of a class with multiple route callbacks.

7. +++ b/core/modules/image/lib/Drupal/image/EventSubscriber/ImageStyleRoutes.php
   index 34fd75d..34d49a4 100644
   --- a/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
   
   --- a/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
   +++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/ResourceRoutes.php
   

   Should we also move the files into another directory so they are easier to find, especially because they are not event subscribers anymore.

8. +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterTest.php
   @@ -115,11 +115,6 @@ public function testControllerPlaceholdersDefaultValuesProvided() {
   -    // Test the dynamically added route.
   -    $this->drupalGet('router_test/test5');
   -    $this->assertResponse(200);
   -    $this->assertRaw('test5', 'The correct string was returned because the route was successful.');
   

   What is the reason to remove that test here?

Switching from a DYNAMIC_ROUTE event to a callable: +1
Having that callable return Route objects rather than arrays: +1
Having that callable return an array of route objects rather than a RouteCollection: -1
Moving definitions from dynamic routes to route alter: + Er, why?

Due to the way PHP works, the code as written here would allow both a RouteCollection and an array to work, I think, since RouteCollection is iterable. That said, from an API definition perspective I far prefer returning a RouteCollection than "an array keyed by name, or something that could be iterated as such". That's less of an API-by-design and more an API-by-implementation, which is in most cases inferior.

+++ b/core/modules/image/lib/Drupal/image/Routing/ImageStyleRoutes.php
@@ -5,28 +5,31 @@
+   * Returns an array of route objects.
+   *
+   * @return \Symfony\Component\Routing\Route[]
+   *   An array of route objects.

+++ b/core/modules/search/lib/Drupal/search/Routing/SearchPluginRoutes.php
@@ -0,0 +1,70 @@
+
...
+   */

The @inheritdoc is a lie ;)

I think returning an array of route objects is slightly nuts :/ What do we gain from that? Seems like we just lose?

This started out as an issue to keep things more in line with the YAML equivalent? Then we started returning arrays of route objects...huh? :)

No interaction with RouteCollection is needed (and therefore the collection cannot be improperly modified)

I really think that is a bad thing (having no interaction with a collection). Being able to modify them is a fair point raised, only alters should be allowed to do that.

My first choice would be b) It does not required you to know about the route collection but it lets you use a route object, which really helps you to know what exactly can be passed in.

C is my strong preference. Nearly everywhere else in code we use RouteCollections for "a set of routes"; Writing a new route filter requires collections; writing a route alter requires collections; using a RouteCollection here is consistent with that. Not doing so is inconsistent.

Failing that, B I can tolerate. At least it's still using Route objects.

D makes no sense at all, frankly. And A is right out unacceptable IMO.

B is the best option.

If all you're doing is providing a list of routes (not altering an existing set), I see absolutely no value in forcing people to use a RouteCollection. This is not an issue of consistency because this isn't working with a collection of routes, it's just route discovery. Nothing in the RouteCollection interface is adding value here, it's just requiring knowledge of another class for no benefit.

From the options in the issue summary, I like B as well. All of them still have the ugly '_stuff' but that's another battle :)

Edit: the patch looks good too but I prefer to let someone more familiar with the routing system RTBC it...

+1 b

Allow dynamic routes to be defined via a callback

From the title, this issue is a good idea. However, I still don't understand this burning desire to return an array? Can someone explain that to me? :)

From what I can see, there are not really any benefit of doing this, as I mentioned above (in my previous comment) - we still just lose. Except, this is Drupal...

Sorry, not trying to be awkward. honest.

damian: Well, a module can define multiple dynamic routes. That means either an array of routes, or a collection object that holds multiple routes. Which of those we do seems to be the only point of contention remaining. :-)

Thanks, I get that, and understand the issue ;) I guess you mis interpreted my comment. I was basically saying I don't agree with returning an array.

See my previous comment to that; I'm advocating keeping the use of RouteCollection objects.

For what it's worth I agree with @msonnabaum's remarks in #39. And there's definitely an improvement regardless.

Also, nice diffstat on the patch.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -98,6 +109,17 @@ public function rebuild() {
+      if (isset($routes['route_callbacks'])) {
+        foreach ($routes['route_callbacks'] as $route_callback) {
+          $callback = $this->controllerResolver->getControllerFromDefinition($route_callback);
+          if ($callback_routes = call_user_func($callback)) {
+            foreach ($callback_routes as $name => $callback_route) {
+              $collection->add($name, $callback_route);
+            }
+          }
+        }
+        unset($routes['route_callbacks']);
+      }

Just a note, strictly speaking since RouteCollection implements IteratorAggregate over the internal routes list, the foreach loop here will work with both an associative array *and* a RouteCollection. The code as written supports both equally well. So really, all we're debating is what to put in the documentation for which you're "supposed" to do. Or we just document it as "an iterable of route names and route objects, such as an associative array or RouteCollection."

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
@@ -36,7 +36,13 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager) {
-  public function routes(RouteCollection $collection) {
+  protected function alterRoutes(RouteCollection $collection, $provider) {
+    // Because ConfigMapperManagerInterface uses the route provider to determine
+    // the routes to add here this must only run during the dynamic alter stage.
+    if ($provider != 'dynamic_routes') {
+      return;
+    }
+

As discussed in IRC, I don't think the dynamic_routes pseudo-provider is even needed anymore. Dynamic routes from foo module will get passed through the ALTER event along with all other foo module routes.

We should remove the dynamic_routes alter call entirely, which means this check (and other references to dynamic_routes in this patch) should be able to just go away.

Aside from that (which includes references not quoted here), I'm fine with the rest.

Please try to keep the scope of the issue in place. (Who will be able to understand all the effects of this patch)
Removing an alter event on any dynamic created is so wrong.
We rely on the alter event for more than just defining routes. ParamConverterSubscriber is just one of those examples.
You won't be able to alter any REST/Views/Panels routes.

Additional this is an unneeded API change.

Can't we just add the callback support, keep the dynamic events support and be happy?

Each mapper gets the route provider *that is still being built* and tries to inspect it for other routes.

I have been complaining about that approach since a long long time. What still should work from my perspective is using alter events + dynamic events exactly like views is doing it. Yes this is quite an effort though it solves the problem from ground up.

@tim
The reason why I posted was to not let you run into a horrible mistake.
Not sure whether you really want to let me review this patch.

1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
   @@ -28,41 +26,23 @@ class ContentTranslationRouteSubscriber extends RouteSubscriberBase {
   +  protected function alterRoutes(RouteCollection $collection, $provider) {
   ...
          if (!$entity_route = $collection->get($entity_info['links']['canonical'])) {
   ...
   +      // Try to get the route from the current collection.
   

   Do we really have to add new routes on alter? This opens a new level of complexity, new bugs etc.

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
   @@ -28,43 +26,25 @@ class RouteSubscriber extends RouteSubscriberBase {
   +        // Try to get the route from the current collection.
            if (!$entity_route = $collection->get($entity_info['links']['admin-form'])) {
   ...
   +  protected function alterRoutes(RouteCollection $collection, $provider) {
        foreach ($this->manager->getDefinitions() as $entity_type => $entity_info) {
   

   Same as before.

3. +++ b/core/modules/image/lib/Drupal/image/Routing/ImageStyleRoutes.php
   @@ -5,28 +5,31 @@
   -class RouteSubscriber extends RouteSubscriberBase {
   +class ImageStyleRoutes {
   

   This makes it harder to find examples of callbacks :(

4. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
   @@ -97,7 +97,7 @@ protected function getViewsDisplayIDsWithRoute() {
   +  protected function alterRoutes(RouteCollection $collection, $provider) {
   ...
   -  protected function routes(RouteCollection $collection) {
   
   +++ b/core/modules/system/tests/modules/router_test_directory/lib/Drupal/router_test/RouteTestSubscriber.php
   --- a/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
   +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
   
   index c8b0437..5768dd3 100644
   --- a/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
   

   So why does views not use a callback in the routing.yml file to define the dynamic non-alter routes?

After some discussion in IRC, I'm going to RTBC this.

1) It is a DX improvement on its own.
2) There is no new bug introduced here. Rather, there's some conversion that cannot be fully completed due to pre-existing bugs in config_translation. config_translation is already buggy because it's trying to use the route provider within the route build process, which is a circular dependency. That it hasn't already blown up is purely due to luck.
3) There is already a follow-up to sort out config_translation, which is referenced from this patch.

So let's let this land, then finish fixing config_translation separately. That way we still get the DX improvement for normal dynamic routes now as a Christmas gift to contrib developers. :-)

Okay.

It looks like there is consensus, and I find myself in agreement that (b) is the best options. I'm not sure I'd call it 'Major' but it is certainly a DX improvement. Committed to 8.x. Thank you.

I'm getting the following error/exception today, even when trying to rebuild via the rebuild script.

Scanning the commit log, it seems to be caused by this change, as it appears to be the only one that touched this area in the past days.

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'field_ui.instance_edit_comment' for key 'PRIMARY' in core\lib\Drupal\Core\Database\Statement.php on line 54
{main}( )
drupal_rebuild( )
drupal_flush_all_caches( )
Drupal\Core\Routing\RouteBuilder->rebuild( )
Drupal\Core\Routing\MatcherDumper->dump( )
Drupal\Core\Database\Driver\mysql\Insert->execute( )
Drupal\Core\Database\Connection->query( )
Drupal\Core\Database\Statement->execute( )
execute ( )

IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'field_ui.instance_edit_comment' for key 'PRIMARY': INSERT INTO {router} (name, provider, fit, path, pattern_outline, number_parts, route) VALUES (...snip...)
{main}( )
drupal_rebuild( )
drupal_flush_all_caches( )
Drupal\Core\Routing\RouteBuilder->rebuild( )
Drupal\Core\Routing\MatcherDumper->dump( )

(FWIW, I don't know why the exception is thrown twice, and why the first has a better backtrace and why the second has a better exception message...)

Created #2160811: Router rebuild fails (even via rebuild script), in case the provider of existing routes has changed

The "Providing dynamic routes and altering existing routes" handbook page at https://drupal.org/node/2122201 might need an update ?

We have to update or add new change notice for this.

Tagging to write up the change record.

Proposed change notice:

Dynamic Routes can be defined in routing.yml

Previously, if you wanted to create routes dynamically, you'd need to create a class that extends RouteSubscriberBase and create a service definition tagged with 'event_subscriber' for it to be picked up.

Now you can declare a dynamic routing method directly in your routing.yml file that returns an array of \Symfony\Component\Routing\Route objects or a Symfony\Component\Routing\RouteCollection object.

Example snippet from my_module.routing.yml:

route_callbacks:
  - '\Drupal\mymodule\Routing\SomeClass::routes'

I think we should add SomeClass.php to the change notice as well.

I am just asking that what is the class definition of \Drupal\mymodule\Routing\SomeClass or how the method \Drupal\mymodule\Routing\SomeClass::routes looks like?

Ah ok. I'll just add the examples in the issue summary.

Marking fixed.

Thank you @kim.pepper for the change notice and update.

Well, we can still use the event based method using a Route subscriber too.