Allow SystemMenuBlock tree manipulators to be altered

Quickly skimmed patch and it looks awesome!!!

That works and covered with tests

I think menu UI should be continue to display all items

The issue summary is full of TBD and there is no change record.

Updated IS with current state

Looks better name could be more block-centric hook_system_menu_block_manipulators_alter instead of hook_system_menu_tree_manipulators_alter because we alter manipulators for block only

@andypost, thanks for the heads up about this issue.

This approach would solve my problem raised in #2466553-43: Untranslated menu items are displayed in menus as contrib modules such as Superfish and Menu Block that extend SystemMenuBlock and override the build() method could also call this alter hook.

Here are some other related contrib issues raised by @id.tarzanych that would benefit from this hook.

#2834616: Make menu tree manipulators alterable
#2834619: Integrate with Superfish module

But I also feel something like a generateMenuTree method that did just the following and included the alter hook would mean that contrib and custom modules would not need to call the alter hook separately as well as avoid having to duplicate the other lines of code to generate the tree.

public function generateMenuTree($menu_name, $parameters) {
  $tree = $this->menuTree->load($menu_name, $parameters);
  $manipulators = [
    ['callable' => 'menu.default_tree_manipulators:checkAccess'],
    ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
  ];
  $this->moduleHandler->alter('system_menu_tree_manipulators', $manipulators);
  $tree = $this->menuTree->transform($tree, $manipulators);
  return $tree;
}

The build method would then call this method.

@andypost, @tuutti, what do you think?

No longer applies.

Re-rolled.

I'm wondering whether the better approach would be to actually make it easier to extend the specific block class, and well, add a method which returns the manipulators, that should be used.

#14 certainly makes sense, IMO. To go even further, IMO we could make the list of manipulators part of the configuration, but not expose it in the UI. Instead we could make it a '#type' => 'value' element, and then people could either use a form-alter if they want to give site builders any options to change something, or people can just edit the configuration directly if that's sufficient for them.

#14 & #15 both does not solve contrib issue when multiple modules needs to alter one block

Basic example is domain + superfish

CR filed https://www.drupal.org/node/2940082

Not endorsing the patch but #18 was just a random fail ;-)

Basic example is domain + superfish

I'm curious, why is it not enough to change the templates etc. to get superfish working? I think documenting something like this in the issue summary would be a great step forward.

There was no answer to the questions in #14 and #21

Extending block lass is no go cos when you wanna enable domain module & allow it to be disablable you will need to update/remove all block configs to change plugin id
I thing duplicating blocks to provide a set of manipulators that should be site wide is bad idea

Updated summary with explanation and a list of contrib which affected

#14 is that if you have more then one module which overrides block class like https://github.com/skilld-labs/domain_menu_access/blob/8.x/domain_menu_a... then only last wins - no way to apply more then one override this way
As example #2847313: Discussion about merging the module with Menu block current language - menu_block & menu_multilingual both require to inject own manipulators ...and fail

#21 superfish - at the template level there's not enough information and it anyway will have collision if any other contrib will alter block, basically module require to manipulate menu structure instead of templates. Also it will have a huge performance impact when whole menu will be passed to template which supposed to have different structure.

This collisions affects following contrib modules

Implement a new hook that allows menu tree manipulators to be altered.

Hooks don't prevent collisions.

Why can't all those modules provide alternative menu blocks? For example, "Superfish menu block". If they want to, they can subclass the existing logic. We could refactor \Drupal\system\Plugin\Block\SystemMenuBlock::build() to not hardcode a list of manipulators, but instead have a method return those. Then they'd only need to subclass this method.

So, concerns:

1. new hook
2. not convinced the actual problem is solved

Hooks don't prevent collisions.

@Wim Leers I mean "collisions" in block classes - each module replace SystemMenuBlock with own class implementation but requirement is to provide more then one manipulator

For example I need to apply "domain manipulator" & "superfish" same time to the main menu & domain + menu_per_role to sidebar block https://cgit.drupalcode.org/menu_per_role/tree/src/MenuPerRoleLinkTreeMa...

instead have a method return those

Again they will replace block without ability to extend list of manipulators, also you can not override block depending on its plugin ID (which is extra feature to have)

hook will allow to have more then one manipulator to be added to extend core "hardcoded" list

Instead of hook maybe better to implement event?

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -159,6 +171,8 @@ public function build() {
+    $this->moduleHandler->alter('system_menu_tree_manipulators', $manipulators);

would be great to add the block instance here as well to allow alter manipulators per block

Here is an alternative suggestion. Let's move this list into something which is statically configureable, let's say a container parameter. Modules then can add new ones to all menu queries, without having a runtime dependency.

@dawehner why you wanna make the list static? container param is good choice btw ...but still no ability to apply different set of modifiers per block instance. Event looks good compromise for speed instead of hook and static set

Needs a re-roll due to #2594425: Add the option in system menu block to "Expand all items in this tree" and I hope keeps this in mind #3018863: Make SystemMenuBlock's new constructor argument dependency optional

Re-rolled on latest dev. Always using an alter hook to modify the menu tree manipulators.

Added a context in the alter hook as suggested in #28

I believe this is a much needed feature, however it seems to me that altering of manipulators should be done more centrally within \Drupal\Core\Menu\MenuLinkTree::transform() otherwise this only works for menus rendered via SystemMenuBlock or extending blocks. This will fail to cover use cases such as rendering a menu directly within a theme / preprocess function. I've created #3091246 for this approach

I would also be in favor of an event for this instead of an alter hook.

I just needed to implement this and I was honestly surprised it wasn't possible. Looks like a must-have feature to me. Seeing this has been in the works for 3 years already, I thought I'd share what I found.

After reviewing both issues, I went with the event-based solution from @malcolm_p in #3091246. Events are the way forward, not adding new hooks. In my case I didn't even need to add a new manipulator, but just modify the $tree in my event subscriber. Very useful.

Not having this issue resolved is not only badly affecting multiple contrib projects, like the ones already mentioned, but it's making people find ways to do it however they can. Take this for example: #2535162, where the proposed solution is to override the menu.link_tree service. What happens if all the contrib projects do this?

The problem here is inheritance. Wondering if we allowed block classes to be decorated this problem would be solved. Along with separating the manipulators to its separate method.

Hmm yes we need maintainer input on whether to go whether the alter hook solution here, or event solution at #2854013: Allow SystemMenuBlock tree manipulators to be altered.

Just a thought of mine 🤔Maybe it's a bad idea ;)

Couldn't this also be realized with a Plugin or tagged Service based approach?

Having a new plugin type, with a service that loads them. Which then can be used inside the SystemMenuBlock. And also for custom developments. Possibly also add a property types or however you may call it, in case a module needs it's own manipulators, that should not be used elsewhere. Would probably also need a weight/priority to know in which order to execute them.

Something like this:

/**
 * @MenuTreeManipulator(
 *   id = "manipulator_my_manipulator",
 *   label = @Translation("Some menu tree manipulator"),
 *   weight = 12,
 *   types = {
 *     "default",
 *     "my_manipulator_type",
 *   }
 * )
*/

However this is probably not really good regarding BC?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thank you @lauriii for the link to the other issue in #49.

I think the solution proposed there is better than what we've done here.

I suggest we can close this current issue and continue work on the other.

What do you think?