What to do with "Recursive router rebuild detected."

You need to figure out what is causing the recursion and then we need to fix it. Might be rest.module.

drush doesn't show backtrace, so add a debug_print_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS) call above the place where the exception is thrown.

Could be the way it is called in EntityDerivate in rest.module:

              if (($collection = $this->routeBuilder->getCollectionDuringRebuild()) && $route = $collection->get($route_name)) {
              }
              else {
                $route = $this->routeProvider->getRouteByName($route_name);
              }

If $collection is returned, it should never try to call getRouteByName(), so the second part there should move in an inner if.

+++ b/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php
@@ -115,12 +115,18 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+              else {
+                throw new RouteNotFoundException($route_name);
+              }

I think instead of this, we should just repeat the logic inside the catch block and continue. Throwing and immediately catching the exception again is just unecessary overhead.

+++ b/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php
@@ -115,12 +115,18 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+                throw new RouteNotFoundException($route_name);

Beside the comment from berdir, in general at least let's add a message which is partly helpful.

DRY is a nice principle, but not using exceptions for code flow is as well. And we're talking about two lines here.

If you want DRY, then move those two lines within the catch below it and check for if (!isset($route)).

But it is not a misconfiguration, it is a known issue with the current routing/entity links systems and is being addressed in #2259445: Entity Resource unification.

This is what I was suggesting. Also had to harden ResourceRoutes in case you have default configuration for entity types during installation which are currently not there because the derivative class (which should be named Deriver) skipped them.

I assume no way we can add a test here? Unless rebuild.php can be used?

You can always test :)

Reroll of #14. I've re-included the test-only patch to illustrate the fix.

Looks almost ready!

+++ b/core/modules/rest/tests/src/Unit/Plugin/Derivative/EntityDerivativeTest.php
@@ -0,0 +1,134 @@
+    $this->assertSame('bar', $definition['foo'], 'The base plugin definition was not properly merged.');
+    $this->assertSame('entity:entity1', $definition['id'], 'The derivative ID was not properly set.');
+    $this->assertSame('/entity/entity1/{entity1}', $definition['uri_paths']['canonical'], 'The default canonical uri was not properly set.');

the assertion messages are confusing. I think the "not" is wrong. The assertion message should tell you what should happen. If it does not happen during the test then the assertion message will be red, so you don't specify the fail message here but the success message.

The assertion message should tell you what should happen.

That is the case with SimpleTests, but with PHPUnit, my understanding was that we use a message to describe the failure since that is the only time it is displayed. I'm not finding this documented for Drupal though. Looking through our existing tests, I'm seeing that to be the case (see \Drupal\Tests\Core\Site\SettingsTest for instance).

Interesting, so this should not be blocked on the assertion messages.

If you look at https://phpunit.de/manual/current/en/appendixes.assertions.html#appendix... PHPUnit has very useful output in the generic assertion messages by printing the actual values, which is very helpful when looking at broken tests. Maybe we should move the message to a code comment and stick with the default assertion?

I noticed that there is no test coverage for the RouteNotFound exception in getDerivativeDefinitions(), correct?

The RouteNotFound exception is tested in the last assert, it just isn't testing for a thrown exception (since the exception is caught).

I could reroll to remove the custom assert messages--most core tests seem to not use custom messages.

This moves the custom assert messages to code comments. I'm not re-attaching the failing test.

1. +++ b/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php
   @@ -113,15 +113,22 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +            if (($collection = $this->routeBuilder->getCollectionDuringRebuild())) {
   

   You can remove a set of parens

2. +++ b/core/modules/rest/src/Routing/ResourceRoutes.php
   @@ -71,7 +72,14 @@ protected function alterRoutes(RouteCollection $collection) {
   +      } catch (PluginNotFoundException $e) {
   

   Newline, please.

3. +++ b/core/modules/rest/src/Routing/ResourceRoutes.php
   @@ -71,7 +72,14 @@ protected function alterRoutes(RouteCollection $collection) {
   +        // of module enabling/disabling, so we simply exclude this entity
   

   Not "enabling/disabling" anymore.

4. +++ b/core/modules/rest/tests/src/Unit/Plugin/Derivative/EntityDerivativeTest.php
   @@ -0,0 +1,140 @@
   +  public function testGetDerivativeDefinitions() {
   

   Can this method be chopped up to test different parts of the code flow, please?

+++ b/core/modules/rest/src/Routing/ResourceRoutes.php
@@ -71,7 +72,14 @@ protected function alterRoutes(RouteCollection $collection) {
+      try {
+        $plugin = $this->manager->getInstance(array('id' => $id));
+      } catch (PluginNotFoundException $e) {
+        // If the plugin type does not exist it means we are in a brittle state
+        // of module enabling/disabling, so we simply exclude this entity
+        // type.
+        continue;
+      }

It would be great to understand how this could happen, so we could check that as well, like with some kind of flag somewhere?

[#2382937] just landed, which is supposed to make this a lot less fragile.

We probably need to re-evaluate if we still need anything added here, but it is probably a good idea to still add the test scenario that resulted in this problem.

@dawehner: I had that too I believe in my project, it happens when the entity type is skipped in the deriver and try to access it (in my case, it was default configuration that triggered that).

Corrrection. After #2406439: Cleanup EntityDerivative and RouteBuilderInterface.

When I debugged that some days ago the problem was the following:

Rest module ships with some rest.settings.yml which causes a $entity_manager->getDefinition()calls
which throws an exception during route rebuild time.

Due to our "excellent" idea to render exception pages via our normal rendering system, we get another router rebuild, boom.

After #356399: Optimize the route rebuilding process to rebuild on write the chance of this happening is much lower, it just can happen if some code actually rebuilds the router on purpose, which most probably won't happen.

+++ b/core/modules/rest/src/Routing/ResourceRoutes.php
@@ -71,7 +72,14 @@ protected function alterRoutes(RouteCollection $collection) {
-      $plugin = $this->manager->getInstance(array('id' => $id));
+      try {
+        $plugin = $this->manager->getInstance(array('id' => $id));
+      } catch (PluginNotFoundException $e) {
+        // If the plugin type does not exist it means we are in a brittle state
+        // of module enabling/disabling, so we simply exclude this entity
+        // type.
+        continue;
+      }

This is the key portion of this patch.

Which means it's solved by #2397271: REST configuration fails if it contains a plugin reference that does not exist, which itself is a temporary measure until #2308745: Remove rest.settings.yml, use rest_resource config entities is solved.