This is a follow-up to: #3537721: Admin Toolbar Search: Remove dependency on Admin Toolbar Tools
Problem/Motivation
After removing the dependency on Admin Toolbar Tools and the recent work carried on refactoring the Admin Toolbar Search module, it appeared the module still has a dependency on Admin Toolbar.
Theoritically, there is no reason the Admin Toolbar Search module could not work properly without the Admin Toolbar module.
See dependency in the code:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
Additionally, it could really help further clarifying the role of each module:
- Admin Toolbar: Provides the JS dropdown menu features.
- Admin Toolbar Search: Provides a quick JS autocomplete search filter through the links in the page.
Looking at the code, there doesn't seem to be any reason this dependency would be necessary, in the sense, the module does not seem to use any feature from the admin_toolbar module, whether, JS, CSS, images, etc...
In other words, the impacts and changes that could be required would be very limited, while still allowing to decouple the modules to provide more flexibility.
The idea behind that is to try to make the code packages as granular as possible, with only the dependencies that are really required in code.
Steps to reproduce
Enable the admin_toolbar_search module ==> Requires admin_toolbar to be enabled.
To uninstall the admin_toolbar module, the admin_toolbar_search module needs to be uninstalled first.
Proposed resolution
Replace the dependency on admin_toolbar with core toolbar.
Impacted files:
admin_toolbar_search.info.yml: Update dependency constraint.
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
admin_toolbar_search.libraries.yml: Removed dependency on library
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
AdminToolbarSearchTestBase
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
Enable the 'admin_toolbar' module for the tests.
AdminToolbarSearchSettingsFormTest
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.1/admin_tool...
Probably to be addressed in a separate issue: Review if we could try to move check for module's tabs links to its own Functional test class in Admin Toolbar, instead of admin toolbar search.
This would allow dropping test depency for admin toolbar tools/admin_toolbar and test the standalone features of the module.
Add a FunctionalJavascript test class to test the module without admin_toolbar:
AdminToolbarSearchTest.php
or adapt new ones refactored in other issue/ticket.
Enable admin_toolbar_search and search for certain keywords and combinations.
The rest of the code should stay unchanged, in particular all the Automated tests and the module should work very well with the current dependencies.
Remaining tasks
User interface changes
When enabling the admin_toolbar_search module, the admin_toolbar module will no longer be enabled at the same time, but users will have to enable/select it manually as well.
Before:
Select admin_toolbar_search to be enabled ==> Enabled admin_toolbar and admin_toolbar_search.
After:
Select admin_toobar_search to be enabled ==> Enabled admin_toolbar_search.
Select admin_toobar_search and admin_toolbar ==> Enabled admin_toolbar and admin_toolbar_search.
Select admin_toobar_search and admin_toolbar_tools ==> Enabled admin_toolbar, admin_toolbar_tools and admin_toolbar_search.
API changes
Data model changes
Issue fork admin_toolbar-3554658
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 #3
dydave commentedQuick follow-up on this issue:
All the changes detailed in the issue summary have been implemented and described in the merge request MR !187 above at #2.
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.
More work is probably needed on tests coverage but could probably be addressed in follow-up issues, for example, for some refactoring or adding new test classes.
This merge request and issue account for the bare minimum required to remove the dependency.
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
ressaThis was a very interesting experience, to only have the Admin Toolbar Search module installed :)
But it works well, by itself -- though it's a bit limited, since only the eight top level menu items are available. I can then individually install the Admin Toolbar module additionally, and it works as it normally does. More granularity and flexibility is always a good thing, so thanks for raising this idea, and providing a solution as well @dydave!
Comment #6
dydave commentedThanks a lot @ressa for taking the time to test and review this issue as well! 🙏
I knew you might be interested to see how the module could work with only the core toolbar module enabled. 😉
Following your confirmation, I rebased the MR and fixed an issue with a failing test missing a dependency on 'admin_toolbar'.
I thought about potentially adding a standalone test class in this MR, but there are already a few refactoring tickets pending, with a lot of moving pieces around admin_toolbar_search tests, so I thought we might want to come back to this a bit later, once other major refactoring changes could have settled and created the follow-up task: 👌
#3556902: Admin toolbar search: Improve automated tests
Since all the jobs and tests were still passing 🟢, I went ahead and merged the changes above at #5. 🥳
Indeed @ressa, the idea behind this is maybe down the road, to allow the module to expose some kind of hook or plugin manager so it could collect extra links added by other modules, for example.
Or it could maybe search in the links from the core navigation instead of the Toolbar... (?!) 🤔
But this is an initial step towards helping to extend module's features and better integrate with other modules. 🤞
Marking this issue as Fixed, for now.
Still more pending tickets in the queue! 👍
Thanks again very much for your great help @ressa! 🙏
Comment #8
ressaGreat @dydave, thanks for yet another improvement on the Admin Toolbar module!
And great thinking ahead, that the flexibility may allow connecting to other modules, for example Navigation, and others as well in the future. I really appreciate your efforts with getting Admin Toolbar, as well as Block Class in tip top shape!