Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
This issue is a follow-up to #3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks where the 'Manage Permissions' was found missing and non standard menu labels in Admin Toolbar Search suggestions.
The 'Manage Permissions' link is added properly to menu links in the toolbar with ExtraLinks, but not in the SearchLinks which are displaying extra links that are not displayed in the HTML of the page, see:
Before:
After:
The label for Menus Search links does not currently follow the same standard as other entity types, see how the screenshot above compares with the screenshot below:
Before:
After:
The current label: Menus > Shulewuj
Should be: Menu > Shulewuj > Edit
Steps to reproduce
1 - Install the Admin Toolbar Tools and the Admin Toolbar Search modules.
2 - Login with an Administrator user to be able to see the 'Manage Permissions' link.
3 - Configure the max bundle number to 2 on the settings form page (admin/config/user-interface/admin-toolbar-tools)
4 - Add a test content type so that there are at least 3 content types.
5 - Flush the caches.
6 - Search in the Admin Toolbar Search for the content type created at 4 by typing in the label.
7 - Confirm the 'Manage Permissions' suggestion is missing from the search results.
8 - Search in the Admin Toolbar Search for the 'Main navigation' menu by typing in 'navigation'.
9 - Confirm the displayed suggestions labels do not match other search links standards.
Proposed resolution
Modify the class 'SearchLinks' to fix these issues:
1 - Add the 'Manage Permissions' entity bundle link to the search suggestions, based on the existing code used to add other search links, see for example:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
2 - Refactor the code for the 'menu' entity search links labels, based on the code used for other entities (content types, for example), see $label_base:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
Feel free to let us know if you have any questions or concerns on any aspects of this issue or the project in general, we would surely be glad to hear your feedback.
Thanks in advance!
| Comment | File | Size | Author |
|---|
Issue fork admin_toolbar-3552177
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:
changes
Comments
Comment #3
dydave commentedQuick follow-up on this issue:
I think it is better to isolate these changes so they could really be focused on testing these specific search suggestions.
All the changes detailed in the issue summary have been implemented and described in the merge request MR !181 above at #2.
Overall, just copied code from different sections of the
SearchLinksclass and adapted to match with the corresponding search links.Resulting search suggestions, after applying the changes from the merge request:
Manage Permissions search link suggestion:
Standardized 'Menu' search link suggestions labels:
There are no tests in this merge request, since they are added in related issue #3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks.
Since all the tests and jobs still seem to be passing 🟢, moving issue to Needs review as an attempt to get more testing feedback and reviews.
Feel free to let us know if you have any comments, questions or concerns on any aspects of this issue or the suggested changes in the merge request, we would surely be glad to help.
Thanks in advance!
Comment #4
ressaThanks for looking at this one @dydave, another complex one to test :)
The first task, to show Permissions, I can confirm, the patch fixes that.
The second though ("The label for Menus Search links does not currently follow the same standard as other entity types") I am not sure ... Perhaps you can clarify the current wrong format, and the desired format?
Another observation:
"Structure" is not included as first part of the links displayed in the search field, if "Maximum number of bundle sub-menus to display" is set to "2", for example for the "Main" menu. Also, "Menus" change to "Menu". Shouldn't it always be "Menus", to reflect the actual Admin Toolbar menu items? (indented to highlight differences):
Comment #5
ressaI had another look at the second task ("Refactor the code for the 'menu' entity search links labels ...") and after comparing the two images, and the text side-by-side, I now see that the MR does fix this as well, so the issue is completed 🙂
Perhaps the "Structure"-missing ting, when you change "Maximum number of bundle sub-menus to display" value could be for another follow-up issue? Also, it seems to only happen with the reduced sub-menus ("3 - Configure the max bundle number to 2 [...]") and I am not sure if that was even a necessary step for this issue? But anyway, it has now been seen ... 🧐
Comment #7
dydave commentedThanks a lot @ressa for clarifying the issue summary! 🤩
It's so much clearer with the before and after images side by side.... I'll make sure to remember this method next time, it's very helpful! 👌
Great investigation work, as usual!
I think you probably understand a bit better now what the
'max_bundle_number'does 😅I tried to give a few very basic examples to demonstrate how this feature works out-of-the-box, with minimal setup steps, in particular with supported entity types that already have a few bundles created by default by Drupal core, such as
'menu':5 undeletable menus: 'account', 'admin', 'footer', 'main', 'tools'
So setting the
'max_bundle_number'to a value lower than 5, for example 2, would allow immediately reflecting some of the impact of the'max_bundle_number'feature on theExtraLinksgenerated byadmin_toolbar_tools:Under "Structure > Menus":
/admin/structure/menu'All menus'link should be displayed first in the list.'Add menu'link should be displayed second.See: #3508772: Automated tests: Improve tests for AdminToolbarToolsSettingsFormTest
and this behavior is supposed to be the same for all other supported entity types, such as Block content, Content (node), Media, Comment, etc...
Same thing, with the 'All types' and 'Add type' links under Structure, with a max number of entity bundle links displayed, etc...
However, the current implementation of this feature in
admin_toolbar_toolsdoes not seem to be very clear, complete or consistent across entity types or supported modules..../admin/content) menu is not limited by the'max_bundle_number'.From a user standpoint, it is difficult to understand that the links under "Structure" would be limited to 2 (for example), but under "Content", all the types would be displayed...🤔
Overall, it really adds a non negligeable amount of complexity to the modules (search and tools) and I personally think this feature should be re-evaluated at some point (with specific tickets of course).
This is extensively discussed in the follow-up ticket (unblocked by this one):
#3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks
(See the related issues in the issue summary)
On top of that, different entity types have different methods for generating extra links, resulting in a lot of code duplication and great challenges to keep the integrity of the feature across all code files.
In this particular case:
admin_toolbar_tools: ExtraLinksadmin_toolbar_search: SearchLinkshave very similar features, but different display output methods for links and suggestions.
When the "Manage permissions" link was added to the ExtraLinks class, it was easy to forget it should be added to the SearchLinks class as well.
So there is definitely still a lot of room for refactoring or improvement and that's why full Tests coverage of this feature is essential if we would like to go any further to fix these problems. 👍
Yes, definitely! I have noticed that 😅
I have started preparing the next round of tickets for the refactoring of module's Javascript
admin_toolbar_search.jsand the SearchLinks class, which should add a lot of improvements to the User Interface (UI) of the toolbar search field. 👌It is on my list of objectives in terms of refactoring, after PHP and the Tests, we fix module's JS.
But at least at that point, we should already have a much more complete set of Automated Tests for the PHP and the JS code, on which we should be able to strongly rely to make changes and implement new features. 😎
I went ahead and merged the changes above at #6 which should now unblock:
#3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks
impacting only Admin Toolbar Tools
FunctionalTests class files.👌Marking issue as Fixed, for now. 🙏
Thanks again so much for your great help testing and reviewing these complex issues!
Comment #9
ressaThanks @dydave for a fast response and for creating the issue and the MR in the first place, it's so great to land another one!
Your images were really helpful, it was just that the changes between them were so small, and easily missed ...
And yes, I get max_bundle_number now 😅
But like you write, it's oddly inconsistent, and I also noticed that the looong list of Views are unaffected. So I agree, that if it adds a fair amount amount of complexity to the Search and Tools code, that removing it could be evaluated in a follow up issue. A lot of test code could also be removed I suppose, and further simplify maintenance. I left a comment about it in #3261664: Bundle sub-menu maximum not honored.
Sounds good! Thank you @dydave for keep on going full steam ahead with the Admin Toolbar improvements, I very much appreciate it.