Routes added in RouteSubscribers cannot be altered

Well, "No freaking idea:" is thrown via one of the tests :/

Will have a shot at this tonight.

Btw, only one of the test-case-fails is legit: ConfigTranslationListUiTest, i.e. the one in #4. One seems random and the others are just PHPUnit-Mocking expectations that need to be updated due to the ConfigNamesMapper change.

Haven't grokked the patch 100% yet, but it seems like the right approach to split this is out into two steps (i.e. exactly what this patch does):
a) gather the information that is needed during building
b) add additional routes in alter

We should expose the current bug, somehow.

Well, it would help to actually describe the bug then.

OK, so before I'm actually able to work on this I have to let off a bit of steam. I do not mean to rain on anyone's parade, but this has to be said:

1. This issue apparently originated in #2145041: Allow dynamic routes to be defined via a callback. Because I was a bit pissed already after reading this issue I needed to do something else and by chance checked the commit log where I found that issue. It would have been *incredibly* helpful to link that issue and reference it in the issue summary.

2. This is being classified as a major bug, without actually describing anything that is broken. Let alone anything that would qualify as major. Something being "fragile", whatever that means, but working fine does not qualify as major. Regardless of whether there is a bug or not, this is incredibly frustrating. Due to config_translation being an exception in being committed so late in the cycle, it causing major bugs is a pretty big deal, so if it actually does that should be well justified.

3. Regarding the actual "bug". It is quite insulting* to read that the code you wrote works "by accidence" (see #2145041: Allow dynamic routes to be defined via a callback). The previous code in config_translation's RouteSubscriber (previous as in before #2145041: Allow dynamic routes to be defined via a callback) worked because dynamic route events got fired after the static ones were all done. That is or was not an accident, but quite simply how the code worked. Using the alter event has the problem that it makes it harder for others to alter config_translation's routes. So claiming that using the alter is more robust or anything of the like is a falsehood.

4. Claiming that the config mappers "bypass the best practices of both the plugin and routing system" *without actually explaining what you mean by that is quite simply FUD and, again,
insulting*. I'm not saying that that's not true, I am always eager to learn of better implementations. But if you want to criticize then please actually do so and don't just claim it's bad code.

* I realized after reading this again that this might sound as though I'm insulted because it's my module or I wrote most of the code or something. Neither is true, of course and I am well aware of that. I did write a significant part of the code, however, and was very involved in the architectural design.

From #2145041-51: Allow dynamic routes to be defined via a callback

The real problem here is config_translation.

\Drupal\config_translation\Routing\RouteSubscriber calls ConfigMapperManager::getMappers().
ConfigMapperManager::getMappers() instantiates all of the mappers.
Each mapper gets the route provider *that is still being built* and tries to inspect it for other routes.
Each plugin then stores the route object it will base its routes on, even though it only ever calls getPath().

This only works in HEAD (and in the above patch) because this subscriber *happens* to run after all of the static routes.
Now that there is no final stage, it is supposed to run for each provider and inspect those routes only.

You cannot use the router provider while the routes are being rebuilt.

One potential bug is if the routes that config_translation is expecting are in the final collection being passed, they will never be in the route provider, and never found. These alters *must* only use the collection they are passed.

This issue should have been filed with more context from the other issue, that's partly my fault. It was about 1:30am for me when this was all being discussed initially.

I completely fail to see why you say that

These alters *must* only use the collection they are passed.

But I completely agree that they *should* anyway. I just don't see why that's such a big deal. I also still disagree with this being major, btw.

So let's just leave the differences behind (I'll defer and leave this at major) and focus on the fact that we agree on where we want to go.

I'm working on this now, I guess this needs a re-roll now first.

Here we go. Since current HEAD now contains the @todo to fix this and this is still a WIP resolving the conflict was not easily possible, but here's a best effort so we can improve on this.

Will now actually step through the code.

OK, so I did manage to botch up the rebase. This should be better.

Dammit, rebasing is hard... Sorry for the noise.

So I already added a comment to the priority change. I also added a comment where we build up the list of base route names, feel free to improve on that or suggest more comments elsewhere.

I also
- added a couple other comments elsewhere.
- renamed a couple variables
- simplified the logic a bit. Now that we don't have a 'dynamic' step anymore there is no point in splitting up the logic in two steps. (In theory I should have probably done this already as part of the rebase.)

Have not looked at the tests yet, doing that now. Should be ready for final-ish review, unless the tests reveal some deeper issue.

