Look up module name as well, when searching with Admin Toolbar Search

Working on it.

Super helpful, friendly and nice reply @ressa!

Thanks everyone for getting the work started on this issue 🙂

Overall, I think you've got the right idea here 👍
Great work @aryan-singh, thanks a lot! 🙏

But there is still work needed, indeed....
Good catch @Gogowitsch, PHPSTAN is not happy ☹️
PHPSTAN Errors to fix:
https://git.drupalcode.org/issue/admin_toolbar-3549068/-/jobs/6913650#L51

The tests currently seem to crash here:
https://git.drupalcode.org/issue/admin_toolbar-3549068/-/jobs/6913657#L146
or as indicated by @ressa:
https://git.drupalcode.org/issue/admin_toolbar-3549068/-/pipelines/62985...

System output
Drupal\Tests\admin_toolbar_search\Functional\AdminToolbarSearchSettingsFormTest::testAdminToolbarSearchSettingsForm
Behat\Mink\Exception\ExpectationException: The string "[]" was not found anywhere in the HTML response of the current page.

/builds/issue/admin_toolbar-3549068/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/admin_toolbar-3549068/vendor/behat/mink/src/WebAssert.php:363
/builds/issue/admin_toolbar-3549068/web/core/tests/Drupal/Tests/WebAssert.php:559
/builds/issue/admin_toolbar-3549068/admin_toolbar_search/tests/src/Functional/AdminToolbarSearchSettingsFormTest.php:202

Error: The string "[]" was not found anywhere in the HTML response of the current page.

Corresponding to line:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

See the inline comment:

    // Test the extra links search route does not return an access denied error,
    // but an empty array, even with the 'admin_toolbar_tools' module enabled,
    // since the modules adding the extra links are all disabled in this test
    // ('field_ui', 'node', 'media', 'menu_ui', etc...).
    $this->drupalGet('/admin/admin-toolbar-search');
    $assert->responseContains('[]');

Since you've added the modules to the links array... it is probably not empty anymore....
In other words, from what I understood reading quickly the changes: The links array should never be empty and always contain at least the modules names.... something like that...
So the tests would probably need to be adjusted accordingly to take into account the new logic.
That's what tests are here for ==> Enforcing the logic implemented in module's code and merge requests. 🥳

Other impacts on tests would probably be expected... Since this code adds a substantial new piece of logic to the Admin Toolbar Search module.
==> Needs to be integrated with module's tests.

I have not tested this manually myself... just reviewing the merge request and tests results.

Could there be any way to associate the module name with a URL without having to load the info YAML file and getting the configure link? 😅
Seems pretty heavy ... Any way with JS? Through the HTML code in the admin menu toolbar?
(View source and search for glossify and see if anything comes out in the menu links in the admin toolbar, for example)
See JS code: https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
I don't really know yet, since I have not started looking any further at this issue.

Otherwise another option would be to add ExtraLinks for Text Formats, which would fix the issue in this particular case for Glossify, but would not be exactly the same type of feature.
Usually, module names can be found in settings forms links, configure links, etc... Added to the admin menu in the toolbar.... So they should get picked up by the Admin Toolbar Search, with JS.

This is not in my list of priorities for now, but I'll try to keep an eye out and help giving advice, reviewing or providing suggestions if anybody fancies working on this. 🙂

Let us know if you have any questions or would need any help on this issue, we would surely try answering as soon as possible.
Thanks in advance! 🙏

Thank you @ressa for helping me out in finding actual errors.I have solved the phpstan errors that were there in my code.I’ll look further into fixing the PHPUnit test failure. In the meantime, if anyone has suggestions or a solution for the test issue, you’re most welcome to share.

Sry for the duplication. But the MR consists of the following:

The issue is that Admin Toolbar Search currently matches against the visible toolbar breadcrumb label and the route path, but not the module name itself. That means searches like Glossify can fail when the actual admin link is something generic like Formatter settings, because neither the label nor the path contains the module name.

The fix adds module names as extra search keywords for installed modules that expose a configure route. On the client side, those keywords are merged onto existing toolbar links by URL, so the visible suggestion label stays the same and duplicate results are avoided. In practice, this means users can now find a module’s config page by searching either the page label/path or the module name.

I also added test coverage for this case with a small fixture module whose admin link title does not contain the module name, and verified the functional BrowserTestBase coverage locally. The existing JS search test was updated to cover this scenario as well.

Thanks a lot Muneeb (@mukhtarm) for taking the time to look at this issue and for contributing a merge request, it's greatly appreciated. 🙏

I tried rebasing your changes in MR !210 but I was unable to really understand or clean-up a lot of the unrelated changes there...
I'm not sure exactly, but it looked like the MR was reverting some changes we made more recently in forms and such...

If you don't mind, I allowed myself to hide your MR for now, so maybe you could take another look at it and help cleaning it up a bit so the review could be a bit easier.

Therefore, I thought it would be best to try going back to where I left off the initial merge request MR !183 at #10, which should fix the issue initially reported.

I tested the changes again locally with the Glossify module and it seemed to be work fine.

So mostly: I added another commit to help with the tests coverage, see the updated documentation in the merge request, with the acceptance tests.

Otherwise the rest of the logic is pretty much the same, so at this point, it would be good if would could have another Functional review if possible, of the feature to see if it is working as expected, see a small screenshot of the tests results locally:

Therefore, moving issue to Needs review as an attempt to get more reviews and testing feedback on MR !183.

Requalifying issue as Feature request since this is more a new piece of logic rather than fixing an existing one.

Any questions, comments or reviews would be greatly appreciated.
Thanks in advance!

Fantastic! Super nice @ressa, as usual, for the super fast and positive feedback! 🤩

I gave this quite a lot of tests locally yesterday, plus I'm quite confident of the PHPUNIT Tests coverage, so your confirmation was enough and since all the jobs and tests were still passing 🟢, I went ahead and directly merged the changes above at #18. 🥳

This is a great new addition to the module (thanks for sharing with us again @ressa) and will hopefully help a lot of users. 👍

Let's keep going through the issue queue and see what we could stacked up in the next 3.6.4 release. 👌
(before switching over to 4.x)

Apart from a bit of documentation, which should be addressed in another existing issue, since I don't see any more actions in this one at this point, marking it as Fixed, for now.

Feel free to let us know if you have any questions or concerns on any aspects of the recent code changes or the project in general, we would surely be glad to help.
Thanks again everyone for the great help on this issue! 😊