Generating a local task based on a missing route causes a crash

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -182,7 +185,16 @@ public function getDefinitions() {
   +      if (isset($definition['module_dependencies'])) {
   +        if (($this->checkModuleDependencies($definition['module_dependencies'])) == FALSE) {
   +          unset($definitions[$plugin_id]);
   +        }
   +      }
        }
   +
   +    // Filter out definitions with unmet module dependencies.
   +    $this->filterByModuleDependencies($definitions);
   

   Do we still need both? I guess you just forgot to drop that code.

2. +++ b/core/lib/Drupal/Core/Plugin/ModuleDependencyTrait.php
   @@ -0,0 +1,103 @@
   +  }
   +}
   

   Just put a newline between them.

You could argue that 'dependencies][module' could be used as well, but this would imply that other dependencies are supported which is simply not the case. Let's be more parallel with routing instead.

Reroll of #7 with updates from review in #8

Worked a bit on this issue:

• Removed the manual calls from the menu plugin managers and moved it into the default plugin manager
• Added a proper test coverage
• Some things here and there

Just a general idea: we could pull in the provider filtering as well.

I totally forgot that traits sucks.

I was thinking how can we create a test only patch for this bug?

Well, it should be for sure possible to create a menu link in some new test module which depends on the existance of some other module.

* Check that the link is not there is the other module exists
* Enable the other module, check that the link appears.

I've tried to do the test that @dawehner mentions in #19 and create a new module with the route dependent on another module (menu_ui in this case), then enable the second module, but the link seems to be there for both cases, I'll ask for feedback.

+++ b/core/modules/system/src/Tests/Menu/MenuLinkDependencyTest.php
@@ -0,0 +1,41 @@
+    /** @var \Drupal\Core\Menu\MenuLinkManagerInterface $menu_link_manager */
+    $menu_link_manager = \Drupal::service('plugin.manager.menu.link');
+    $definitions = $menu_link_manager->getDefinitions();
+    $this->assertTrue(empty($definitions['menu_dependency_test.dependency']), 'A menu link with unmet dependencies is not found.');

Maybe also check the error in the IS which is executing the local task manager?

Maybe more like this? Open for discussion.

+++ b/core/modules/system/tests/modules/menu_dependency_test/menu_dependency_test.links.menu.yml
@@ -1,3 +1,4 @@
+  module_dependencies: 'menu_ui'

+++ b/core/modules/system/tests/modules/menu_dependency_test/menu_dependency_test.routing.yml
@@ -4,5 +4,3 @@ menu_dependency_test.menu_dependency_test:
-  requirements:
-    _module_dependencies: 'menu_ui'

I am not sure about this change but other then this it is RTBC.

Rerolled #23 to have a non-conflicting class name.

#26 should be adressed afaik ... if possible it makes sense to define it both routing level ... if you don't control the route, maybe define it on the menu link level as well.

As talked with @pcambra: It would be cool if the property set on the route would be automatically inherited/reflected for the menu links as well.

Edit: This is not the patch you're looking for.

Generated an interdiff with patchutils, it should be easier to review.

Ok, so I had added an extra file totally unrelated, sorry about that. Here's the right patch with interdiff and all.

As discussed with @dawehner, adding the check on the routes as well as on the menu links, so if you add module_dependencies to either part, it would work.
Sorry for the horrible code in that part, but I'm not really sure how to get the routes in a clean way (I tried to inject the service but there's already a constructor in this part). Directions on how to improve that part are highly appreciated.

Also, I've noticed a mismatch when using YamlDiscovery, some bits like RouteBuilder use the component directly whereas other parts use the plugin. Is that expected?

+++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
@@ -190,6 +190,25 @@
+    // Also filter by the unmet route dependencies.
+    $routeDiscovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
+    $route_definitions = $routeDiscovery->getDefinitions();
+    foreach ($definitions as $plugin_id => $plugin_definition) {
+      $route_name = $plugin_definition['route_name'];
+      if (!empty($route_definitions[$route_name]) && !empty($route_definitions[$route_name]['requirements']) && !empty($route_definitions[$route_name]['requirements']['_module_dependencies'])) {
+        if ($modules = $route_definitions[$route_name]['requirements']['_module_dependencies']) {
+          $explode_and = $this->explodeString($modules, '+');
+          foreach ($explode_and as $module) {
+            // If any moduleExists() call returns FALSE, remove the menu and
+            // move on to the next.
+            if (!$this->moduleHandler->moduleExists($module)) {
+              unset($definitions[$plugin_id]);
+            }
+          }
+        }
+      }
+    }

