Add getAllRoutes() method to RouteProvider

For certain cases, we need to have all routes in the system without providing $name or $path. This might not be a use case of core and surely contrib modules would need it. Add adding SELECT query in contrib might not be a good choice

Can you give an example where this is needed? Note: The current interface makes it potentially possible to use in systems without a list all interface.

If you want to add information to them you can always use the route events ...
This would add a massive amount of memory to your system so using this should be really discouraged.

Well the use-case is in config_translation. We parse all routes and find all of them that contain a '_form' declaration in order to generate 'Translate' tabs on certain configuration form. The only other alternative would be a central form registry, but that's not going to happen in D8, so really there is no other way to say "Give me all configuration forms" than to find them manually by scanning the routes.

Regarding the actual patch:

1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -262,4 +262,24 @@ protected function getRoutesByPath($path) {
   +   * Note that this method may not throw an exception if some of the routes
   +   * are not found. It will just return the list of those routes it found.
   

   I don't understand this comment. Why would someone expect an exception to be thrown? And which routes may not be found?

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -262,4 +262,24 @@ protected function getRoutesByPath($path) {
   +   *   Iterable thing with the keys the names of the $names argument.
   

   We usually don't call stuff "iterable thing" even though it may be technically correct. I would prefer "An array of routes." or "A list of routes." if you find array too specific.

@dawehner: basically config translation needs to map an array of cmi keys to a path multiple times. When talking about this with @alexpott and @effulgentsia, our initial idea of automating that was putting it in the route itself. They did not like it because the route should not know what the controller is doing. The controller should know. So they suggested that we add a getConfigNames() method on config form controllers (#2095289: Make getEditableConfigNames() public and use it in config_translation and #2095291: Remove *.config_translation.yml for config forms, use form getConfigNames() ). For this though, since we need the cmi keys mapped to routes anyway, we need to iterate through all the routes and then see if they are config forms and invoke their getConfigNames() and then associate that back to the route.

So we need all routes for this to work. We also need to instantiate the controller (which sounds like may be tricky, but @tstoeckler told me he cracked it), then invoke it. May be too much hassle IMHO but @alexpott and @effulgentsia have seen this as the right placement of concerns.

What do you think?

1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -262,4 +262,24 @@ protected function getRoutesByPath($path) {
   +    $route_list = $this->connection->query("SELECT name, route FROM {" . $this->connection->escapeTable($this->tableName) . "}")
   

   Is there any serious sort we could apply and would make sense?

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -262,4 +262,24 @@ protected function getRoutesByPath($path) {
   +      $routes[] = unserialize($route);
   

   I guess we want to have the result keyed by the route name?

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -262,4 +262,24 @@ protected function getRoutesByPath($path) {
+  /**
+   * Provides all the routes on the system.
+   *
+   * Note that this method may not throw an exception if some of the routes
+   * are not found. It will just return the list of those routes it found.
+   *
+   * @return \Symfony\Component\Routing\Route[]
+   *   Iterable thing with the keys the names of the $names argument.
+   */

I think this function should explain that you should not use this function if possible.

Updated for the above comments.

I tested this with all of core and a bunch of D8 contribs and the $routes array returned by getAllRoutes() was 1.5MB. So while it's not great, it's not terrible either and using it during cache clear, as we do in config_translation shouldn't be a problem, IMO.

We very specifically designed the route dumper to never have all routes in memory at once. We absolutely should not then go back on that and dump everything into memory at once again. That 1.5 MB is only going to grow once you have a bunch of views defined, a few dozen contrib modules, etc. This is entirely the wrong approach to take.

@Crell: ok, lets step back a bit then. The way we arrived here is config translation needs to associate CMI keys with routes/paths, so it can attach a translate tab to pages with forms generated for those CMI keys from the config schemas. So we need to associate CMI keys with routes for that. We currently do that in config_translation using YML plugin discovery through modules defining *.config_translation.yml files.

That is where @effulgentsia and @alexpott suggested to not do that custom YML file but instead have a generic way to tell which config keys are managed by which forms. This may be useful for all kinds of other reasons later on. So their idea was to add a getConfigNames() method on forms (#2095289: Make getEditableConfigNames() public and use it in config_translation) so that forms can tell others about their CMI keys. That is all fine, but config translation still needs the list of CMI keys associated with routes. So we need to discover all the routes that are forms and then filter them to those implementing getConfigNames(). That essentially means we need to iterate through *all the routes* and figure out which ones are config forms and then *instantiate* them and then call this method on them and get the config keys (and still figure out if there is any schema for those keys and if those schemas define anything translatable). I believe this is a pretty heavy handed approach (especially compared to the basic YML discovery that config translation does now), but we are in the middle of exploring this right here.

@Crell: Now, any better ideas? :)

Well, I don't think anyone is trying to argue that in an ideal world this wouldn't need to exist, but not providing this means we need to figure out some other way for config_translation to do its thing. @Crell, do you have any ideas for that?

Would be great to get @effulgentsia and @alex.pott in here, as well, as the config_translation issue was discussed with them in Prague.

Oops, that was a crosspost. What @Gábor Hojtsy said... :-)

Well, instantiating all forms in the system sounds like a terrible idea on its own. :-)

If you want to apply logic to all routes, the time/place to do so is with a route_alter event. That gets called multiple times on route build, once for each route set (module) with just the routes from that one set/module. We already do that for a number of things; that's how, for instance, access checks work because we introspect every route and then attach options to track which checkers will apply later on routing. It's still only done on one route set at a time. That sounds very similar to what you describe, and you still get the opportunity to investigate every route; just one set at a time.

In that alter, you could check for any route that specifies _form, then check that class to see if it has the interface on it you want. (That's how ContainerInjectionInterface works.) Instantiating each one of them, though, seems like a very expensive and complicated thing to do, especially since some of them will need the container in order to do so. The list of "config IDs that I'm going to futz with" sounds like something better described via an annotation or a static method.

@Crell, nice, I have been investigating something like you propose in the meantime. I'll ping you once I have a patch over there for you to review.

In that alter, you could check for any route that specifies _form, then check that class to see if it has the interface on it you want. (That's how ContainerInjectionInterface works.) Instantiating each one of them, though, seems like a very expensive and complicated thing to do, especially since some of them will need the container in order to do so. The list of "config IDs that I'm going to futz with" sounds like something better described via an annotation or a static method.

It's not possible to find out the interface directly, as we also support service IDs as '_form' declaration. See https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Controller!HtmlFo...
Also, just like getFormId() we have already come across dynamic getConfigNames() in this patch, so an annotation or static method is not possible. So the instantiation still needs to happen.

Yeah, again, this is not our idea. Considering most if not all admin forms will work with config files, instantiating all of them is not really something we can escape in this scheme. I pinged this issue to @effulgentsia and @alexpott on IRC who are the masterminds behind this plan. This is just on the way to get config_translation module into core, so that is the extent we are focused on this :) In fact as far as core forms go, the use case for routes vs. forms vs. CMI keys is only relevant for 4(!) routes in total, so this is indeed a lot of overhead to build for that. This is all the data we need ATM from core: http://drupalcode.org/project/config_translation.git/blob/refs/heads/8.x... I agree building this whole system for that only is overkill, but the idea is if this larger system is feasible then it may be useful for other things as well. The config translation use case reaches as far as these 4 routes relating to those 5 cmi keys :D

Again, what Gabor said.

To re-iterate a bit on what I said above, why we cannot do a static method, or an Annotation. Again, this is very much like the getFormId() method we have currently. When talking with @effulgentsia and @tim.plunkett about this, we at first thought that getFormId() could/should be static as well. But then we found EntityFormController::getFormId(). And similarly, we have found a dynamic use case for getConfigNames(). The aggregator settings form includes the settings forms of its processors plugins as part of the main form. But unlike e.g. block settings forms, each of the processors saves its configuration into a separate configuration file. Therefore to return an accurate list of configuration names for the main aggregator form, getConfigNames() needs to loop over the contained plugins and ask each one about its configuration names.

Also (although we're currently not doing (yet) doing that in the patch in #2095289: Make getEditableConfigNames() public and use it in config_translation) we want to support the use-case of modules altering config forms with settings form additional config files. So the getConfigNames() may end up needing a $module_handler->alter() which is also not cleanly possible from a static method.

In terms of a generic @Form (or even @ConfigForm) annotation (without the config names part), this would *immensely* help the discovery, for obvious reasons. That would alleviate the need for the route futzing we currently have to do. @effulgentsia in fact proposed this from the start, we just didn't pursue this because we didn't think it was viable for D8 at this point. If we can get some traction behind that idea, that would be awesome, and - while not solving *all* of our problems (again, we still need to instantiate the forms :-() - would remove a lot of the ickyness.

We'll need to match up routes with those form classes either way, so even if the forms have an annotation on them, we need to figure out which route they are registered with.

Well, then the only viable approach is to have an extra interface for forms that can be instanceof-checked, and processing a route in the the existing build/alter process like everyone else. A "get all the routes" method is still an absolute non-starter.

I am wondering whether we could just provide some magic php object which works like all routes but just allows iteration using some special interfaces.
This would not load all the routes into memory.

This is related to #2106811: Create route discoverability. There's going to be cases where people want to consult the routes - whether via development or for UIs (I could see someone writing a route UI that lets you move routes around as well then does that in an event listener on rebuild).

I'm not sure an alter event is acceptable here - if you want to know the final state of the routes, then you'd be in an arms race with listener priority since you're never guaranteed that they won't change after you run. When you don't even want to change anything that's a bit messy. Depends what the use-case is really.

Storage that can't list all routes, while it's possible to have that, I can't see that being particularly useful (Redis certainly can handle listing everything). It's only really memcache and APC that can't handle listAll() and neither of those are going to be used for primary storage. Storage that can't do it can either return nothing or throw an exception on that method.

An iterator sounds like a good idea for this - if you're going to foreach over routes, not doing something like array_search() that's plenty and it solves the memory issues. It's still potentially a lot of data to crunch through, so shouldn't be encouraged. Also baking in those limitations early would prevent some of the gymnastics I had to do with CacheArray in Drupal 7 - much better to start with an actual object than have to backport an iterator in later.

... *censored*.

I still hold that for what Gabor is trying to do getAllRoutes() is the Wrong Way(tm) to do it. However, I think catch is right that for debugging purposes we'd need that sort of introspection.

For that, a lazy-loading RouteCollection subclass seems like the least likely to blow up memory. (It may blow up SQL hits if it requests one route at a time, but that seems more tolerable for a debug/devel feature.)

Here is a first version.

1. +++ b/core/lib/Drupal/Core/Routing/LazyLoadingRouteCollection.php
   @@ -0,0 +1,132 @@
   +    $this->elements = $routes;
   

   If the whole idea is to save memory, I think we should do a $this->elements = array() at the top of the function. Then we never have two sets of routes in memory.

2. +++ b/core/tests/Drupal/Tests/Core/Routing/LazyLoadingRouteCollection.php
   @@ -0,0 +1,113 @@
   +class LazyLoadingRouteCollectionTest extends UnitTestCase {
   ...
   +class TestRouteCollection extends LazyLoadingRouteCollection {
   

   Is this some sort of standard to include multiple classes in test files? Seems a bit strange to me, TBH.

EDIT: Fix typo.

If the whole idea is to save memory, I think we should do a $this->elements = array() at the top of the function. Then we never have two sets of routes in memory.

Really good point!!

Is this some sort of standard to include multiple classes in test files? Seems a bit strange to me, TBH.

keeping test only classes in the same file feels okay given that you have one single context to read what is going on.

This seemed to be a random test failure.

+++ b/core/lib/Drupal/Core/Routing/LazyLoadingRouteCollection.php
@@ -0,0 +1,134 @@
+class LazyLoadingRouteCollection implements Iterator {

This needs to be an interface, and an object returned BY a RouteProvider. We need the MongoDB version to work and be straightforward to write.

This needs to be an interface, and an object returned BY a RouteProvider

Haha I totally forgot that bit, come this is just a detail :P

We need the MongoDB version to work and be straightforward to write.

Well isn't Iterable some kind of interface?

Yes Iterator's enough of an interface for me - if we have to add an extra method at some point it will likely be another built in PHP interface, if it's not then we can add it then.

Hm, I'm fine with pure Iterator. It means forward-looking contrib can just return a generator in PHP 5.5, which is all we need. :-)

We still need to actually *use* the class, though.

Here is the patch which was supposed to be uploaded in #34

The last patch included some other patches.

Looks great!

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MockRouteProvider.php
@@ -75,5 +75,9 @@ public function getRoutesByPattern($pattern) {
+    // TODO: Implement getAllRoutes() method.

Is this meant to be implemented in this issue before commit? If it's here to stay it should be "@todo"

+++ b/core/lib/Drupal/Core/Routing/RouteProviderInterface.php
@@ -28,4 +28,16 @@
+   *
+   * @return \Symfony\Component\Routing\Route[]
+   *   An array of routes keyed by route name.

No, not an array. An iterable. Iterables are not, sadly, isomorphic to arrays. All you can do with them is foreach(); you can't do any other array-ish behavior.

@crell
Do you think keeping the "[]" syntax is okay? Sadly I could not find anything in the upcoming PSR-5 proposal about that.

I don't think that's part of any existing standard, but I've seen it used. I've no problem with it, but we should bring it up with the PSR-5 folks.

1. +++ b/core/lib/Drupal/Core/Routing/LazyLoadingRouteCollection.php
   @@ -0,0 +1,134 @@
   +    $query = $this->database->select('router');
   

   Anywhere else we have a reference to the router table, we pass the table name in via the constructor. We should do that here, too, and then pass the table name in from the provider (which will have it).

2. +++ b/core/lib/Drupal/Core/Routing/LazyLoadingRouteCollection.php
   @@ -0,0 +1,134 @@
   +  public function count() {
   +    return (int) $this->database->select('router')->countQuery()->execute();
   +  }
   

   We should probably cache this to an object property.

RouteProvider also needs to be updated to pass in its table name.

Done.

OK, I think we're set here. Thanks! (Let's see if I know how to use the new d.o...)

47: 2098197-47.patch queued for re-testing.

47: 2098197-47.patch queued for re-testing.

47: 2098197-47.patch queued for re-testing.

47: 2098197-47.patch queued for re-testing.

Hm. Not totally sure on this, but I guess Crell approves and it sounds like alexpott and catch did at one point, too.

Very well, committed and pushed to 8.x. Thanks!