Allow MenuLinkTree manipulators to be altered

First of all, thanks for your solution. I tested it and it works as expected.

I have some suggestions on your code though:

1. You have some coding standards issues:
   • Opening braces for classes go on the same line.
   • The @var comments should be 3 lines, with the opening and closing in their own line.
   • The __construct function must declare visibliity public in MenuLinkTreeManipulatorsAlterEvent.php.
2. I'm not sure about function return types declarations. While officially d8 requires PHP 7.0.8 or higher, 5.5 is still supported for existing installations (https://www.drupal.org/node/3044409). This patch might break some sites on old servers as is.
3. Adding an abstract class similar to RouteSubscriberBase would be nice.

Looks like a potentially useful addition (and the patch comes with tests, which is great!), but needs work because of code style at least. I also spotted this:

+++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
@@ -3,7 +3,7 @@
 use Drupal\Component\Plugin\Exception\PluginException;
-use Drupal\Core\Cache\Cache;
+use Drupal\Core\Cache\RefinableCacheableDependencyTrait;
 use Drupal\Core\Plugin\PluginBase;
 use Drupal\Core\Url;
 
@@ -12,6 +12,8 @@

@@ -12,6 +12,8 @@
  */
 abstract class MenuLinkBase extends PluginBase implements MenuLinkInterface {
 
+  use RefinableCacheableDependencyTrait;
+
   /**
    * The list of definition values where an override is allowed.
    *
@@ -167,25 +169,4 @@ public function deleteLink() {

@@ -167,25 +169,4 @@ public function deleteLink() {
     throw new PluginException("Menu link plugin with ID '{$this->getPluginId()}' does not support deletion");
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function getCacheMaxAge() {
-    return Cache::PERMANENT;
-  }
-
-  /**
-   * {@inheritdoc}
...
 }

This looks like an unrelated change.

This solution is similar to #2854013 except that it applies to any menu link tree regardless of how it is built (ex. via a block or theme preprocess). This decouples the building of a menu link tree from how it is rendered.

I am wondering if there would be use cases where, i.e.

Hide links without a translation.

What if you want a certain menu block that does not do that?

This seems like a better approach in theory, but when I went to use it I almost immediately ran into an issue where I want to apply a manipulator only to some trees and not others, but the alter event does not expose any identifiable information I could find. This means that unless you filter based on the content of the tree, any manipulators added via this method are an 'all or nothing' type deal.

@s.abbott your comment is incorrect. I'm for example using the getMenuName() function of every tree element to make changes to a specific menu only.

Fixed some coding standard issues and rerolled for 9.2.x.

Re-rolled patches against 9.3.x as patch was not getting applied for MenuLinkTreeTest.php file so re-rolled it manually.

Re-rolling #14 for 9.5.3

Last patch was generated incorrectly and was missing files.

Is there a patch that works with D10?

Adding a patch that can be applied to D10.

Fixing my previous patch, to be used with D10

Patch for Drupal 10.2.

I've rolled ##24 onto an MR and updated the event dispatcher based on https://www.drupal.org/node/3376090

This was set to NW all the way back in #7 so I think this may be ready for another review.

The main thing I'm interested in is allowing menu links to have their cacheable metadata refined (i.e adding RefinableCacheableDependencyTrait to MenuLinkBase).

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Appears to have test failures

core/lib/Drupal/Core/Menu/MenuLinkBase.php will the removed functions have to be deprecated?

core/lib/Drupal/Core/Menu/MenuLinkBase.php will the removed functions have to be deprecated?
No, they are provided by the trait that's added instead.

Failures should be fixed with latest commit.

Left 2 more small comments, will keep an eye out for this one.

Resolved the comments and also applied similar changes to the rest of the new files. Also removed the reference from getManipulators as we don't need that when we have a setter.

Failing tests due to https://www.drupal.org/project/drupal/issues/3041318

+1 to this solution

The event does not alters the manipulators array, help!

    $this->eventDispatcher->dispatch(new MenuLinkTreeManipulatorsAlterEvent($tree, $manipulators, $this), MenuLinkTreeEvents::ALTER_MANIPULATORS);
    foreach ($manipulators as $manipulator) {
      $callable = $this->callableResolver->getCallableFromDefinition($manipulator['callable']);

My subscriber:

  /**
   * React to manipulators alter event.
   */
  public static function manipulatorsAlter(MenuLinkTreeManipulatorsAlterEvent $event) {
    $manipulators = $event->getManipulators();
    if (!empty($manipulators)) {
      array_unshift($manipulators, ['callable' => 'translatable_menu_link_uri.alter_links:alterMenuLinksManipulator']);
    }
    else {
      $manipulators = [['callable' => 'translatable_menu_link_uri.alter_links:alterMenuLinksManipulator']];
    }
    $event->setManipulators($manipulators);
  }

Maybe the array should be by reference?

I would say

  $this->eventDispatcher->dispatch(new MenuLinkTreeManipulatorsAlterEvent($tree, $manipulators, $this), MenuLinkTreeEvents::ALTER_MANIPULATORS);
    foreach ($manipulators as $manipulator) {
      $callable = $this->callableResolver->getCallableFromDefinition($manipulator['callable']);

should be:

  $event = new MenuLinkTreeManipulatorsAlterEvent($tree, $manipulators, $this);
  $this->eventDispatcher->dispatch($event, MenuLinkTreeEvents::ALTER_MANIPULATORS);
    foreach ($event->getManipulators() as $manipulator) {
      $callable = $this->callableResolver->getCallableFromDefinition($manipulator['callable']);

The $manipulators array is not changed during dispatch because arrays are copied when assigned, so a call to setManipulators inside a subscriber changes the array inside the $event but not the original $manipulators array used to create the instance of $event.

We are using 10.2.7, we are not sure, if Drupal 11 treats arrays in a different way, but the @tunic comment seems to be logical.

The change proposed by @tunic works for as on Drupal 10.2.7

Tests are failing https://git.drupalcode.org/issue/drupal-3091246/-/jobs/2307611

The manipulator is not applying

    There was 1 failure:
    
    1) Drupal\KernelTests\Core\Menu\MenuLinkTreeTest::testMenuManipulatorAlter
    Failed asserting that an array contains 'menu-test-link'.
    
    /builds/issue/drupal-3091246/core/tests/Drupal/KernelTests/Core/Menu/MenuLinkTreeTest.php:220

Here came the regression, the references were removed on commit https://git.drupalcode.org/project/drupal/-/commit/552c484ad403a37e93056...

The options are to apply the solution proposed by @tunic or recover the property manipulators as reference, the last one could be a bit confusing to implement by the user on the Subscriber.

Failed Javascript tests https://git.drupalcode.org/issue/drupal-3091246/-/pipelines/275582

Ready to review!!

Posted a few comments in the MR.

The current implementation in #2466553: Untranslated menu items are displayed in menus is very similar to this

Opened #3475433: Make it easier to decorate menu.default_tree_manipulators which proposes another method of decorating manipulators.

Faffing around with this now.

How does one determine exactly which menu they are adjusting the manipulators on?

I am using the code from the patch for 10.2, so perhaps this has been addressed later... but I'm looking to remove checkAccess from certain menus as we can simply rely on published... but I'll be darned if I can't figure out how to determine the menu based on what's available to me in the event.

Solve MR conflict with Drupal 11

Deprecation:

    1 test triggered 1 deprecation that can be ignored on phpunit tests:
    
    1) /builds/issue/drupal-3091246/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
    Calling Drupal\Core\Menu\MenuLinkTree::__construct() without the $eventDispatcher argument is deprecated in drupal:11.1.0 and will be required in drupal:12.0.0. See https://www.drupal.org/node/3463907
    
    Triggered by:
    
    * Drupal\Tests\navigation\Functional\NavigationShortcutsBlockTest::testNavigationBlock (7 times)
      /builds/issue/drupal-3091246/core/modules/navigation/tests/src/Functional/NavigationShortcutsBlockTest.php:35

Removed constructor parameter comments , MenuLinkMock::create to MenuLinkMock::createMock, changed deprecation version to 11.3.0,
parse the eventDispatcher to parent constructor.

Please review

Sorry but deprecations need to be bumped to 11.4 now.

Also think we probably need a CR for the new event, if the goal is to allow others to alter it.

Bumped to 11.4 and new CR for the new event

Believe super close! 1 thread still open on MR.

removed RefinableCacheableDependencyTrait

believe feedback for this one has been addressed

Tagging for subsystem and/or framework manager review. While allowing menu tree structures to be altered is a good idea, the implementation and idea of "manipulators" feels a bit like a Drupalism. This MR adds an event where callbacks can be set, which are then immediately called. Why doesn't transform() just dispatch an event containing the tree, and the manipulators themselves can be event subscribers? Every case in core where transform() is called does a mostly copy-pasted setup with some hardcoded strings:

    $tree = $this->menuTree->load($menu_name, $parameters);
    $manipulators = [
      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
      ['callable' => NavigationMenuLinkTreeManipulators::class . ':addSecondLevelOverviewLinks'],
      ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
    ];
    $tree = $this->menuTree->transform($tree, $manipulators);

    $tree = $this->menuTree->load(NULL, $parameters);
    $manipulators = [
      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
      ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
    ];
    $tree = $this->menuTree->transform($tree, $manipulators);

    $tree = $this->menuTree->load($menu_name, $parameters);
    $manipulators = [
      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
      ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
    ];
    $tree = $this->menuTree->transform($tree, $manipulators);

    $tree = $this->menuTree->load($menu->id(), $parameters);
    $manipulators = [
      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
      ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
    ];
    return $this->menuTree->transform($tree, $manipulators);

etc.

This feels suboptimal, and I'm not sure adding this API on top of this is the right thing to do.

For the case in Navigation (or elsewhere) where the caller wants something different to happen, perhaps transform() could be passed a custom event class so downstream subscribers can tell the difference between who is calling.

edit: not sure this will work actually with the way event subscribers are set up, but maybe there is some alternative solution to the variations.

I like the idea of this as an event subscriber alone where you can adjust the menu tree manipulators.

For our use case with groups, having menu.default_tree_manipulators:checkAccess on a larger menu made for a very large amount of additional db calls, when all we care about is published on this particular menu. Removing that manipulator made our life a lot easier.

I still didn't get an answer on how you'd determine just which menu you're adjusting however, which I think would be a critical piece for this. I looked through all the values available to me in xdebug and wasn't seeing any identifying values to ensure which menu I was altering. Maybe I missed something obvious though, I'm capable of being wrong for sure.

Being able to identify just which menu you are altering would be critical though, as removing menu.default_tree_manipulators:checkAccess is fine for our main menu for example, but it would be an issue if we did that on other menus... say admin menu!

I agree with the what seems to be @longwave's sentiment that this approach might be making an ugly situation a little worse.

Every menu link has a menu, but also the transform might not be happening in the context of that specific menu in code like \Drupal\system\SystemManager::getAdminBlock() where you can be rendering anything you want based on on incoming menu link.

The fact that the manipulators are almost the same across core suggests the transform() method could just start with that standard list and add an alter hook? I'm not sure I understand why an event is preferred here?

It does seem like there needs to be more context provided (e.g. menu name or information about where the tree is being built).

+1 for hooks. I have the same question, what benefit do event subscribers provide here?

I was also wondering why this couldn't be an alter hook, although it might need to be fired from the place adding the manipulators rather than centrally?

Don't know exactly how yet, but this would probably help #3529163: Default menu block plugin should only show enabled links - e.g. could workspaces remove the manipulator again?

In our case, the hook is too late for alteration; we need to alter the menu before the pre-render see issue https://www.drupal.org/project/translatable_menu_link_uri/issues/3472291

Coming here from #3559353: Find a way to bulk load entities and aliases in menus where this could be useful too as it might be too opinionated for core.

event vs hook is IMHO an implementation detail, they essentially achieve the same. For a while we moved things, especially lower-lever to events so we could use OOP/services, but now we have OOP hooks, so that changes things again.

I think the problem with replacing manipulators entirely is the necessary context as #60 said. navigation is a good example. That extra logic runs just because it is the navigation block. The same menus could be shown in a different place where that manipulator should not run. If we replace them completely then we need a way to pass in a context string or other identifier that allows the navigation alter to just run on itself.

navigation module seems to have invented two different alter mechanism, one is the passed in manipulator, but it _also_ subclasses MenuTree as a separate service, so it can run an alter hook in \Drupal\navigation\Menu\NavigationMenuLinkTree::transform(), which it then implements in \Drupal\navigation\Hook\NavigationHooks::navigationMenuLinkTreeAlter(). So we essentially already have a precedent for an alter hook.

BC is tricky as it's again a multi-directional BC thing. We need BC if the menu link tree service is customized, we also need BC for callers. We could completely deprecate the transform() method and add a new alter() method on a new optional interface but it also seems quite unusual to require an explicit call between load and build for some that's pretty fundamental like access checks. But how do we pass in the context if not through a new method? And how do we ensure correct behavior when MenuLinkTree is subclassed, which would then also have the new method? We had similar issues with the user authentication API changes.

Calls in contrib: https://search.tresbien.tech/search?q=menuTree-%3Etransform%20-r%3Adrupa...
subclasses: https://search.tresbien.tech/search?q=%22extends%20MenuLinkTree%22%20-r%... (doesn't catch some cases that alias the parent)

Not sure we need an event or a hook here, but instead we could use tagged services.

The common case is currently:

    $tree = $this->menuTree->load($menu_name, $parameters);
    $manipulators = [
      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
      ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
    ];
    $tree = $this->menuTree->transform($tree, $manipulators);
    return $this->menuTree->build($tree);

What if we changed this to just:

    $tree = $this->menuTree->load($menu_name, $parameters);
    $tree = $this->menuTree->transform($tree);
    return $this->menuTree->build($tree);

The two default manipulators would become tagged services. Anything else that wants to manipulate any menu item (the original aim of this issue) becomes another tagged service; it can add itself before, in between or after checkAccess and generateIndexAndSort.

For special cases where callers add their own manipulator, they just call their helper directly. For example ModuleAdminLinksHelper::getModuleAdminLinks() becomes:

  $menuTree = $this->menuLinkTree->load('system.admin', $parameters);
  $menuTree = $this->menuLinkTree->transform($menuTree);
  $menuTree = $this->flattenTree($menuTree);
  $this->memoryCache->set(self::ADMIN_LINKS_MENU_TREE, $menuTree);

or MenuParentFormSelector::getParentSelectOptions() becomes:

  foreach ($menus as $menu_name => $menu_title) {
    $tree = $this->menuLinkTree->load($menu_name, $parameters);
    $tree = $this->checkNodeAccess($tree);
    $tree = $this->menuLinkTree->transform($tree);
    $this->parentSelectOptionsTreeWalk($tree, ...);
  }

The only case in core where this becomes tricky is NavigationMenuBlock:

    $manipulators = [
      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
      ['callable' => NavigationMenuLinkTreeManipulators::class . ':addSecondLevelOverviewLinks'],
      ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
    ];

However I'm pretty sure we can refactor this so the second level links can be either before or after.

We also have some awkwardness in that both Navigation and Toolbar provide their own subclass of MenuLinkTree - however I'm also pretty sure that that is not strictly necessary either, although that's almost certainly out of scope here.

Note that because we're removing the second argument to transform() here we can both do this with backward compatibility and issue a deprecation notice to downstream users.

Had a further idea here which is a slight BC break, but means the API becomes cleaner.

1. Deprecate transform() entirely.
2. load() becomes responsible for also providing the default sort order for the results, ie. fold generateIndexAndSort() into load(). The BC break here is that the keys returned from load() will no longer be mlids. Or maybe we can use a custom sort method that doesn't change the keys?
3. Add an explicit checkAccess() method. MenuForm (or other callers that don't care about access) don't call this. Callers that want to do different access checks optionally call this if they need to, and can provide their own additional checks.

This means the generic calling case becomes:

    $tree = $this->menuTree->load($menu_name, $parameters);
    $tree = $this->menuTree->checkAccess($tree);
    return $this->menuTree->build($tree);

This splits the API much more cleanly into load > access check > build phases. Maybe we add hooks at the load and access check phases for contrib to easily make changes at both these points?

I 100,000% support splitting out access checking.

I'm on several projects where permissions checking bogs down menu performance and is irrelevant since all that matters is published.

MR!15727 is a rough implementation of #71, partially assisted by Claude Code.