Routing silently fails in kernel tests

There's a regression in D8 compared to the D7 router:

It is not a regression, this was an active design decision, see #356399: Optimize the route rebuilding process to rebuild on write

Also, possibly move the router table out of the schema into the router builder.

We had an issue for that, see #2338747: Move {router} out of system.install and create the table lazy

The patch doesn't actually patch the current kernel test base though, see \Drupal\KernelTests\KernelTestBase::register

Just got bitten by this writing a test in a module using the KernelTestBase. As chx explained on IRC, such tests can solve the issue for now adding the following fragment, much like the patch does it:

$this->installSchema('system', 'router');
$this->container->get('router.builder')->rebuild();

Given that my productivity increased a lot with the new test base, here is an issue to also improve that: #2605956: Port #2605684 to the new KernelTest

In short: checking the $update in postSave is wrong because it says nothing about the newness of the link as demonstrated. Instead, we can just check whether the definition exists and if it does then upgrade it.

Sounds like a good fix!

1. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -200,15 +200,16 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
        // The menu link can just be updated if there is already an menu link entry
        // on both entity and menu link plugin level.
   

   Mh, this comment now is not really exact anymore

2. +++ b/core/modules/simpletest/src/TestServiceProvider.php
   @@ -25,4 +27,17 @@ function register(ContainerBuilder $container) {
   +      for ($id = 'router.route_provider'; $container->hasAlias($id); $id = (string) $container->getAlias($id));
   

   This for without a body is valid PHP? Sounds like a while loop for me, which would be IMHO more readable

3. +++ b/core/modules/simpletest/src/TestServiceProvider.php
   @@ -25,4 +27,17 @@ function register(ContainerBuilder $container) {
   +      $container->setDefinition('router.route_provider', new Definition('Drupal\simpletest\RouteProvider'));
   

   Unneeded suggestion: You could use RouterProvider::class here

This is clearly developer experience

+++ b/core/modules/simpletest/src/RouteProvider.php
@@ -0,0 +1,122 @@
+  public static function getSubscribedEvents() {
+    RouteProvider::getSubscribedEvents();
+  }

This would better have a return statement

All tests pass so we are fine here. I hope we don't get bitten by the additional magic later, because well, this could be tricky for other people to debug.
On the other hand tests aren't part of any supported API.

The only problem I have with this is that this changes things from an explicit system to a every implicit system, which many people like, but I think most of the time implicitness
is problematic.

Given that these are test only patches, this is 'rc eligible'

Let's talk about the change in MenuLinkContent ...

#2605684-12: Routing silently fails in kernel tests describes the problem pretty clear. The solution for this problem is really elegant as it removes the complexity of the code and by that remove the potential
edge cases.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -200,15 +200,16 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
-    if ($update && $menu_link_manager->getDefinition($this->getPluginId())) {
+    $definition = $this->getPluginDefinition();
+    if ($menu_link_manager->getDefinition($this->getPluginId(), FALSE)) {

It looks like you are throwing out the check on the $update flag?

@pwolanin
So yeah $menu_link_manager->getDefinition($this->getPluginId(), FALSE) will return TRUE, if there is already an entry in the menu_tree and FALSE otherwise, which
is IMHO exactly what we want to check.

Ok, so it sounds like this just needs some code comments explaining that logic and why the flag is (should be) ignored?

I looked quickly at the call chain but not enough to understand what's happening in detail. Something seems wrong there if we get that mysterious saving behavior, but at least this added comment makes it possible to understand the code.

This is still RTBC

Can we split the MenuLinkContent fix out into its own issue that blocks this one?

Both parts would potentially be eligible for a patch release, so @effulgentsia and I don't think this needs to be done during RC. If this issue does actually contain something disruptive that wouldn't be eligible for a patch release, let's add that to the issue summary.

I am still convinced about it, given that it makes things easier for people to write tests.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -200,15 +200,22 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+    $definition = $this->getPluginDefinition();
+    // Even when $update is FALSE, for top level links it is possible the link
+    // already is in the storage because of the getPluginDefinition() call
+    // above, see https://www.drupal.org/node/2605684#comment-10515450 for the
+    // call chain. Because of this the $update flag is ignored and only the
+    // existence of the definition (equals to being in the tree storage) is
+    // checked.
+    if ($menu_link_manager->getDefinition($this->getPluginId(), FALSE)) {
       // When the entity is saved via a plugin instance, we should not call
       // the menu tree manager to update the definition a second time.
       if (!$this->insidePlugin) {
-        $menu_link_manager->updateDefinition($this->getPluginId(), $this->getPluginDefinition(), FALSE);
+        $menu_link_manager->updateDefinition($this->getPluginId(), $definition, FALSE);
       }
     }
     else {
-      $menu_link_manager->addDefinition($this->getPluginId(), $this->getPluginDefinition());
+      $menu_link_manager->addDefinition($this->getPluginId(), $definition);

It looks like this change could cause equivalent failures in contrib/custom modules doing the same thing.

So the question for me is:

- is this a bug in MenuLinkContent that will affect production sites, that is exposed by the test coverage
- or, is it something that will only fail in tests

If it's the latter, this starts to look 8.1.x-ish to me - since we're forcing changes to production code in a patch release. Back to CNR to figure out which it is.

@catch, @effulgentsia and I discussed this and we feel the benefits outweigh the risks. Committed 817ee92 and pushed to 8.0.x. Thanks!

Let's remove that in 8.1.x again, see #2605956: Port #2605684 to the new KernelTest