Problem/Motivation

I'm profiling a site that has a large (several hundred/low thousands) number of disabled menu links in its main menu - my understanding is the links are there to support menu breadcrumbs but should never be shown in the menu block.

MenuLinkTree::buildItems() removes disabled items from the tree.

However SystemMenuBlock doesn't call onlyEnabledMenuLinks() when building the initial tree, and build/build/Items runs only after menu tree link manipulators have run. As a result, access checks are called on those hundreds/thousands of disabled menu links, which then get filtered out anyway.

This site may end up overriding the menu block to optimize/customize some other things, but I don't see why we'd load disabled menu links for a visitor-facing block like this anyway.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3529163

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !12332

catch’s picture

catch
he/him
Primary language English
commented
Category: Feature request » Task
Status: Active » Needs review

This is a one-liner apart from the comment changes and performance test update.

While the performance test only shows that the query is updated rather than the improvement itself (the Umami main menu doesn't have any disabled items) that's enough to ensure that this won't regress accidentally.

I think having this many disabled menu links is not very common, but since we already filter the links out when building the tree after manipulators run, there's going to be no functional change for sites.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

This breaks workspaces menu integration - e.g. menu links that only exist in a workspace don't get shown in the workspace.

There are two ways to fix that:

1. Add an optional bool argument to ::onlyEnabledLinks() so that the condition can be removed in WorkspacesMenuTreeStorage::loadTree() - essentially undoing the optimisation added here, but only when viewing a workspaces.

2. Bring wse_menu into core, which makes the entire tree storage workspace-enabled.

I'll see if I can get #1 going quickly.

catch’s picture

catch
he/him
Primary language English
commented

Another way to deal with the workspaces issue would be to only add the ::onlyEnabledLinks() call in SystemMenublock if workspaces isn't enabled, but that would require a ::moduleExists() check, however we could then have a follow-up for workspace-enabled menu trees which would eventually remove that @todo.

smustgrave’s picture

smustgrave commented

Appears to be 1 open thread @catch thoughts on mstrelan comment?

catch’s picture

catch
he/him
Primary language English
commented

Replied on the MR - I think it would be a problem if that was possible, but I don't think it is for the menu block because there's no way alter this.

smustgrave’s picture

smustgrave commented

Left 1 small question on the MR. Rest looks good.

smustgrave credited mstrelan.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Applied the suggestion, rest LGTM :)

catch credited amateescu.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review

I discusssed this with @amateescu in slack and he's not convinced that this is safe to do.

The workspaces code operates on any code that's loading a menu tree, not only the system menu block, so it's removing the condition in cases where it could have been added from elsewhere. Unless we add some kind of explicit alter hook for the system menu block itself, it's not going to be straightforward to target this only at that block.

Moving back to needs work and adding the subsystem maintainer review tag (even if the subsystem is more workspaces than system module.

amateescu’s picture

amateescu commented

One solution could be for the system menu block to add a "fake" condition to the parameters (like system_menu_block = TRUE), and workspaces could check for that parameter condition before removing the enabled one.

catch’s picture

catch
he/him
Primary language English
commented

#14 should work and would allow other modules to tweak the output of the plugin without completely replacing it.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.