so a) we do we not just check for 'route_name' of the plugin definition in filterByModuleDependencies to copy it over? (Note: here you just use it for menu links, but don't you want to have the exact same behaviour for local tasks / actions ...

Also, I've noticed a mismatch when using YamlDiscovery, some bits like RouteBuilder use the component directly whereas other parts use the plugin. Is that expected?

I don't see a problem with that :) Stuff which aren't plugins (routes) don't use the YamlDiscovery class from plugins, stuff which are plugins use the YamlDiscovery class
from plugins, which itself uses the other yaml discovery internally.

.

Here's a second try applying #37, I have add an additional check instead of merging the dependencies because it would be very hard to combine something like ['a','b','c'] with 'd','e' or 'f'+'g'

+++ b/core/lib/Drupal/Core/Plugin/ModuleDependencyTrait.php
@@ -21,12 +23,40 @@
+   */
+  protected function getRouteDependencies(array $definition, array $route_definitions) {
+    if (isset($definition['provider']) && isset($route_definitions[$definition['provider']])) {
+      return NestedArray::getValue($route_definitions, array($definition['provider'], $definition['route_name'],'requirements', '_module_dependencies'));
     }

Good question whether we have to merge, I would be fine with just overriding the value from the parent, if defined ... What do you think about it?

Here's another take, I think I had the wrong check.

just overriding the value from the parent

What if there's a mismatch? if the plugin depends on the module but the route doesn't, isn't this forcing to have it defined in both places?

What if there's a mismatch? if the plugin depends on the module but the route doesn't, isn't this forcing to have it defined in both places?

How can the menu link depend on it, but not the route? Maybe its just an example which is not obvious for me.

What about this then?

I've had to add unit test expectations about the getModuleDirectories method of the module handler that is mocked as the filterByModuleDependencies method is called from the DefaultPluginManager as well.

+++ b/core/lib/Drupal/Core/Plugin/ModuleDependencyTrait.php
@@ -55,7 +55,6 @@ protected function filterByModuleDependencies(array &$definitions) {
-//    if (isset($definition['provider']) && isset($route_definitions[$definition['provider']])) {

what is with that?

It was a commented line I leftover in #44, still dealing with some odd test failures

Missed one of the manager tests.

Let's fix this.

+++ b/core/modules/system/src/Tests/Menu/MenuLinkDependencyTest.php
@@ -0,0 +1,56 @@
+ * Definition of Drupal\system\Tests\Menu\MenuLinkDependencyTest.

Contains

Here we go.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -27,6 +27,7 @@
+  use ModuleDependencyTrait;

+++ b/core/lib/Drupal/Core/Plugin/ModuleDependencyTrait.php
@@ -0,0 +1,150 @@
+    $route_definitions = $this->getRouteDefinitions();
+
+    foreach ($definitions as $plugin_id => &$definition) {
+      // If the route has module dependencies, use them instead, menu links,
+      // actions, etc shouldn't have dependencies that are not shared with the
+      // routes.
+      if ($route_dependencies = $this->getRouteDependencies($definition, $route_definitions)) {
+        $definition['module_dependencies'] = $route_dependencies;
+      }
...
+  protected function getRouteDependencies(array $definition, array $route_definitions) {
+    if (isset($definition['provider']) && isset($definition['route_name'])) {
+      return NestedArray::getValue($route_definitions, array($definition['provider'], $definition['route_name'],'requirements', '_module_dependencies'));
+    }
+  }

The trait is being used in the DefaultPluginManager which is for all plugins but contains very specific assumptions that it is dealing with routes.

Re-rolled the patch for 8.9.x-dev in order to continue working on this.

Only problem has been that I couldn't apply the last test any more:

diff --git a/core/tests/Drupal/Tests/Core/Render/ElementInfoManagerTest.php b/core/tests/Drupal/Tests/Core/Render/ElementInfoManagerTest.php
index ef9a956..01cd07d 100644
--- a/core/tests/Drupal/Tests/Core/Render/ElementInfoManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoManagerTest.php
@@ -68,6 +68,9 @@ public function testGetInfo($type, $expected_info, $element_info, callable $alte
       ->will($this->returnCallback($alter_callback ?: function($info) {
         return $info;
       }));
+    $this->moduleHandler->expects($this->once())
+      ->method('getModuleDirectories')
+      ->willReturn(array());
 
     $this->cache->expects($this->at(0))
       ->method('get')

There is no reference in the new code which would allow me to re-apply this change. Maybe somebody could help me with this?

I'm planning to work on comments from #55 next.

I've now moved the ModuleDependencyTrait out of the default plugin manager over to the route related plugin managers in the Drupal\Core\Menu namespace.

Patch #66 didn't apply.

Sorry for that, for some reason my patch was created with a duplicated base path for each file. Not sure how that happened but it's fixed now.

I re-rolled the patch for 9.4.x.

I encountered this issue using the Scheduler module: it adds a local task pointing to a module-provided view. When I disable this view, the site crashes because the route no longer exists. The current patch doesn't fix the issue, because Views doesn't add module_dependencies to its routes.

Fixed Drupal CS issues of patch #72.