Follow-up to #2855786: Menu items that link to an empty administration page are not hidden
Problem/Motivation
For users with limited permissions (i.e. content editors), enabling the "access administration pages" permission makes a lot of menu items appear that they don't have access to, which show a "You do not have any administrative items." message.
Having all these links that go nowhere is not a very good UX:
Steps to reproduce
- Cleanly install Drupal using the
standard
profile - Log in as user 1
- Visit
/admin/people/create
- Create a new user and grant them the Content editor role
- Open a private browser window
- Log in as the newly created editor
- Once authenticated, change the menu to its vertical orientation mode (either by clicking the toggle button on the right side of the toolbar or by shrinking your window width until the menu hits its responsive page break)
- Expand the Configuration menu item
- Observe that the System menu item is visible (it should not be) and click it
- You will be directed to an empty system admin menu block page with the text
You do not have any administrative items.
Proposed resolution
Don't show menu items that go to empty administration pages by checking if children are accessible in addition to checking the "access administration pages" permission.
Remaining tasks
Patch
Review
Commit
User interface changes
Menu items to empty administration pages will be hidden.
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#15 | drupal-disable-empty-system-menu-page-links-2856666-15.patch | 2.02 KB | gabesullice |
Issue fork drupal-2856666
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:
Comments
Comment #2
stefan.r CreditAttribution: stefan.r commentedComment #13
LendudeChecked this with an 'editor' user in a clean Umami install. I only see links that I have access to, so this seems to have been fixed since this was opened.
If you feel this is still an issue, please provide some steps to reproduce this on a current version of Drupal. Thanks!
Comment #14
gabesulliceI was able to reproduce this on the tip of the
9.5.x
branch. I updated the issue description with steps to reproduce this bug from a clean installation.Comment #15
gabesulliceFailing test.
Comment #16
gabesulliceI spent some time trying to get this working by adding an additional access check to all of the routes which use the
SystemController::systemAdminMenuBlockPage
controller, but I don't think that's going to be a fruitful approach. It's difficult to check access to menu links from that controller and after digging in, it felt a little hacky.Instead, I think it might be easier and generally useful to add a new flag to menu items to indicate whether the item should or shouldn't be shown when it doesn't have any visible children. Tentatively, I'm naming this flag
transitive
with a default value ofFALSE
to maintain BC. The concept is that menu links likePeople
andDevelopment
could be marked "transitive" since their only purpose is to transit users from a grandparent page to a child page. Thus, if they have no visible child links, they can be safely elided.I'm trying to do that in
MenuLinkTree
, but I'm running into a hiccup because, at some point in the process of building a menu link tree, a particular item may not have any visible children even though one will be added later. Thus, my current code is removing items that it shouldn't because it doesn't "know" about the child that doesn't exist yet. I think I just have to spend more time with Xdebug to figure out where these missing child links are added.A drawback of the menu link tree based approach is that the system menu block pages would still be accessible, even without any links, if a user visited the right URL directly. That feels like a separate issue that could be handled in a follow-up though. All we'd need to do would be to throw a 403 or 404 HTTP exception in the controller instead of printing out the
message.Comment #17
gabesulliceComment #19
gabesulliceComment #20
gabesulliceNeeds work for an update hook to add the
transitive
column to existing sites.Comment #21
gabesulliceOkay, I added code to update the database schema on existing sites. I was expecting to write an update hook to do this, but it seems like the default menu tree storage service handles its own schema changes, so I made my changes in there instead. This probably needs special attention.
Needs review for tests.
Comment #22
gabesulliceNeeds work to update the toolbar tests, which makes sense.
Comment #23
gabesulliceTo follow up on this: the issue was in the toolbar module. It does a quirky thing where it only loads the top level of the admin menu, then it loads the subtree of each of those top level items separately, rather than loading the whole tree at once. This explains why I wasn't seeing any children when I was step-debugging the first load of the admin menu (it was only loading a shallow depth).
To clarify: it makes sense because I'm fiddling with the menu and the toolbar tests probably make some assumptions that don't hold any longer.
Comment #24
lauriiiComment #27
omkar.podey CreditAttribution: omkar.podey at Acquia commentedComment #28
tim.plunkettComment #29
tedbow@gabesullice et al, great work on this but...
I have talked with @tim.plunkett and @lauriii and I (we?) think we should close this issue in favor of #296693-257: Restrict access to empty top level administration pages from 2008 which has different approach to solve the same general UX problem. See my linked command but basically I think route access a better way to solve this then menu items.
Comment #30
tim.plunkett+1