So this fixes the unit tests at least. Running out of steam for now. Will pick this up tomorrow. Unassigning for now, though, as if someone wants to fix the test fail before I do it, knock yourself out. :-)

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
@@ -30,27 +38,49 @@ class RouteSubscriber extends RouteSubscriberBase {
+    }
...
+      throw new \Exception('Well, if @dawehner has NFI, what is @tstoeckler supposed to say?!');
...
+    if (!empty($this->baseRouteNames)) {

I just realized that this might not be clear: In terms of "final-ish" review this part should be ignored. Once we figure out why this is being hit in the tests, we should fix the cause(s) and then eventually either remove this exception or make it more meaningful.

Alright this should get us a bit closer.
There was a flaw in the exception throwing in the previous patch. It was being thrown on each invocation of the alter, i.e. for every $provider. But the routes get build up consecutively, so we should only check on the very last run ($provider == 'dynamic_routes') whether there are any left and only then throw an exception.

Also because after I made it more meaningful the exception was actually quite helpful in debugging I vote for keeping it. If we do remove the 'dynamic_routes' invocation per the current code, then we'll have to figure something else out (or remove the exception then).

On thing that still doesn't work though is field UI routes. They are defined in a RouteSubscriber during the alter event themselves, so they never get altered themselves, so config_translation can never process them. This is sort a chicken-and-egg problem, that we were previously working around with the DYNAMIC_ROUTES event. I have something in mind that might or might not work, we'll see. Might be the weekend before I post an update, though.

So this works at least with some (limited) manual testing.

As described in #26 the problem was that routes added in an alter event cannot be altered themselves because they do not have their own alter event.

This patch does just that: It invokes a separate alter event for those added routes. This needs to be done manually by those RouteSubscribers that need it, although I've provided a helper in RouteSubscriberBase and tried to document the constraints. It's not the prettiest code I've ever written, but I think it's OK. Suggestions for improvement (of course!!!) welcome!

Let's see where we're at.

Also: I'm not going to do it but I think the "Needs tests" tag should be removed. There really is nothing to test. What we could test is that routes added an alter event are currently not altered (i.e. see above) but that is not related to this issue title.

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
@@ -30,27 +38,69 @@ class RouteSubscriber extends RouteSubscriberBase {
+  }
+
...
+    return $events;
...
+    $events[RoutingEvents::ALTER] = array('onAlterRoutes', -1000);
...
+    // needs to be lower than that.
...
+    // routes for which we need to generate translation routes so this priority
...
+    // so all route subscribers that add routes need to run before this one.
...
+    // This route subscriber adds routes based on the existence of other routes,
...
+    $events = parent::getSubscribedEvents();
...
+   */
...
+   * {@inheritdoc}
...
+  /**
...
+  public static function getSubscribedEvents() {
...
+    // Field UI's route subscriber, for example, has priority -100, but provides

This *should* no longer be necessary. As explained above, the order of the alters is irrelevant. It's the altering of the alters that's important. Let's get to green first, though :-)

Yay, that fail looks manageable, at least. Will hopefully finish this on the weekend (if no one beats me to it), but, can't guarantee 100%.

Here we go, let's see if this works.

The problem was the following: config translation's test theme tries to provide a config mapper for the system performance page, and then there are tests to assert that that works. The mapper was registered but the corresponding local task was never found. That is because there is no base local task for the performance page and therefore LocalTaskDerivativeBase::getPluginIdFromRoute() cannot find the proper tab_root_id and, thus, no local task.

I have no idea why this used to work, but I couldn't be bothered to investigate. It's broken either way.

So #31 was at least partly incorrect. I still think it makes sense to provide the local task at the very least for manual testing. The test, however, does not check for the 'Translate' tab but drupalGet()s the translation page directly. I'm not sure why that fails in the test. When manually enabling the test theme the translation page works fine.

Will hopefully finish this before New Year's.

Re #34: As explained in in #33 the local task is in fact not the problem unlike I asserted in #31.

I'm having a really hard time debugging this one. Something is going very wrong with the route rebuilding in the test, as no dynamic config translation routes are present. I.e. even the system.module ones (translating the maintenance mode page, for example) aren't in {router} even though they should always be there as soon as you turn on config_translation. Why that is, I have not found out yet.

This should pass.

See the comment in RouteSubscriber for what was going wrong.

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
@@ -30,27 +39,87 @@ class RouteSubscriber extends RouteSubscriberBase {
+    // configuration mappers are available. As the instance of this class is
+    // statically cached as a service in the dependency injection container, it
+    // does not get re-instantiated during subsequent route rebuilding
+    // processes. Therefore this information cannot be gathered in the

should be "... during subsequent route rebuilding processes when more configuration mapper information is available".

Also noticed that my comment there is partially incorrect as $this->mappers only ever gets set in the constructor. So it's only the call to getBaseRouteName() which gets deferred to each route rebuilding process. That directly accesses the stored plugin definition, so it really shouldn't make any difference. Hmm...

Thanks for the review, that is very helpful!

This fixes 3. and 4.

Regarding 2:
Instead of adding a @todo to figure it out, I figured it out :-) I am a bit embarassed that I didn't realize this earlier but here's what happened:

Why this only cropped up in the test:
In regular Drupal runtime route rebuilding only ever happens once per request so the following was not noticeable in any other tests. In the test, route rebuilding happens twice in the same request: Once for the test setup and once manually in the test after the theme has been enabled.

Now to the actual problem:
In config_translation's RouteSubsriber I was setting up $this->baseRouteNames in the constructor and then consecutively unsetting the respective route names until $this->baseRouteNames is empty at the end of route rebuilding. Now comes the problem: When the second route rebuilding happens $this->baseRouteNames is still empty (because it's the same instance of the RouteSubscriber) and doesn't get re-initialized. So the RouteSubscriber iterates over an empty array and no config_translation routes get added. D'uh!!!!!

So I fixed that in this patch by adding a $this->baseRouteNamesToProcess which keeps track of the route names during a single route rebuilding process and gets re-initialized at the beginning of the next one. It's basically the same as #36, but should be much clearer I think.

Regarding 1.:
This is certainly possible, but the question is what should $provider be? I will upload a patch in the next comment, which sets $provider to the class name of the route subscriber which provided the routes. While I think this makes some amount of sense, it is not consistent with the other usages of $provider which are either $module_name or 'routes_dynamic'. Therefore I wanted to roll that in a separate patch.

Here's that patch.

40: 2158571-40.patch queued for re-testing.

Fail with "Allowed memory size of 268435456 bytes exhausted (tried to allocate 268435456 bytes)" in file download test was not real / related :)

First of all thanks for taking this one! Second: looks good overall! Some notes:

1. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
   @@ -91,14 +91,13 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   +    $this->routeProvider = $route_provider;
   

   But there is no documented routeProvider property (yet)?

2. +++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
   @@ -43,8 +90,25 @@ public static function getSubscribedEvents() {
   +    if ($provider != get_class($this)) {
   +      $this->alterRoutes($collection, $provider);
   +    }
   

   While this is a bit twisted, it is also pretty clever.

3. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
   @@ -17,11 +19,29 @@
   +   * A mapping of configuration mappers to their base route name.
   

   Document that this is keyed by mapper id.

4. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
   @@ -658,9 +658,8 @@ public function testAlterInfo() {
   -    theme_enable(array($theme));
   +    \Drupal::service('theme_handler')->enable(array($theme));
        \Drupal::service('router.builder')->rebuild();
   -    menu_router_rebuild();
   

   I don't think this is related? (There are also other theme_*() functions used, eg. theme_list() which can be done away). We may want to have an issue to remove these instead?

5. +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigNamesMapperTest.php
   @@ -139,8 +133,26 @@ public function testGetBaseRouteParameters() {
   +   * Also tests ConfigNamesMapper::setBaseRoute().
   ...
   +   *
   ...
       * Tests ConfigNamesMapper::getBaseRoute().
   

   What about "Tests ConfigNamesMapper::getBaseRoute() and setBaseRoute()" instead? Short and sweet :)

6. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
   @@ -48,6 +48,9 @@ protected function alterRoutes(RouteCollection $collection, $provider) {
   +        // Create an additional route collection for the field UI routes.
   +        $additional_collection = new RouteCollection();
   +
   

   Is this due to the same bug in field UI or just due to the need for our shifted state of processing to allow further altering.

   As for docs purposes how do we explain when to use this further altering way and not to use (as other route subscribers like views are not doing it).

Also thanks for the review!
1. Will fix that in the next re-roll
2. Thanks. Do you think that needs better comments? Also is that an endorsement of #40 over #39?
3. Will fix that in the next re-roll
4. Yeah I was poking in the dark in that test method for a while so I fixed that at some point. I can remove it if you want and open a follow-up to fix that more generally.
5. Yeah, that works for me. If you include the class name both times you get over 80 chars, which is why I did it the way I did.
6. Here the story:
- In current HEAD routes that are *added* by RouteSubscriber's cannot be *altered* by other RouteSubscribers.
- Since config_translation needs to *alter* the field_ui routes, it currently performs the hack that is the reason for this issue.
- The invokeAdditionalAlter() method introduces a clean(ish?) way of doing that additional alter. Field UI using that method allows config_translation to pick up the respective routes and to add the proper translation routes.
- Config translation using that method is only done for consistency and not strictly necessary to fix the test fails.
- However, we should establish the following standard: If you *add* routes in your RouteSubscriber you should call invokeAdditionalAlter(), if you only *alter* existing routes, you do not need to call that method. Views' RouteSubsciber falls in the latter category, for example.
- I tried to explain the previous point in the docs for invokeAdditionalAlter(). Do you think those could be improved?

Edit: Fix wrong comment link.
Edit2: Again

Re

2: I think the comments are good there. Indeed, the change form #39 to #40 is a huge step forward. Much cleaner to automate it like that :)

4: I think since others need to change too to remove the theme_* stuff, I think a separate normal issue would be good. It will be great cleanup, lots of code removed :)

6. I think this should be added in the issue summary as the result of this patch. Also if Views does not do this yet, we should add it there too, no? Looks like it would be easy. I think understanding this patch after the fact would also be easier if the solution would be consistently applied to all places where logical.

2. Cool.
4. Agreed.
6. Like I tried to explain in #45 views does not *add* routes it only alters existing routes, so no need to call invokAdditionalAlter(). As always, I'm open to improving documentation if that's needed.

Regarding the issue summary: That definitely needs to be updated, I also tagged it that way. I'm not going to be the one to update that however, because I disagree with the whole premise of why this issue was opened. If this issue were re-titled as "Routes added in RouteSubscriber's cannot be altered" I would gladly rewrite the issue summary, otherwise @dawehner or @tim.plunkett will have to do that.

Here we go. Someone want to quickly re-RTBC?

Re #48.1: I was halfway into implementing that but in the end it seemed more complicated than I had thought. Because we don't want to call the alter event on routes that have already been altered, we would have to specifically pick out the ones that were added and make a new route collection out of that. So we basically have to re-compute knowledge that the route subscriber already has, which is not ideal IMO. Anyway, let's stick with the current approach for now.

Yay! Also thanks for the issue summary update. @dawehner++

In reviewing this again myself I found a small optimization: We currently invoke the additional alter for each set of field UI routes instead of once for all field UI routes.

This fixes that. Pretty trivial change, but setting back to needs review.

Ahh, sorry. Forgot to rebase my branch before rolling the patch, so the last patch reverted the last couple days of core commits. Here is the correct patch. The interdiff is correct, though.

The change looks good, and @dawehner agreed before that it was RTBC. Back there :) Also moving to the right component then according to title :)

So I don't know what I was smoking in #45 / #47 but @Gábor Hojtsy's doubts were well justified: Views' RouteSubscriber does in fact *add* routes to the collection. See PathPluginBase::alterRoutes().

Making those routes alterable in the manner introduced here, however, could prove to be a little more complicated than the field UI / config translation use-case due to the additional indirection of Views' display plugins. So I would personally recommend handling that in a follow-up / separate issue (which I will promise to work on).

So I should have looked a bit further up than the $collection->add() because a couple lines before that there's a $collection->remove(). Views basically takes an existing route name and replaces the given route with a completely new one, yet retaining the route name. One could argue that it would make sense to allow that to be altered as well, but that's a whole different issue. These routes are being altered, not added.

Ugh. Cascading alters is pretty ugly. I'd like to dig in a bit more to see if there's another option.

@effulgentsia: the question then is how models like:

- these are the entity routes
- this entity has fields, it needs field routes
- oh, field routes, these need config translation routes on them

would work.

Sorry, English pedantry. Ignore me. :)

@effulgentsia: any bright ideas?

Just wanted to point that as discussed above, we could hide away the additional alter from RouteSubscribers if we simply compare the route names before and after alterRoutes() in RouteSubscriberBase and then invoke the additional alter there. That would not change anything conceptually, though, of course.

What about adding a RoutingEvents::CONDITIONAL event (or better name if you can think of one), and having RouteBuilder dispatch it prior to dispatching RoutingEvents::ALTER? Then any module that wants to add a new route based on information about some other route does so in the CONDITIONAL event. And any module that wants to alter a route does so in the ALTER event.

And for #63, we use priorities, so config_translation would register its CONDITIONAL subscriber at a lower priority (i.e., run later than) field_ui's CONDITIONAL subscriber.

I personally find weight futzing a lot less clean than explicitly invoking an alter when it's needed, but if people agree on #67 I can live with it and would write it.

Ah, right, #69 is a good point: conditional routes need to be based on the post-altered information of the routes they depend on. So actually, #55 doesn't handle that, since a high priority subscriber would create the new route based on only a partially altered route that it depends on. I guess you could say that that's ok, because a low priority subscriber could implement the necessary alter on the conditional route to get it into a matched state, but that seems brittle.

So, what if we did the 2 events idea, but in the opposite order: RouteBuilder first calls ALTER, then CONDITIONAL. Then, after it has collected all the conditional routes, it calls ALTER on that set, and then CONDITIONAL, and keeps doing that until 0 routes get returned in the CONDITIONAL event. It's essentially the same concept as #55, but moving the responsibility of invoking ALTER on everything (and only once on any given route) from the subscribers to the builder. I think it also helps clarify the responsibilities to subscribers better by having 2 distinct events rather than trying to do everything inside one and dealing with reentrancy issues.

We could certainly do that, but I don't understand why we need the separate event then. If this is just about not having to invoke the alter manually, then why don't we go with #66 / #48.1 ?

To reiterate, the idea would be the following: Before invoking the alter event, we fetch the list of route names and then the same after the alter event has run. Then we diff them and if there are new ones, we invoke the additional alter.

I personally would find that clearer than a CONDITIONAL event which is not very self-documenting IMO.

Well, I think there's a pretty important conceptual difference between creating new routes and altering existing ones, and that it's good to express that difference with different events, but I'll admit to that being subjective.

On a technical front, I think it helps ensure the order of operations better. It means that if you're creating a new route, then you're basing its information on an existing route that has already been fully altered (both lower and higher priority ALTER subscribers have run, because you're in a new event rather than competing in the same event). And it also means that for the new route created, each other ALTER subscriber runs only once on it, rather than having lower priority subscribers run once within the same ALTER invocation, and then again due to the second one.

And it also means that for the new route created, each other ALTER subscriber runs only once on it, rather than having lower priority subscribers run once within the same ALTER invocation, and then again due to the second one.

If you're claiming that with he current patch route subscribers run twice on the same route collection that's not true as far as I'm aware.

Anyway, @dawehner and @effulgentsia endorsing something works for me, will try to code something up on the weekend latest if no one beats me to it.

The idea of deliberately building recursion into the route definition pipeline fills me with dread. I can very easily see it as a way to leave your site in an unrecoverable state. However, I cannot see an alternative other than "add lots of steps and hope it's enough", which is not an alternative I support either.

So, let's see if we can make it work but let's also be extremely careful about error handling, and recursion in the call stack. We need to ensure that one stray syntax error in one module doesn't cause the whole site to die. (Which can very easily happen with a number of hooks today in D7.)

Hmm... so I guess I'll hold off implementing this if there's not an agreement yet.

Since there's really no way of manipulating the output of get_class($this) I think in terms of infinite recursion we're good here. I am obviously welcome to more reviews of that part (and in general) but, ...

If you have a RouteSubscriber base class with multiple child subscribers that each call the parents alterRoutes() that would get recursed into a couple times but a) not infinitely, just once per child, and b) that's a pretty
contrived use-case to begin with as the altering would be duplicated even without this patch.

I didn't mean don't try. I just mean "GAH! Be very very careful! Digging that deep may unearth a Balrog."

Just a reroll of #55 for HEAD drift while we wait for a new patch per #74.

@dawehner and @effulgentsia endorsing something works for me, will try to code something up on the weekend latest if no one beats me to it.

Thanks! I appreciate the trust.

If you're claiming that with he current patch route subscribers run twice on the same route collection that's not true as far as I'm aware.

The field_ui ALTER subscriber calls this:

$this->invokeAdditionalAlter($additional_collection);
$collection->addCollection($additional_collection);

Which means suppose some other subscriber (e.g., SpecialAttributesRouteSubscriber) has a lower priority than field_ui. Then, it runs on $additional_collection per the code above, and then on the modified $collection once field_ui is done, due to being lower priority. So for any route in $additional_collection, it does its alter logic twice. That would be solved by #70.

So, let's see if we can make it work but let's also be extremely careful about error handling, and recursion in the call stack. We need to ensure that one stray syntax error in one module doesn't cause the whole site to die.

Agreed, and I think the proposal in #70 would achieve that, because all the complicated stuff would be centralized in RouteBuilder. Subscribers would only be responsible for fulfilling their contract (alter routes in ALTER, add new routes in CONDITIONAL), and RouteBuilder would ensure that things get called in the correct order and that any give route is only passed to each step once.

Therefore, it wouldn't even be a recursive algorithm any more, but it would be a potentially infinite loop. For example, if module A defines route A, then module B defines a conditional route B1 based on A, then module C defines a conditional route C1 based on B1, then module B defines a conditional route B2 based on C1, module C defines a conditional route C2 based on B2, and B and C keep ping-ponging like that. An example might be something like if module B was something that added Field UI routes to every route that's a form (e.g., Entityforms gone crazy), and module C added a config translation route to those Field UI routes, but then those config translation routes were themselves treated as forms that module B needed to add additional Field UI routes to. This would be faulty logic on the part of either B or C (e.g., it would be B's responsibility to add code to not treat the config translation form of a Field UI form as something that needs to be separately fieldable). But, if we want to add some protection against faulty modules like this taking down the site, we could add something to RouteBuilder to break out of the loop after a certain iteration length (e.g., 9) just like we arbitrarily limit menu links to not have a tree depth >9.

So due to #75 I did not write a new patch since that made me feel like there was still consensus missing. Maybe I misread that comment.

Then, it runs on $additional_collection per the code above, and then on the modified $collection once field_ui is done, due to being lower priority.

You are, of course, completely correct. Thanks for pointing that out! I hadn't thought of that.

I personally don't feel like we should be doing anything against such infinite looping but I wouldn't block it if other people are adamant about it.

So I'll try to code something up here in the next few days, but feel free to beat me to it.

I am very adamant that we should "fail gracefully" here. This process smells like a very easy "break all the things" location. We need to ensure that one badly written module can't break the whole site, which means testing that's the case.

Re Crell: Sorry but I completely fail to see your point. A badly written module can break the entire site in any which way it wants to. That includes the routing system, but also just about everything else. I can write an entity storage controller, that accidentally deletes from {system} or I can accidentally delete used files from the files directory. Can you elaborate what is special about route rebuilding?

There were a couple of hooks in Drupal 7, and even now in D8 I think, where if you return NULL instead of an array (an easy accident to make) your entire site would WSOD with no indication as to why. You had to sort of guess which hook you screwed up.

In D7, if you move a module from one directory to another and don't have just the right page loaded as an admin before doing so, you can end up with a broken registry. The drush rr command was written to externalize the rebuild process because otherwise Drupal itself could not be recovered.

That's the kind of problems I'm worried about here. A situation where one module does something dumb, and as a result you have an empty router table. (Or a router table missing large swaths of important paths.) DB transactions only partially help here, and since we allow non-SQL backends for routing we can't rely on that anyway. Hence my "be careful!" warnings.

Here's a stab at implementing @effulgentsia's #70.

The logic in RouteBuilder is not exactly convoluted but it's not trivial either, so please give that extra scrutiny. For DX I chose to let RouteSubcriber's return the conditional routes in conditionalRoutes(), that's of course arbitrary and we could pass in the additional collection, just as well. I just think that makes it more clear than having two different route collections to operate on. We *could* also have them operate on just one route collection, and then extract the added routes in RouteSubscriberBase, but I would consider that overarchitecting.

I did not run any tests on this, but should be good for human reviews on the approach nonetheless.

Also: I originally wanted to improve the patch in #78 to not require RouteSubscriber's to call invokeAdditionalAlter() manually but figure that out dynamically, but that basically led me in the same direction as this patch. At the very least the second route collection in RouteBuilder would be needed then just like in the patch.

Wow, that was fairly hard to debug for such an innocuous error. Weird that PhpStorm didn't complain.

This should at least install.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterKernelListenersPass.php
@@ -22,8 +22,14 @@ public function process(ContainerBuilder $container) {
+      }
...
+        $GLOBALS['foo'] = TRUE;
...
+        new \ReflectionClass('Drupal\views\EventSubscriber\RouteSubscriber');
...
+        $foo = class_exists('Drupal\views\EventSubscriber\RouteSubscriber');
...
+
...
+        $a='a';
...
+      if (!isset($GLOBALS['foo'])) {
...
-

Sometimes I fail at life...

Interdiff was correct (minus 2 minor whitespace changes which I'm sweeping under the rug).

Next attempt.

89: 2158571-88-route-subscriber-conditional.patch queued for re-testing.

Mark Sonnabaum, Tim Plunkett, Daniel Wehner, and I just chewed through this issue in IRC. Our basic conclusion:

1) Dynamic routes based on dynamic routes based on dynamic routes is too edge-casey to try supporting as a first-class operation.
2) The use case at hand is specifically build off of entities. So let entity and/or field modules define their own event in a dynamic routes callback that other modules can respond to specifically. The likelihood of other modules needing to do that often enough that we *need* an infinitely recursive universal generative approach in core is really really low; and if we're proven wrong, we can always fix that in 8.2 or 8.3. That's what minor releases are for.

As for what to do with the dumping process, it hinges on whether or not we keep the chunked-by-module dumping. We don't currently have benchmark data either way to say how much that helps memory. Therefore, I am volunteering Mark to get hard data on how much (if it all) that helps. :-)

Once we know if the chunked dumping generates worthwhile benefit (for some definition of worthwhile), we have two options:

IF (chunking is worthwhile) {
  Foreach module: 
    1) collect routes (YAML and dynamic); 
    2) pass to a DYNAMIC event (new routes allowed); 
    3) Pass to an FINALIZE event (no new routes allowed);
    4) Dump; 
  Then call a single FINISHED event with no data (as we do now);
}
ELSE (chunking is not worthwhile) {
  1) Collect all routes (YAML and dynamic), from all modules; 
  2) Pass all routes to DYNAMIC (new routes allowed); 
  3) Pass all routes to FINALIZE (no new routes allowed, but editing is fine); 
  4) Dump.
}

We won't try to hard-enforce "no new route allowed", but document it as such. In either case, this gives us 2 passes, and modules can safely rely on the second pass (FINALIZE) to do global-enhancement like setting param converters and type information. (Safely in the sense that any module that adds more routes in FINALIZE is officially Doing It Wrong(tm), and that's where the bug gets filed.) We then don't have an "ALTER" event anymore as it gets split into two.

Advantages:
1) We know what the memory implications are, finally.
2) There's a clear separation between dynamic routes and futzing with someone else's routes.
3) It's a fixed number of known events, so the logic is straightforward and the opportunity for weirdness limited.
4) More complex use cases can solve themselves without burdening core with too many "What if?"s.
5) We can iterate. We have the technology...

Thoughts?

So, I don't necessarily object to the decision here but I do consider it to be a little short sighted. We already have "dynamic routes based on dynamic routes" (== 2 hops) in core (i.e. config translation routes for field ui routes), so as soon as someone has a use-case in contrib for adding routes dynamically based on config translation routes we'll have 3 hops. So at least 3 or maybe 4 hops is not at all far-fetched and at that point I think having 2 or 3 different events which are invoked by arbitrary modules and listened to by other arbitrary modules and all that in the process of another event (namely RoutingEvents::DYNAMIC) while all those events in fact also have a dependency on each other (i.e. it's crucial that the field UI routes exist before config translation comes in) gets incredibly messy and near impossible to debug if something goes wrong. So while I'm not necessarily a fan of the recursiveness of the previous patch the code that can go wrong is all in one place, which IMO will make it much more maintainable in the long run.

In any case, I'm unassigning this from me. I'm not pissed in any way (anymore, see above :-)) but I just cannot be bothered to rewrite the patch for the n-th time. I originally got sucked into this due to the config_translation stuff, but that hasn't changed in the 2-3 last rewrites, this is all routing system now. So have fun! :-)

1) collect routes (YAML and dynamic);
2) pass to a DYNAMIC event (new routes allowed);
3) Pass to an FINALIZE event (no new routes allowed);

Am I correct in understanding that step 2 gets passed the routes collected in step 1? It would need to, since the whole use case of 2) is new routes that depend on other routes. Because for independent routes, the "dynamic" portion of step 1) is sufficient. However, if this understanding is correct, then step 2) actually needs to be passed the finalized routes. I.e., if a dynamic route B needs to be created based on route A, and some module out there needs to alter the path of route A, then the code that's creating route B needs to be passed route A in its finalized state.

But then, the newly created route B needs to be passed to a FINALIZE event. Which gets us to a loop=1.5 (in the sense that for a given batch, DYNAMIC is called once but FINALIZE is called twice, though FINALIZE is only called once on any given route). So then, I think the question is:

a) do we stop there,
b) implement one additional iteration through the DYNAMIC/FINALIZE loop to solve core's use case of config_translation depending on field_ui depending on entity modules,
c) implement a few extra iterations to leave room for contrib, or
d) implement an unlimited while() loop to let contrib go beyond whatever limit we can currently imagine but also crash a site

a) requires #93.2 (some additional special code path for entities), which is hard to evaluate without seeing some code. I'm open to it though. If we don't do a) though, then I think b) would be short sighted and not offer any advantage over c). Per #83, I prefer c) over d).

Yes, the DYNAMIC event would get passed the existing routes. So perhaps DERIVED would be a better name for it (since "dynamic" is what we're calling routes generated on the fly by the callback that was recently added).

The proposal is indeed "stop there, core doesn't need to handle every use case and fields/translations can take care of themselves." (IE, "A" from #95.) If we loop through multiple times, then splitting DERIVED and FINALIZE doesn't offer much benefit.

Remove D8MI tags since the problem and the discussion is now way out of scope of D8MI.

Bumping to critical per #2177041-61: Remove all implementations of hook_menu

#2177041: Remove all implementations of hook_menu means we need to be able to access the final list of routes after all alters have fired. If 'after' is by convention that's enough though I think as long as it's not dependent on listener priority as it is now.

This isn't a new problem but we got around it before by coupling the path root state information to menu_rebuild() directly previously, whereas it's now done in the alter event there - so to get the same functionality back that makes this critical to resolve.

Is this also a beta blocker or "just" release blocker?

Should only be an API addition, so not beta blocking- although possibly depends if the current event name changes.

Another issue that's run into this:

#2224553: Add _admin_route for field manager and admin/content.

wrong tab...

We discussed dawehner's data in IRC. Conclusion: A ~2 MB savings is not enough to justify the extra complexity of chunked route building, so we'll drop that and go with approach 2 from #93, albeit with the "DYNAMIC" event renamed to "DERIVED" to avoid confusion.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -142,17 +140,19 @@ public function rebuild() {
+    // DYNAMIC is supposed to be used to add new routes based upon all the
+    // static defined ones.
+    $this->dispatcher->dispatch(RoutingEvents::DYNAMIC, new RouteBuildEvent($collection));

This should be called DERIVED, I think, per follow up comments. That is to avoid confusion with the dynamic routes callback.

Apologies for changing status (wrong thread).

Yeah agreed with dawehner. Routes can be derived from YAML as much as anything else, so it's not really saying what it's about.

Also the confusion with dynamic_routes callback is not due to wrong naming: There are two ways to achieve exactly the same thing. The fact that they are both called 'dynamic' makes a lot of sense. The fact that there are two? Not so much.

(I didn't mean to start a bikeshed, really...) My concern is someone saying "it's a dynamic route", and the listener having no idea which of the two things we call "Dynamic routes" being responsible for creating it. Do I look in a listener, or in a callback method? I have no idea, because they're called the same thing.

Calling those routes that are dependent on other routes, not just on arbitrary configuration, "derived" (or, really, any other word than "Dynamic") neatly avoids that confusion and accurately describes what is supposed to go in that hook: Just those new routes that are based on some other route.

Re #117: Do you mean that we would make it safe to just call the route provider at any time, including during route rebuilding, and the provider would just return the current collection during rebuilding?

Wanted to re-roll this, but I can't get this to apply to any commit. Do you have this as a local branch @dawehner?

dawehner: You describe a terrible terrible thing. During rebuild, the route provider should be considered unusable. The race conditions or tight coupling we risk introducing otherwise are unacceptable.

Re #119: The problem is that that's not the case. You can access the route provider during rebuild and everything will work perfectly fine, depending on what you want to do. Proof is the current config_translation route rebuild. So I think making it work safely, i.e. independent of the current implementation of the route provider wouldn't be so bad. Otherwise we should fail explicitly and throw an exception if you access the route provider during a rebuild. That wouldn't be hard either, we should add a RoutingEvents::STARTING event and then the route provider just listens to that and RoutingEvents::FINISHED and sets an internal flag on those.

"Pass along the route collection"... Um, isn't that what we said we would do for route rebuild to the DERIVED and FINALIZE events?

I think what @dawehner means is the following:
- We do pass in the route collection during route rebuilding into the route subscriber.
- However, many route subscribers call into external services/classes and *those* need access to the route provider.
- For this reason @dawehner added a capability to the config_translation system (ConfigNamesMapper) to pass in the route collection and that it either uses the passed in route collection or the route provider. This works, but it makes that class aware of whether it is being called from route rebuilding or not, which is not really clean. It also adds a bunch of code.
- The same treatment would have to be done with REST, where the route subscriber calls into the resource plugin manager and the entity resource calls the route provider to get the routes for the entity links.
- Search apparently needs the same treatment, I don't know what the exact use-case is there.
- So we really only have two options: A) Make all those services/classes aware of the concept of route rebuilding in one or the other way or B) Make the route provider work during route rebuilding.

Having written this and realizing that we simply have the use-case of accessing routes during route rebuilding I tend to agree with @dawehner that we should do B). Now that we have decided to kill the segmeted dumping and dump the whole router once I don't think race conditions are really that much of an issue. And as explained this in fact *decreases* tight coupling. It introduces knowledge about route rebuilding into the route provider and thus lets the rest of the world be stupid and not have to know about route rebuilding.

Just a re-roll to see if patch applies.

Let me try this.

I have been trying to re-roll this patch for hours now and I feel very stupid now. Git just doesn't work for me today...

Test passed locally on my machine. @tstoeckler, can you please try to replicate the test?

Tagging beta target - this could end up with an API change for any dynamic route. Tempted to make it a beta blocker.

Awesome, thanks so much @dawehner. I had started fixing tests locally and wanted to finish this at some point, but yeah... thanks!

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -87,15 +87,10 @@ public function addRoutes(RouteCollection $routes) {
        // If there are no new routes, just delete any previously existing of this
        // provider.
        if (empty($this->routes) || !count($this->routes)) {
          $this->connection->delete($this->tableName)
   -        ->condition('provider', $options['provider'])
   

   The comment needs to be updated to match the new query.

2. +++ b/core/modules/config_translation/config_translation.module
   @@ -99,6 +99,9 @@ function config_translation_config_translation_info(&$info) {
            $base_route = $route_provider->getRouteByName('field_ui.instance_edit_' . $entity_type_id);
          }
          catch (RouteNotFoundException $e) {
   +        if ($collection = \Drupal::service('router.builder')->getCollectionDuringRebuild()) {
   +          $base_route = $collection->get('field_ui.instance_edit_' . $entity_type_id);
   +        }
   

   Don't we want to remove the first part of this?

3. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
   @@ -145,7 +159,12 @@ public function getBaseRouteParameters() {
      public function getBaseRoute() {
   -    return $this->baseRoute;
   

   Shouldn't we still store the result in $this->baseRoute?

4. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
   @@ -145,7 +159,12 @@ public function getBaseRouteParameters() {
   +      return $this->routeProvider->getRouteByName($this->getBaseRouteName());
   

   So we'll still allow some introspection?

5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Routing/RouteSubscriber.php
   @@ -36,15 +37,9 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager) {
   -  protected function alterRoutes(RouteCollection $collection, $provider) {
   -    // @todo \Drupal\config_translation\ConfigNamesMapper uses the route
   -    //   provider directly, which is unsafe during rebuild. This currently only
   -    //   works by coincidence; fix in https://drupal.org/node/2158571.
   -    if ($provider != 'dynamic_routes') {
   -      return;
   -    }
   

   Thank goodness.

1. What I like about this patch is that it splits off the static/declarative route processing from ::DYNAMIC.

   That should always be the standard pattern when input is processed from declarative definitions as well as dynamically executed PHP code.

2. I disagree with the idea of accessing routing data during a rebuild though.

   That wasn't ever possible/supported in the past — and/or if it was, then that was out of sheer luck + coincidence only.

   The idea of doing so is an architectural flaw. The order in which routes are being processed is not guaranteed at any point in time. Whatever your code relies on is not guaranteed to exist. If it's not guaranteed, then any amount of new methods and workarounds will not address the root cause and just give a completely false sense of a "solution."

   In case the argument is that ::DYNAMIC has the benefit of event subscriber priorities, then that's the next best massive architectural flaw:

   1. It assumes that your ::DYNAMIC doesn't rely on data provided by other ::DYNAMIC subscribers.
   2. Hard-coded priorities require you to know the hard-coded priorities of all of your dependencies — not just one or two, but all. And all of those can change without notice at any time, causing your module/subscriber to break at any time, out of a sudden.

1) is a good idea, but rather a minor clean-up.

2) means that the proposed solution does not resolve the problem. If there is a circular dependency, then we need to attack and address the root cause of that circular dependency. If that requires to change APIs, then we need to change the APIs. If that has to be a critical, beta-blocking issue, then it has to be. But changing the code to introduce explicit support for circular dependencies is a terrible idea; it doesn't resolve the actual root cause and will not fully resolve the root cause.

2.2) is a much more general flaw that ought to be a separate, critical, beta-blocking issue in my book. Symfony's EventDispatcher was designed for a piece-meal application framework that is under control of a single developer; it is not aware of the concept of a modular, extensible application, in which event subscribers are NOT under control of a single party. It is impossible to release without an EventDispatcher that is able to operate on relative dependencies. Doing so would seriously screw up contrib for the entire lifetime of D8.

Yes, #140 hearkens back to earlier discussions in this issue and during WSCCI meetings about what is ideal.
#142 illustrates well what separates that from reality.

1) Per #116, we should not be reusing the word "Dynamic". We already use that word for the callbacks in the routing.yml file, don't we? Reusing the term for a different code path is begging for developer confusion.

2) Tim: To be fair, the "reality" you refer to is "we have a design flaw that we're hacking around." This issue is adding more technical debt to avoid fixing existing technical debt. Let's not lie to ourselves and pretend otherwise. (Putting the in-progress collection on the builder object rather than the provider or its own service is less ugly, but it's still technical debt.)

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -123,7 +118,7 @@ public function dump(array $options = array()) {
   -          'provider' => $options['provider'],
   +          'provider' => isset($options['provider']) ? $options['provider'] : 'provider',
   

   Why even keep the field at all? If we're not doing separate dumps, just eliminate the field entirely.

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -103,9 +109,12 @@ public function rebuild() {
   +    // Truncate the router table by passing dump() without any passed in routes.
   +    $this->dumper->dump();
    
   +    $collection = new RouteCollection();
   +    $this->routeCollection = $collection;
   +    foreach ($this->getRouteDefinitions() as $routes) {
   

   If we're only going to have one collection rather than multiple, why do we need to truncate first? Just change dump() to truncate as part of the dumping process (which I think is closer to how the dumper was originally intended in Symfony anyway).

   rather, the dumper should do the delete and all the inserts, inside of a transaction (for the SQL implementation). Doing it here, outside of the dumper, means that the table will be emptied in a separate transaction than the data is entered, which introduces a race condition that would leave the site in a broken state if something dies before the new routes are added.

   Also, there's no router table. There's the dumped routes. "Table" exists only for the SQL implementation. The comments and implementation here should NOT be tied to the SQL implementation.

3. +++ b/core/modules/rest/lib/Drupal/rest/Routing/ResourceRoutes.php
   @@ -9,13 +9,16 @@
   +class ResourceRoutes extends RouteSubscriberBase{
   

   Missing space.

In the old menu system we were writing the insert query incrementally every 50 items or so to avoid the query hitting the MySQL query length limit. (All inside the same transaction as the delete.) We should probably still do so now that we're dumping all routes at once. (The dumper isn't doing that now because it assumes one module's worth of routes is small enough to not care; if we're dumping all at once, we should bring that safety check back.)

Arguably we should leave a comment that the route builder for why we're being gross and maintaining state inside the service.

Other than those, I think this is acceptable.

Unlike @Crell, I still disagree with the proposed architectural change, because (1) it adds complexity and (2) removes a major performance-related design aspect, whereas (3) logical sense dictates that even with the additional complexity, the problem will not and cannot be resolved entirely. We're duct-taping the symptom, but we're not fixing the root cause. That's in no way "academic"; it's based on experience of designing modular/extensible architectures.

I do bet that this change will back-fire in significant ways during the course of D8's lifetime. However, I will not derail or hold up this issue any further, because (1) arguments are only repeated, (2) I admit that I have no idea how to fix the deeper call chains [all I know is that consuming data while the data is being rebuilt is fundamentally flawed], and (3) worst case, it looks like we have some maintainers who are willing to learn The Hard Way™. :-)

This is not meant to say "I know better"; it's meant to say "I don't know how, but we should do better, because past experience provides us sufficient data points and because personally, I hate to repeat mistakes of the past."

We're duct-taping the symptom, but we're not fixing the root cause.

I totally agree. And in fact I think everyone here does. But until there is a concrete proposal to fix that root cause it's better than not duct-taping the symptom, right?

In contrast to @sun I actually believe this patch greatly improves the situation as it makes it very apparent where we have a conceptual/architectural flaw/deficiency in the system: By simply checking the usages of the getRouteCollectionDuringRebuild() method. The same flaws/deficiencies exist already in 8.x, but they are not in any way apparent unless you are aware of them already.

#157: That's why I'd like to see the patch document that we know what we're doing is nutty.

Thanks @dawehner! I think that addresses the rest of the criticisms, especially #158. Let's get this in and move forward with #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation and #2150747: Switch back to URI templates in entity annotations to resolve the rest of Crell's concerns.

Since the last patch didn't have one, here was the last interdiff.

Agreed with everyone else on the solution here - don't like it but can't think of a better way to fix it centrally.

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -87,80 +87,64 @@ public function addRoutes(RouteCollection $routes) {
   +      $this->connection->truncate($this->tableName)->execute();
   

   Since we know we're in a transaction here, should we just use an explicit delete instead of truncate?

2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -87,80 +87,64 @@ public function addRoutes(RouteCollection $routes) {
   +      // Split the routes into junks to avoid big INSERT queries.
   

   s/junks/chunks/

3. +++ b/core/lib/Drupal/Core/Routing/RoutingEvents.php
   @@ -13,13 +13,16 @@
   +   * This event is used to add new routes based upon existing routes.
   

   This isn't only used to add new routes based on existing routes is it? Also used just for routes that are dynamically defined from views or wherever.

4. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/Derivative/EntityDerivative.php
   @@ -104,12 +117,17 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +                // If the route does not exist it means we are in a brittle state
   

   I thought this issue might fix that. What's the issue that deals with it?

Now that routes don't have a provider key, what happens if two modules attempt to define the same route? Not sure that changes but worth checking.

Concur with #160.

Re #161:

1) It looks like Truncate isn't transaction-safe in MySQL: http://dev.mysql.com/doc/refman/5.5/en/truncate-table.html (DDLs are not transaction safe either). So yes, that should probably switch to a Delete statement.

3) For adding new routes purely off of some other existing data there's the "callback" entry in the routing.yml file. That was added to reduce the verbosity of registering a new event (which would likely always require a service or two). This way you have 4 steps, which minimizes the potential race conditions: Static routes, dynamic routes, dynamic routes that are based on other routes, alter routes without editing them.

4) I believe #2150747: Switch back to URI templates in entity annotations would deal with that, since it would remove the existing-route dependency on REST route building.

5) I think that would break now anyway. The provider is not a namespace; it was only used for chunking the delete statements during rebuild. It had no other use. The route-name-collision potential is unaffected by this patch.

@Crell #1 our truncate implementation is transaction safe, because it falls through to a delete if we're in a transaction, but I had to go and look at code to double check that. So functionally this would be the same, but I think it'd be clearer to use the delete directly.

#3 fair enough.

#4 not sure about that issue still but at least this doesn't make this worse.

#5 also fair enough

Thanks, all!

Back to catch.

OK. Committed/pushed to 8.x.

See you in #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation.

Yayayay!

There is a @todo in EntityDerivative saying "// @todo remove the try/catch as part of http://drupal.org/node/2158571

Not sure what to do. Follow a critical followup?

Works for me.

Bah! Bot, you aren't helpful today (it didn't pick up diff.txt).

RTBC + 1