🇫🇷France @dydave

Thanks everyone for all the help and explanations!

Looks like this has been addressed in:
#3468976-17: Untangle branch confusion and document missing features in 3.x →

with the 3.x and 4.x branches both having stable releases with automated testing pipelines in place.👌

Since there has not been any more activity on this issue for more than a year, it is probably safe to assume it could be marked as Fixed, for now.

Feel free to let us know if you have any questions or concerns on any of the recent code changes or the project in general, we would be glad to help. 😊
Thanks again very much!

Added related topics:

• 💬 Moving from V2.0 to V3.0 Closed: works as designed
• 📌 2.x release vs 3.x release: Which should I use? Active

3.x: https://git.drupalcode.org/project/block_class/-/commits/3.0.x
Upgrading from Drupal 7:
Supported to D9, D10, D11 ==> Use the 3.x branch.
Very simple, lightweight, fully tests covered, easy to use
Does one thing only: CSS classes in a plain text field store as a string.
Very stable.

4.x: https://git.drupalcode.org/project/block_class/-/commits/4.0.x
Latest features and developments.
Experimental ==> Use the 4.x branch.
Lots of features: CSS classes, ID, Attributes
Much larger code base, limited tests coverage, limited user documentation.
Does many things but is less easy to use and is still a bit unstable (WIP).

It is possible to upgrade from 3.x to 4.x but not the opposite.

The 3.x branch still provides an intermediate step for sites upgrading from Drupal 7 not necessarily comfortable using the most recent 4.x development branch of the module at the start.
But an upgrade can still be considered after. 👌

Development has now moved to the 4.0.x branch, with related more recent plan:
🌱 Plan for 4.0.2 Active

Marking issue as Closed (outdated).

Thanks again everyone for all the great work on this issue.

There is no plan anymore on working on this issue since the Drupal 7 branch is not maintained.

Marking issue as Closed (outdated).

Thanks again everyone for all the great work on this issue.

This was addressed more recently in 🐛 Autocomplete widget not working as expected Active with merge request MR !67, where the pieces of code reported in the issue summary and in the merge request, have been removed from the file in more recent versions, see:
https://git.drupalcode.org/project/block_class/-/merge_requests/67/diffs...

Marking issue as Closed (outdated), for now, since these changes are not based on the most recent versions of the module.

Thanks again very much for the great work on this issue!

Sorry for the late reply on this task and thank you very much Chris (@chris matthews) for the wording improvements! 🙏

Just rolled in the patch from #2 into MR !68.

Since these changes had already been reviewed, tested and pending for a very long time, I went ahead and merged them above at #10 without further delay 😅

We will definitely need to get back on other Documentation related tickets, in particular revamping the Project page:

• 📌 Improve Block Class's project page Active
• 📌 Update Project description Needs review

There is still quite a lot of work needed to stabilize the 4.x branch...

Marking issue as Fixed, for now.

Feel free to let us know if you would have any questions or concerns on any aspects of the latest code changes or the module in general, we would surely be glad to hear your feedback. 😊
Thanks in advance!

Bringing this one back to the top of the pile, since module's project page and documentation in general would definitely need some work, refresh and updates to match with newer versions features.

For a more recent inventory of the changes needed, see: 📌 Update Project description Needs review

Since this is starting to be quite old, without any reply or related requests from other users, for several years, we could maybe consider this was addressed in a different issue (?!).

If this request is still current, a new ticket should be created, for more recent versions of the module and Drupal core.

Marking issue as Closed (outdated), for now.

Thanks again everyone for all the great work on this issue.

Since this is starting to be quite old, without any reply or related requests from other users, for several years, we could maybe consider this was addressed in a different issue (?!).
See more recently: 📌 This ID: is already in use by another block Active

I found the idea suggested in the issue summary interesting though:
Do not enforce unique ID validation and leave it to the admin/user's informed decision (with warning, message, etc...).

So if this request is still current, a new ticket should be created, for more recent versions of the module and Drupal core.

Marking issue as Closed (outdated), for now.

Thanks again everyone for all the great work on this issue.

There is no plan anymore on working on this issue since the Drupal 7 branch is not maintained anymore.

Marking issue as Closed (outdated).

Thanks again everyone for all the great work on this issue.

Thank you very much for raising this issue.

Sorry for this late reply, but unfortunately I was unable to reproduce this with current version of Drupal Core, tested with:

• core: 11.2.5
• block_class: 4.0.x (dev version)
• php: 8.3

Is this still issue still happening with more recent versions of the module and Drupal Core?

Could anyone please provide clear testing steps to follow, to reproduce the issue, if possible with more context, versions of module/core, etc... ?

Any additional information that could help us reproduce the problem would be greatly appreciated.

Additionally, since there has not been any comments on the issue for half a year, we could also perhaps assume the problem initially reported might have been resolved with some other changes (from other tickets) (?).

Switching status to Postponed (maintainer needs more info), for now, until the work expected in this ticket could be clarified.

Feel free to let us know if you would have any questions or concerns on any aspects of this comment or the module in general, we would surely be glad to help.
Thanks in advance!

Thank you very much for raising this issue.

Sorry for this late reply, but unfortunately I was unable to reproduce this with current version of Drupal Core, tested with:

• core: 11.2.5
• block_class: 4.0.x
• php: 8.3

Is this still issue still happening with more recent versions of the module and Drupal Core?

From what I have seen, the config variable 'block_classes_stored' is properly declared as a sequence of string and initialized as an empty array, so I can't seem to think of any reason the variable $block_classes_stored could be a string...🤔

In short: I am unable to reproduce this issue and from the look at the code think the problem should not occur again.

Could anyone please provide clear testing steps to follow, to reproduce the issue, if possible with more context, versions of module/core, etc... ?

Any additional information that could help us reproduce the problem would be greatly appreciated.

Additionally, since there has not been any comments on the issue for half a year, we could also perhaps assume the problem initially reported might have been resolved with some other changes (from other tickets) (?).

Lowering ticket priority back to Normal and switching status to Postponed (maintainer needs more info), for now, until the work expected in this ticket could be clarified.

Feel free to let us know if you would have any questions or concerns on any aspects of this comment or the module in general, we would surely be glad to help.
Thanks in advance!

Thank you very much for reporting this issue and for providing a solution in the code, it's greatly appreciated!

Thanks to #6 I was able to reproduce the behavior described in the issue summary.

I have tested the changes from the merge request and they also seemed to implement the correct behavior.

However, when I started investigating the code changes from the MR, I thought I might want to look for an equivalent example from Drupal core and found:
\Drupal\block\Controller\CategoryAutocompleteController::autocomplete()
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...

I thought: we migth as well align the module on the current implementations of autocomplete callbacks in Drupal core (similar types of fields), since the user experience and interactions with module's form fields would therefore stay consistent and would most likely be easier to maintain.

The autocomplete callbacks for CSS classes, Attribute keys and Attribute values were fully refactored based on: CategoryAutocompleteController::autocomplete().
I copied the function over, adapted it slightly, then kept the same callback functions but simplified.

I then looked into the BlockClassHelperService::blockClassFormAlter, which brought me to make the following changes:

- Removed unused services dependencies: I was wondering why there were so many dependent services and started searching for the properties in the class file. Then, found many properties and injected services were not used, so they were all removed with their associated code.

- I was a bit stumped to see example images in the repo, weighting almost 500KB, with almost no importance for module's features, included in the repo and thus downloaded with every single package request... 🤦‍♂️😖
These images were removed and uploaded in Block Class Project page files, hidden on the page, but with a URL on Drupal.org.
Took the opportunity to rework and simplify a few description texts at the same time.
 

After doing several round of tests locally and since all the tests of the MR were passing 🟢, I went ahead and merged the changes above at #13.

From my perspective, there is still a very important amount of work on the module 4.0.x branch to actually become stable...
It really feels like a very basic initial version that would need a significant amount of work and time to be improved so its features actually become exploitable to all users.

Let's try to keep improving overall module's code as we work on pending tickets.👍

Feel free to let us know at any point if you have any questions or concerns on any aspects of the latest code changes or the project in general, we would surely be glad to hear your feedback.
Thanks in advance! 😊

Thank you very much Paulo (@paulocs) for reporting this issue and providing a solution in code, it is greatly appreciated!

Great catch! 👍
Indeed, it doesn't really make sense not to be able to use the value '<none>' for multiple blocks, to allow users to remove the default block ID on multiple blocks. 🤔

After rebasing the MR, I did tests before and after the code changes and was able to confirm they worked as expected:
I was able to save several blocks with the ID value '<none>'.

I checked the display of the blocks and it seemed to work fine as well:
The two blocks would display without any HTML ID.

After updating slightly the inline comment in the merge request, based on the issue summary, I went ahead and merged the changes above at #8. 🥳

I checked in the PHPUNIT tests and currently, the ID field is not tested anywhere....
So that should probably be addressed in a different issue in the future.

For the time being, marking issue as Fixed.

Feel free to let us know if you have any questions or concerns on any of the recent code changes or the project in general, we would be glad to help. 😊
Thanks again very much!

Thank you very much for reporting this issue!

I'm having trouble reproducing the problem with the latest 4.0.x DEV version...
The MR seems fine.

But I'm having trouble understanding in what context or situation the following call could return a string:
https://git.drupalcode.org/project/block_class/-/merge_requests/54/diffs...

$block_classes_stored = $config->get('block_classes_stored');

I tried testing with a fresh install (no block classes) and saved the form with a test class, but everything seemed to work fine, with the call returning an empty array [], as expected.
I tried again with another class and again, it worked as expected: ["TestClass1a"].

Since I'm not very familiar with the previous state of the config and how it could potentially have been stored as a JSON string, so I would definitely appreciate some help better understanding how the problem could be reproduced.

Could anyone please provide clear testing steps to follow, to reproduce the issue, if possible with more context, versions of module/core, etc... ?

Any additional information that could help us reproduce the problem would be greatly appreciated.

Additionally, since there has not been any comments on the issue for more than a year, we could also perhaps assume the problem initially reported might have been resolved with some other changes (from other tickets) (?).

For the time being, I've allowed myself to move the ticket to Postponed (maintainer needs more info), but feel free to set back to Active or Needs review at any time if you encounter the problem again and are able to provide clear testing steps.

Feel free to let us know if you would have any questions or concerns on any aspects of this comment or the module in general, we would surely be glad to help.
Thanks in advance!

Great job Michael (@justcaldwell), as usual! 🙏
Thanks a lot for taking the time to look into this issue and document the logic so clearly with the different possible options.

Sorry for the late reply on this, but I had no idea nobody was actually maintaining this module at the moment...

I'm not very familiar with the 4.x code branch and features yet, but from what I've seen I would definitely agree there would be a lot of work refactoring, cleaning, reorganizing code files, etc...

At least the module has a base of PHPUNIT tests, so this should greatly help with any future refactoring or evolutions of its existing logic.
 

However, for the time being, refactoring the module does not seem to be in the list of priorities: 😅
There are still urgent issues to be addressed in the queue, to stabilize certain features or for Drupal Core compatibility.

Therefore, going with the least disruptive changes (solution 2 from #8) for now seems like a very acceptable compromise.👍

I actually did a manual round of testing of this locally:
1 - Enabled the module on 4.0.x and saved a block without classes and a block with classes, then 'drush cex'.
The dependency was there for the first block, although I had submitted the form with no change.
2 - Switched to repo branch 2.0.x (from the MR), ran 'drush updb', then 'drush cex'
==> The dependency in the first block was removed properly.

I then tried saving several blocks with and without classes and the dependency seemed to be updated as expected each time.

After updating slightly the inline comment in the merge request, based on comment #8, I went ahead and merged the changes above at #13. 🥳
This is a great step towards improving the quality of the configuration storage of the module!

Lastly, I have created a follow-up ticket to circle back on this issue if any refactoring of the module is planned at a later point:
#3555504: Refactor blockClassFormValidate() to avoid setting empty values →

Once again, always a great pleasure working with you Michael! 🙏
Let us know if you spot anything else in the module, or would have any advice or ideas of improvement, your help and feedback would surely be greatly appreciated!

Thanks in advance! 😊

dydave → created an issue.

No reply or follow-up on this issue for more than a year.

We could probably assume the problem is fixed and no other user encountered the issue.

Marking issue as Closed (outdated), for now.

Thanks again!

Thanks a lot Michael (@justcaldwell) for the great work on this!

I've noticed the problem a while ago on a project and never actually took the time to look into the necessary changes.

Just rebased the merge request and fixed a conflict with the number of the hook_install.

I'll give this MR a round of manual testing in priority so it could hopefully get merged very soon. 👌

Back to Needs review for now.

Thanks in advance! 🙂

dydave → made their first commit to this issue’s fork.

dydave → made their first commit to this issue’s fork.

Thanks everyone for the great work on this issue. 🙏

Thanks Carsten (@c-logemann) for putting together this merge request.

While testing and trying to understand the issue, I was able to catch another missing closing </div> tag in the page. 👌

I'm not sure why I'm unable to see them missing on Chrome or any display issue... but I'm assuming it's maybe because of the browser making the correction on display (?!) 🤔

Otherwise, the code changes made sense, so I went ahead and merged them above at #14. 🥳

This might need adding tests, but maybe it should be the object of a different ticket.

Marking issue as Fixed for now.

Thanks again everyone!

Needs rebase ==> Fix conflicts.

No activity on issue for more than a year.

This has most likely been fixed in other issues since there are no more calls to \Drupal.

Additionally, PHPSTAN, PHPCS and PHPUNIT jobs are all happy.

Marking issue as Closed (outdated) for now.

Thanks again very much for the great work!

dydave → made their first commit to this issue’s fork.

Thanks a lot Kul (@kul.pratap) for the great explanation above at #6!
Super nice Security conclusion!

Great job on the merge request as well! 👍
Additionally, I found: https://drupal.stackexchange.com/a/207036

It does make sense to support various types of data other than CSS classes or IDs identifiers.

Since your comments and details made quite a lot of sense, I went ahead and merged the changes above at #9. 🥳

This might need adding tests, but maybe it should be the object of a different ticket.

Marking issue as Fixed for now.

Thanks again everyone!

dydave → made their first commit to this issue’s fork.

Thanks again everyone for the great work and documentation in this issue.

The problem has been resolved in related 📌 Deprecated system_get_module_admin_tasks in drupal:10.2.0 and is removed from drupal:11.0.0 Needs review .

Marking issue as Closed (Duplicate) in favor of the new one.

Thanks again very much!

dydave → made their first commit to this issue’s fork.

dydave → made their first commit to this issue’s fork.

dydave → made their first commit to this issue’s fork.

dydave → made their first commit to this issue’s fork.

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !66 above at #2.

The PHPCS errors were fixed as well.

Since all the tests and jobs seem to be passing 🟢 again, I went ahead and merged the changes above at #3.

This should at least unblock the testing pipeline for pending merge requests in the issue queue so we could at get a more complete validation before moving forward with the changes.

Marking issue as Fixed, for now.

Thanks in advance for your testing feedback, comments and reviews!

dydave → created an issue.

dydave → changed the visibility of the branch 3463832-debug-tests to hidden.

Since this feature keeps breaking with changes to Project Browser, I thought we might need an automated integration Functional test that would help systematically testing the integration with the evolution of core, admin_toolbar and project_browser.

Added a basic Functinal tests class to check the links added by admin_toolbar_tools for project_browser are displayed in the expected order under the 'Extend' menu link.

Hopefully, this should help preventing these types of issues from repeating over and over in the future.

Reviews, feedback and comments are welcome.
Thanks in advance!

Quick follow-up on this issue:

It happens the Bartik and Seven contrib themes have just released a stable Drupal 11 release \o/
Therefore, we're finally able to come back on 📌 Disable Migrate D7 block_class Kernel tests for D11 Active to enable tests for all supported core versions.

All the changes detailed in the issue summary have been implemented and described in the merge request MR !62 above at #2.

The PHPSTAN errors were fixed by setting data provider method providerTest static.

Since all the tests and jobs seem to be passing 🟢 again, except for PHPUNIT tests for 11.3 (next minor) passing with warnings, I went ahead and merged the changes above at #3.

These warnings for the next minor should probably be further investigated in a follow-up ticket, but shouldn't block the current deployment pipeline from being stabilized, by moving forward with these changes.

We will most likely keep an eye on the upcoming changes of APIs, code deprecations, etc... and fix the warnings for 11.3.

Marking issue as Fixed, for now.

Thanks in advance for your testing feedback, comments and reviews!

dydave → created an issue.

Thanks a lot Haris (@haris khan jadoon) for reporting this issue!

The module Admin Toolbar Links Access Filter has been marked deprecated a long time ago, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

Additionally, we added a Warning message on the status report page in issue:
🐛 Warn site owners about the removal of Admin Toolbar Links Access Filter Active

Was there any other action you would recommend as part of this issue or could we consider it Closed (Works as designed)?
Could you please update the status of this ticket accordingly?

Requalifying issue as Support request for now, since it does not seem there would be any code changes that should be made to the module.

Feel free to let us know if you have any questions or if anything is unclear with this reply, we would surely be glad to help.
Thanks in advance!

Quick 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!

dydave → created an issue.

This merge request only impacts a single PHPUNIT Functional Test class for the Admin Toolbar Tools module: 👌
admin_toolbar_tools/tests/src/Functional/AdminToolbarToolsSettingsFormTest.php

So it should be really safe to merge this directly now, since all the Tests seem to be passing 🟢

Since these changes are going to greatly improve the Tests coverage of Admin Toolbar Tools Settings form and its configuration options, I went ahead and merged them above at #7. 🥳

Marking issue as Fixed, for now.

Testing feedback, comments or reviews would be greatly appreciated!
Thanks in advance!😊

This merge request only changes Admin Toolbar Search PHPUNIT Tests files.

Testing feedback or reviews would be greatly appreciated! 😊
Thanks in advance!

The merge request MR !176 should also address the following bug:
#3087173-18: Allow shortcut keys to be configurable →

Side note: When the search shortcut is disabled in the modules settings, the search input placeholder text still includes "(Alt+a)". Probably needs it's own issue.

These changes introduce a new search field title attribute string, when the search shortcut is disabled:
Type text to search for menu links in the admin toolbar.

The placeholder label was changed as well, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/176/di...

All Tests still seem to be passing! 🟢

Testing feedback and reviews would be greatly appreciated!
Thanks in advance!

Thanks 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 the ExtraLinks generated by admin_toolbar_tools:

Under "Structure > Menus": /admin/structure/menu

• The 'All menus' link should be displayed first in the list.
• The 'Add menu' link should be displayed second.
• Only the first 2 menu bundle links ordered alphabetically by menu label should be displayed: 'admin' and 'footer'

 
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_tools does not seem to be very clear, complete or consistent across entity types or supported modules....

• As a very basic example: The number of displayed entity bundle extra links under the "Content" (/admin/content) menu is not limited by the 'max_bundle_number'.
• Certain entity types are not impacted by it, under "Structure", for example: User roles and Views.
• etc...

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):
📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review
(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: ExtraLinks
admin_toolbar_search: SearchLinks
have 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. 👍

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 ... 🧐

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.js and 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:
📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review
impacting only Admin Toolbar Tools Functional Tests class files.👌

Marking issue as Fixed, for now. 🙏

Thanks again so much for your great help testing and reviewing these complex issues!

Thanks a lot @ressa for the CSS code! 🙏

I could see the Stylelint order errors in my VS Code editor and after pasting the code from your last comment (#7), they disappeared. 👍

I then did another quick round of manual testing locally and since everything seemed to work fine, I went ahead and merged the changes above at #8.

That's great progress so far @ressa! 🥳

We've got:
✅ 2 images removed from the repo \o/
✅ 2/2 sub-modules CSS files fully refactored (cleaned-up, simplified and documented).

Probably the next big piece of refactoring is the PHP code: 😅
📌 Admin Toolbar Search: Code clean-up and refactoring of CSS and module files Active
with a few bug fixes as well, in particular:
#3087173-18: Allow shortcut keys to be configurable →

Side note: When the search shortcut is disabled in the modules settings, the search input placeholder text still includes "(Alt+a)". Probably needs it's own issue.

Which introduces a new search field title attribute string, when the search shortcut is disabled:
Type text to search in menu links in the admin toolbar.

Let's keep up the efforts with the next tickets, looking forward to your feedback! 🤞

Marking issue as Fixed, for now.

Thanks again @ressa! 🙂

Thanks @ressa!!

Let's keep working our way down the stack!

There are still 5 issues to go, on which I would greatly appreciate to have your feedback and reviews!

I'm jumping in meetings, but I will have more time in the evening to answer!

Thanks you so much!! 🙂

dydave → changed the visibility of the branch 3540400-typeerror-unsupported-operand to hidden.

Thanks again everyone!

This issue was really easy to reproduce:
Install admin_toolbar_tools 3.x (3.6.2, for example) and project_browser 2.1.x (2.1.1, for example).... The error described in the IS would immediately occur.

The initial issue 🐛 Some mandatory parameters are missing ("source") to generate a URL for route Active was fixed to support project_browser 2.0.x which had the following config structure: an array of source ID values
https://git.drupalcode.org/project/project_browser/-/blob/2.0.0-beta1/co...

enabled_sources:
  - drupalorg_jsonapi
  - recipes

and now with project_browser 2.1.x the config is an associative array keyed by source IDS
https://git.drupalcode.org/project/project_browser/-/blob/2.1.x/config/i...

enabled_sources:
  drupalorg_jsonapi: []
  recipes: []

Change introduced in:
https://git.drupalcode.org/project/project_browser/-/commit/2e70410ced6e...
Corresponding to ✨ Make source plugins support stored configuration options Active

I tried the previous merge request which indeed fixed the error, but the links under the "Extend" menu (/admin/modules) would disappear.... defeating the purpose of having this code in the module in the first place.

I have created a new merge request MR !185 which not only fixes the error but also restores the initial behavior of the extra links logic by adding all the enabled project browser sources extra links under the "Extend" menu item, see screenshot below:

This patch implies dropping support for earlier versions of Project Browser, in other words, users would have to upgrade and use the latest versions: 2.1.x.
Therefore a conflict constraint was added to module's composer.json file as an attempt to prevent users from using Admin Toolbar's next release with any version of Project Browser lower than 2.1.0, see:

  "conflict": {
    "drupal/project_browser": "<2.1.0"
  }

 
Could you please help testing and reviewing MR !185?

Could you please also review the conflict section? I'm not sure how this could really be tested without merging the changes in the module, since composer constraints and requirements are usually evaluated before patches are applied...
I checked in other contrib modules, for example devel, copied the conflict section and adapted the version compatibility constraint.

I've done some quick tests myself and the logic seems to work exactly as it did for previous Project Browser versions:

• Enabled source plugins links are ordered by their weight.
• Updating the Project Browser config form should immediately reflect the changes in the menu.

 
Moving issue back to Needs review as an attempt to get more testing feedback and reviews on this new suggested patch.

Feel free to let us know if you encounter any issues with the suggested merge request or if you have any questions or concerns on any aspects of this comment or this ticket in general, we would certainly try answering as soon as possible.
Thanks in advance!

Floriiiiiisssss !!!!!!!! 🤩🤩🤩🤩

Super content de te voir de retour sur DO 🥳

Merci beaucoup pour les crédits sur le ticket et la MR ! 🙏

Tiens moi au courant, à l'occasion !
A très bientôt, j'espère ! 🙂

Thank you so much Fran (@fjgarlin)! 🥳

I confirm I was just able to update the credit records for the ticket with which we were having issues:
📌 Admin Toolbar Search: Code clean-up and refactoring of CSS and image files Needs review
Looks like the problem is fixed for my user and I should be able to set properly issue credits. 👌

Thanks a lot! 🙏

Thank you so much Fran (@fjgarlin) for the nice and speedy reply, as always! 🙂

Sorry I couldn't make it to DrupalCon Vienna this year and couldn't see you and the team 😖
I watched Dries Keynotes so far though, very promising, as usual! Great slides and designs! 🤩

I hope I can make it next year 🤞

Good luck fixing the issue with contribution records.
Thanks again very much for your great help!

dydave → created an issue.

Great work @ressa!🙏
Thanks a lot once again, for all the great help!

I reviewed and tested carefully your changes locally, after rebasing the merge request with the latest changes and they seemed to work great! 👍

I'm not sure exactly why the Gitlab CI Stylelint job does not catch the same errors I see locally, when developing with VS Code....
It seems much more lenient on GitlabCI or maybe we're missing some kind of configuration... 🤔
(to raise the validation level, or something like that?)

Very good catch though on fixing any Stylelint issues ==> I'll make sure to do the same locally, even though they are not raised in the Stylelint validation jobs.

After doing a quick review and another round of manual tests locally, I went ahead and merged the changes above at #6. 🥳

Marking issue as Fixed, for now.

Let's keep up the good work with the refactoring of Admin Toolbar Search! 👍
See: 📌 Admin Toolbar Search: Code clean-up and refactoring of CSS and module files Active which also fixes and cleans-up the Search Shortcut feature.

Thanks again for the additional changes, reviews and testing! 🙂

Super good catch @ressa!
Once again! Thanks a lot! 🙏

Totally.... That's my bad! 😖
I think it comes from a copy/paste from another MR or module (admin_toolbar_search, I think 😅)...

so we could either carry on with 8203 (which could be seen as a bit strange, since it's based on 8.2 ...) or use the current versioning. Since it should be higher it could be 37001, since the release will be 3.7.0 and then a "0" and a "1" = 37001 ... what do you think?

You're completely right we should probably consider changing the standard....
But I'm not sure we would really be releasing this batch of tickets in a 3.7.0, perhaps 3.6.3?
At this point, I don't think we've made any Backward Compatibility (BC) breaking changes, except maybe, translations strings, which is probably "minor" considering the strings in this module 👌

I was thinking about this the other day and we're not making config changes, API compatibility changes, etc...
I think it's fine to keep using the same Meta Roadmap ticket, even though we don't release a new minor:
My thinking behind that is that it would probably be much easier for sites to update for a patch version rather than minor....

Since we're still unsure for now, I would suggest maybe keeping the same pattern (first option you pointed out), or maybe using the current minor: 36201?

For the time being, I updated the MR with the correct numbering 8203 following the existing standard.
(Just fixed the wrong copy/paste)

Moving back to Needs review, let me know if you still think we should update the version numbering in the MR, I would be glad to make the changes 🙂
Otherwise, we could still do it in future merge requests 👌

In any case the update hooks should work fine 👍

Thanks in advance!

Thank you so much @ressa for taking the time to wrap your head around this one 😅

Again, this is probably pretty minor, but it made things simpler for using a standard order for testing all these links for entity bundle links, see for example the 'All types' link:
https://git.drupalcode.org/project/admin_toolbar/-/blob/c5fc0f14545808fe...
which should always be first, or the 'Add type' links:
https://git.drupalcode.org/project/admin_toolbar/-/blob/c5fc0f14545808fe...

In other words, while the testing logic worked for all other entity types it didn't just for 'menu' and 'user_role' 😅
So we had the choice to either keep the existing logic and add a special case in there tests.
Or keep the same generic testing logic and standardize the order of the problematic menu extra links in the code. 👍

The Devel links are not tested automatically though, but manual testing should help confirm the changes are consistent for all supported entity types.

In any case, this merge request only impacts the 'weight' of the corresponding menu extra links, so it should be very safe to move forward with these changes. 👌

So, following your confirmation above at #4, I went ahead and merged the changes above at #5 🥳

Marking issue as Fixed, for now.

One more to go 📌 Add 'Manage Permissions' and standardize menu label in Admin Toolbar Search suggestions Active and we should be able to move forward with the Functional integration tests. 👍

Thanks again very much for your great help and time testing/documenting @ressa! 🙏

Let's go @ressa! 🥳

Thanks a lot for your feedback on all the issues, that's a really great help! 🙏

Following your confirmation above at #4, I went ahead and merged the changes above at #5 👍

I'm glad the changes worked as expected and are helping standardizing module's behavior.

Basically, this issue was immediately encountered with the admin_toolbar_tools integration Functional tests:
📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review
The tests create 10 Views in the setUp and with the first assertions, right after logging in as an admin user, the expected links to the created Views could not be found.... failing the tests.
So while I was developing the tests, I started investigating why the links would not display... and why would the 'menu' entity links would be displayed .... which brought me to this piece of code 😅
I looked at the HTML output of the tests and indeed, the Views links were not there, so I tried reproducing manually and again.... the same bug appeared 😅
Without these integration tests, testing the integration with all the modules implementing a custom logic with Admin Toolbar Tools, it would have been very difficult to actually catch these bugs.

I've added a bunch of other checks for the delete and update operations, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/c5fc0f14545808fe...
Which should cover these functions (insert, delete, update) for all supported entities now 😎
('comment_type', 'node_type', 'media_type', 'user_role', 'view', etc...)

The idea of this ticket is to try to fix any issues with the entity types that are currently supported.
Down the road we could think of improving this logic, trying to find a more generic method for supporting menu rebuilds with entity types operations.

Marking this issue as Fixed, for now.

Let's keep working of getting the rest of the issues to land. 👌
Thanks again very much for your great help @ressa!

We're very lucky we could have your help and attention Michael (@justcaldwell) on these kinds of tricky JS/CSS interactions and behaviors! 🙏

Thanks a lot for the very clear explanation above at #16 on the two issues with the z-index and the offset.

I've just updated the related Core issue, see:
#3410871-24: The CKEditor 5 toolbar is overlapping with the Admin toolbar →
where I submitted the solution from the MR and asked where it could potentially be added in Core.

I suggest we wait a little bit longer and see if these changes could actually be moved to Core instead of Admin Toolbar, so it fixes the issue for everyone (Users and non users of this module 🙂), before moving forward with this merge request.

Once again, sorry for the delay on this, but I've been giving priority to other issues in the queue.
Let's try pushing together a bit more on the Core side, maybe?

Any feedback, questions or comments would be greatly appreciated.
Thanks again for all the great work! 👍

dydave → changed the visibility of the branch 3410871-the-ck-editor to hidden.

@nod_, @wim leers:

Could you please give us a very quick hand on this one?

We would like to add the following CSS rule to the core Toolbar module styles:

/* Increase z-index to prevent overlap with the CKEditor toolbar. */
body:has(.ck.ck-editor) .toolbar-oriented .toolbar-bar {
  z-index: calc(var(--ck-z-panel) + 1);
}

But all the CSS files of the module seem to be overridden in core themes: Tested with Claro on D11:
Adding this rule in any of the files of module's CSS folder seems to have no effect...
https://git.drupalcode.org/project/drupal/-/tree/11.x/core/modules/toolb...

The object of this ticket is:
Increase Toolbar's z-index to mitigate overlap issues with the CKEditor Toolbar.
(So the Toolbar displays in front of CKEditor's)
Until a better solution could be found in 🐛 CKEditor5 sticky panel top offset does not get recalculated after calling Drupal.displace() Active .

Would you have any advice or recommendations on how to proceed?

• Should a JS based solution be considered?
• Should the overriding files from the themes be patched as well?
• Should this rule be added somewhere else?
• Any other ideas?

Thanks in advance! 🙂

Thanks you very @ressa! 🙏

Hope this could help getting some traction 🤞

From my perspective: The amount of work and time required to integrate the JS library and rework the styles should be lower than the one required for these changes to land and stay maintained overtime, with an immediate return in the short to mid term.

Once I'm done with the refactoring of module's Automated Tests and the clean-up of admin_toolbar_search, I'll most likely start looking into this 🤓

It looks very interesting and I'd like to get more familiar with the Accessibility standards implemented as well.
I'm not an expert at all myself ... I'm sure I've got lots to learn and plenty to catch up 😅

It's good to know though you can rely on developers who actually specialize on these kinds of standards and mostly, keep up and follow the new ones... Pretty much what we do with Drupal Core change records for example 😅
It's really intense to keep up with all the new features, evolutions, API changes, modules, developers trends, etc...

Would be happy to hear some opinions and feedback.
Cheers!

Thanks everyone!

I thought I had mentioned this somewhere here, but it doesn't seem to be the case....

There is a related issue with a different approach ✨ Use the Accessible Menu library to manage the menu structure Active completely outsourcing the Accessible Dropdown Menu features to the very interesting library:
https://accessible-menu.dev/
https://github.com/NickDJM/accessible-menu
Fully accessibility compliant, properly maintained and specialized for these specific components...

See some code examples and demos on the Github page... it really looks like what we're trying to do here 👍

Ideally, it would be great if we could potentially move all Accessible Dropdown menu features to an external library specialized for that.... Trying not to reinvent the wheel 😅
See my comments in the corresponding ticket.

I really haven't had any time to work on this.. but I would personally recommend putting our time and efforts on this issue rather than keeping on working on our own custom solution in the module...

This would also prevent having any sort of conflicts with any changes in the upstream (re #71).... since a ton of code would be moved out of the module...

Feedback, comments and opinions are more than welcome!
Thanks again! 🙂

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 ☹️

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....
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 new module.
==> Needs to be integrated with module's tests.

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

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! 🙏

Thanks @ressa for creating this issue, once again! 👍

Very good catch, not a very obvious one 😅
Really not sure how you could have stumbled on this one 😆
Because these strings are very random and change all the time!

It would be great if we could get all the refactoring tickets for the Automated Tests rolled in the module before moving forward with this one....
We would most likely need to add a check for this in 📌 Automated tests: Simplify admin_toolbar_search FunctionalJavascript tests Active and more specifically here:
https://git.drupalcode.org/project/admin_toolbar/-/blob/22bc71e22a155de7...

• Get first 5 characters of the generated token from the HTML markup in the toolbar.
• Submit it in the search as a query and assert suggestions are empty or do not contain the URL of the flush link.

 
I think it's not the only one: The logout link is the same ....
(try typing logout in the search, check the query string after the question mark ?, type in the search a few letters of the query string and you'll see the same behavior as described in the IS for the flush link ==> Logout will come out in search suggestions)

So we would most likely need to do a CSS query search (with an elementExists CSS search query) to select all links in the toolbar admin menu with a query string and loop through selected links with the same steps as above (get start of query string, submit in search and confirm URL is not found in suggestions).
This would make it bullet proof 👍

In terms of the JS changes in the search to effectively fix this, we would have to change a bit the JS array:
Drupal.behaviors.adminToolbarSearch.links
Loaded when the search field is focused with function populateLinks:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
Break the URL on the question mark and keep the first element in:
'label': label + ' ' + this.href, (replace the this.href with the stripped off URL)
(==> strip off query string from URL and save in labels loaded array of links)

Pretty straight forward, I "think". 🙂
 

I would be very happy to fix this ASAP, but ideally, if possible, it would be great if we could first fix the refactoring tickets...

In particular, on top of the ones already in the stack (9 issues ready to go 🥳), I've got a major refactoring lined up for the file admin_toolbar_search.js with mostly Vanilla JS, code clean-up and ESLINT fixes 🤩
(Not pushed yet in merge requests.... I think I've flooded enough the queue 😅)

It would be great if we could add this issue's fix right after, plus I've got ✨ Make obvious when toolbar search returning no results Active almost ready as well, with the refactored file 👌
(Same thing ==> we need the tests before proceeding with this....)

After refactoring the Automated Tests, I'm now super familiar with module's code base ... I pretty much know it by heart now 😆
I could rewrite the module from memory 😆
It's like reading the same book a hundred times 😆

I'm trying to work on this module with the community (== you pretty much 😅, my contribution partner 🙏), as much as possible, but if I see the refactoring is holding up tickets for too long, I'll most likely move forward and get pending merge requests rolled-in ASAP.
I'm the maintainer after all and thus, have all the required rights and permissions to proceed with these changes, as long as I am confident and ready to take full responsibility 👍
As soon as the refactored tests are in place, I am super confident taking responsibility with any changes.

I've updated the list in the issue summary of the Meta Roadmap ticket with the 9 tickets ready to go at the top 🥳
If there is any ticket, merge request or change you're not sure about, please let me know and I'll most likely get them merged straight away ... Because I really doubt anybody is really going to help or answer, other than you 😅 and I'm not planning on waiting for months to get feedback or reviews on these tickets.... We've still got so much to do....

As always, your feedback is more than welcome, if you disagree with some of the suggested changes or have different ideas, vision, etc... or if you catch anything I could have missed, some untranslated labels, textual improvements, etc... as you did many times before: It would be super helpful!
It has been so constructive working with you on previous issues, in particular, I'll always remember your help bringing me down to earth when I got carried away on the hoverIntent ticket (with the useless config options) 😆

In short, if anything is beyond your technical skills or understanding, let me know and I'll take care of it and most likely get it merged ASAP 👌

Always a pleasure working with your @ressa! 🙂
(missing our back and forths on the scroll up/down ticket 😆)
Thank you very much, once again for your precious help! 🙏

Thanks Martin (@mandclu)!
Same comment I had above at #14.

Stephen (@smulvih2) is this something that could be added to the MR?

Stephen, this would perhaps allow a smoother/easier integration of this feature to the module by allowing to keep the existing/current behavior, while still being able to extend/override with specific site configurations.

Thanks again everyone for the great work on this feature!

Really sorry everyone for the delay on this. 😅

Just a quick update to let everyone know I have already reviewed and tested the MR and the changes suggested by Michael (@justcaldwell) above at #16 completely make sense.

I'll try to get this merged ASAP this week so this issue could be crossed off our list. 👌

Thanks again everyone for the great work, your patience and understanding. 🙏

Thanks Joachim (@joachim) for taking the time to look at this issue, it's greatly appreciated to have some feedback.

I don't think it's a good idea for a tests issue to be making changes in non-test code as well. Those probably need their own issues.

I've pulled out of the merge request all non tests files and created the following 3 corresponding tickets on which we would greatly appreciate your reviews and feedback:

• #3552168: Standardize display order of extra links menu items →
• 🐛 Trigger a menu link rebuild for user roles and views entity operations Needs review
• 📌 Add 'Manage Permissions' and standardize menu label in Admin Toolbar Search suggestions Active

 
I have not removed the files from this MR just yet, since the tests would break as these changes are now effectively tested, which is not the case with the current tests base.

> AdminToolbarToolsConstants

It looks like this is only used for tests? It makes more sense to put this trait in tests/src, or in a test base class.

Done: Moved constants class 'AdminToolbarToolsConstants' under the 'tests' folder.

Also, what are the functional tests checking? Could this be done with Kernel tests instead? We presumably already have Functional tests that test the menu works correctly. Here we just care that certain conditions produce certain menu item plugins.

Indeed, you are right. Maybe I didn't explain correctly what these new test classes are for:

This is a refactoring of current existing tests classes with admin_toolbar_tools:
\Drupal\Tests\admin_toolbar\Functional\AdminToolbarToolsSortTest
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/tests/src/...

\Drupal\Tests\admin_toolbar_search\FunctionalJavascript\AdminToolbarToolsSearchTest
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...

This is more of an integration tests suite to improve the tests coverage of the integration of the Admin Toolbar Tools module with other modules:

• admin_toolbar
• admin_toolbar_search
• config
• block_content
• comment
• contact
• field_ui
• language
• media_library
• menu_ui
• node
• taxonomy
• user
• views_ui

Very little of the integration code is actually currently tested in the module.
None of the issues listed above could have been identified without having a complete integrations test, see for examples issues with User roles or Views.

These new Functional tests classes are a rewrite of the existing classes with many improvements and much more complete coverage of the executed code.

Maybe we could work on writing more precise Kernel tests, but I'm not sure exactly how that would work and how much time that could take.... I haven't looked further than that...

But what I'm sure about is that what we have in the merge request right now is much more complete than the existing tests and would help greatly keeping the module well maintained or taking on more refactoring tickets.
 

I'm setting this back in Needs review even though these changes would have to be merged after the issues above have been fixed, so the MR could be rebased properly, without module's code files in the DIFF.
 

Feel free to let us know if you have any questions or concerns on any aspects of this comment or the suggested changes, we would surely be glad to hear your feedback.
Thanks in advance!

Adding 3 easy ones broken out from another issue so hopefully the testing and reviewing could be easier:

• #3552168: Standardize display order of extra links menu items →
• 🐛 Trigger a menu link rebuild for user roles and views entity operations Needs review
• 📌 Add 'Manage Permissions' and standardize menu label in Admin Toolbar Search suggestions Active

Quick 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 SearchLinks class 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 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review .

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!

dydave → created an issue.

Quick follow-up on this issue:

I think it is better to isolate these changes so they could really be focused on testing the entity operations for user roles and views.

All the changes detailed in the issue summary have been implemented and described in the merge request MR !180 above at #2.

There are no tests in this merge request, since they are added in related issue 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review .

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!

dydave → created an issue.

Quick follow-up on this issue:

I think it is better to isolate these changes so they could really be focused on testing the display order of the updated menu link items.

All the changes detailed in the issue summary have been implemented and described in the merge request MR !178 above at #2.

Resulting display order, after applying the changes from the merge request:

Menus:

User roles:

There are no tests in this merge request, since they are added in related issue 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review .

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!

dydave → created an issue.

Thanks a lot Joël (@joelpittet) for the speedy reply and clarifying the issue! 🙏

It's not me going mad, missing something in the testing steps 😆

Too bad this issue doesn't stand anymore, because I think it is probably one of the clearest and best documented ticket I've ever seen in this module 😆

Happy to hear everything is in order though 🥳

Thanks again Joël!
With great tickets like that, you're welcome to drop by at anytime 😉😆

Cheers!

Thanks a lot Kostia (@_shy) for creating this issue and suggesting this new feature! 🙏

I wasn't aware this was now possible with D11! 🤩
Thanks for bringing this new feature to our attention!

Just a few very quick comments though 😅

I'm not personally convinced Admin Toolbar is the best candidate for this type of change or feature:
Indeed, the module admin_toolbar has about 4 or 5 icon images (chevrons and collapse images), admin_toolbar_tools has 2 (drupal logo and local tasks icon) and admin_toolbar_search has 1 (search icon magnifier)....

Overall, the images are divided in different folders depending on the module to which they belong.... So we would potentially need 3 icon packs, each one with very few images...

On top that, one of the great challenges of the admin_toolbar module is the wide range of major Drupal Core versions that are supported: 3 majors 😅
Currently the module supports 9.5, 10.x and 11.x, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
with.... PHP 8.1 still supported...

I'm sure you could probably understand how much of a challenge this has been for us in terms of maintenance 😅
There are a ton of improvements, code simplification and changes we would like to make with the latest versions of PHP and Drupal Core APIs, such as using config_target in config forms, PHP attributes for plugins, etc... Not to mention D11.1 hook classes and soooo much more.....

But we can't 😅
because, we still need to support 3 major core versions...

Unless we break this down into separate branches supporting 2 major versions each and maintained at the same time, I don't really see how we could move ahead of versions without breaking support for older versions.

Why are there 3 major versions supported?!
It is part of module's policy, Admin Toolbar being such a popular module (in the top 10 of all times contrib), the range of users is very wide and so are the versions of PHP and Drupal Core.

The way the module has been maintained is that when the next Drupal Core major version is published, the last supported version is dropped.
In other word, when Drupal 12 will be out, we will be able to add support for D12, drop support for D9 and the last supported Core version would be 10.4.
At this point we should be able to introduce all the API changes from D10 (config_target and so on...).

Although I'm not a big fan, we could still of course add if (version_compare(\Drupal::VERSION, '11.1', '>')) { cases a bit everywhere, to support specific API changes for certain versions.
But that makes module's maintenance even more complicated and gives more work 😖 than a single standardized method supported across all core versions.

All that being said ..... is it really worth investing time in this? 🙂
Could we maybe come back to this in 5 years? 😆 (when D13 is out and D11 the last supported Core version)
Perhaps other modules with more visual features and icons would be better candidates for such a nice improvement?

Also, for example, the Gin theme icon is not displayed because it uses a new Icon API.

If you think this would be the way to go, then, let's try considering it and seeing how support issues could potentially be approached.
Down the line, I'm certainly not opposed or against this idea or type of feature... I'm just having a hard time understanding the benefits and seeing how this could be integrated and maintained without too much pain 😅

Feel free to let us know if you have any questions or concerns on any aspects of this comment or the module in general, we would be delighted to help!
Thanks again very much for your interest and great contributions to the Admin Toolbar module! 🙏

Thanks a lot Joël (@joelpittet) for the very clear and detailed issue! 🙏

I started working on this and created a custom module which adds a custom ContentEntityType with an associated bundle as a ConfigEntityType.... Very standard setup that I mostly copied from core modules here and there.

I tried to follow the exact steps described in the issue summary, but unfortunately, I was unable to reproduce the error....

In other words :
1 - Install the custom module which adds the test entity type with its associated bundle.
drush en custom_test_module
2 - Browse to add a test bundle.
3 - Browse an entity bundle route defined by admin_toolbar_tools.
4 - Uninstall the custom module
drush pmu custom_test_module
5 - Refresh the page ==> The associated menus are not there anymore
The route previously browsed in 3 now returns a 404.

I tried different types of setups with/without cache, etc....

But after trying several things I was still unable to reproduce the error and the problematic behavior detailed in the issue summary.

Tested with :

• admin_toolbar:3.6.2
• drupal/core: 11.2.5
• PHP: 8.3

 
Could there by any additional context missing from the description of the issue to actually be able to reproduce the error?
Would there be an example of contributed modules that could potentially be used to reproduce the issue?
Could it be possible to reproduce this type of situation with a Core module that adds an entity type, such as the contact or block_content modules?
 
I've got some code already locally for testing admin_toolbar_tools with a custom entity type, so as soon as I am able to reproduce the issue, its resolution should be pretty straight forward without taking too much time.

Any additional context, information or elements that could help us reproduce the issue would be greatly appreciated.

Feel free to let us know if you have any questions or concerns on any aspects of this comment or the module in general, we would surely be glad to help.
Thanks in advance ! 🙂

Thanks a lot everyone for the great work on this ticket and getting it over the line! 🥳

Thank you very much Liam (@liam morland) for granting us contribution credits us on this issue! 🙏
Cheers!

Moved all CSS related changes to a separate specific issue 📌 Admin Toolbar Search: Code clean-up and refactoring of CSS and image files Needs review so they visual changes should not be expected in this issue.

One small CSS rule had to be added to support the change to the <nolink> URL used, changing the generated markup from a link <a> tag to a <span>.

Otherwise, all other changes should be focused on the YAML files and the module file.

These are the effective changes in this merge request:

• Renamed admin_toolbar_search CSS file.
• Fixed Keyboard shortcut displaying in field's title or placeholder if it is disabled.
• Replaced search toolbar tab link with instead of the admin index URL.
• Refactored all module's HTML ID strings intro a constants class AdminToolbarSearchConstants.

All of which are successfully tested in the PHPUNIT tests 🥳

Any testing feedback, comments or reviews would be greatly appreciated!
Thanks in advance!

Quick follow-up on this issue:

I think it is better to isolate these changes so they could really be focused on testing the display and CSS.

All the changes detailed in the issue summary have been implemented and described in the merge request MR !177 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.

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!

dydave → created an issue.

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !176 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.

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!

dydave → created an issue.

Hi Joachim (@joachim)!

Thanks a lot for the great work on this issue and for keeping the suggested code changes maintained.🙏

Just wanted to bring to your attention related issue 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review which references this issue several times 🙂

It would be super helpful if you could please try to see how the changes in the merge request could integrate with the 'ExtraLinks' tests added in the related tests issue.
I would definitely be in favor of working on this issue and getting this change potentially integrated, as soon as we could have the 'ExtraLinks' and 'SearchLinks' automated tests in place.

Overall, I think this ticket is more of a refactoring task than a new feature, so with the tests in place, we should be able to move forward a bit quicker with these changes.

Could we also refactor the following links/labels? 'Add {entity type}' links under 'Structure' ('admin/structure')

'Add contact form'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add media type' ==> I "think" you've already done this one ✅
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add content type'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add vocabulary'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add role'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

'Add view'
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...
https://git.drupalcode.org/project/admin_toolbar/-/blob/af36beae3b23cab0...

Note that Block content and Comment types currently do not have an 'Add {entity type}' link added by the ExtraLinks class.... which should ideally also be standardized: the same behavior across all entity types would be easier to understand for anyone. 👌

It would be great if we could find a more generic way to approach the 'Add {entity type}' links added by the module under 'Structure' 👍
Then we should be able to come back to the 'Add {entity type} content' links, under the 'Content' menu.

Any feedback, comments or reviews on related issue or this ticket would be greatly appreciated.
Thanks in advance!

Thank you so much @ressa! 🙏

Sorry I didn't reply to your other comments in other tickets, but I wanted to focus on really pushing out in priority all the Automated Tests on which I have been working for months! 😅

Some issues really took a lot of time, in particular:
📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review 😅
Some test files are very long with lots of cases because a lot of things are being tested (more than 300 assertions).

There is still more code that could be further tested, in particular, all the Javascript code.... But for now, this should give us a much more robust, complete and stronger base for testing the module with almost 100% of the PHP code being covered, on which we should be a able to keep building. 🥳

Re #14:
Sorry I was not aware you were not familiar with automated tests 😅
Thanks a lot for contributing this documentation: we should be able to direct users who would wish to contribute to testing these issues in case they are not familiar with the required setup 👌
 

Overall the tickets at #16 have almost 0 impact on existing code files, only test classes and files should be impacted.
📌 Remove unused Tools icon for Drupal 8 Active is just some suggested clean-up task and 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review changes slightly extra links order in the admin menu (for roles and menus)... so overall, it should be very safe to move forward with these changes.

A lot of the technical documentation in these tickets or in the code files is initially meant for me 😅
juts to remember some of the technical choices, implementations or problems identified along the way. 👍

With these tests in place, we should feel much of confident moving forward with code refactoring or certain tickets with big changes, such as 📌 GitlabCI: Fix ESLINT validation errors Active .
 

In any case, feel free to let me know at any point if you have any questions or concerns on any aspects of these tickets or related merge requests, I would definitely be very happy to help. 🙂
Thanks in advance!

Super nice @ressa, as usual! Thanks a lot for your help!

OK, here is a big batch of tickets ready for testing 🥳

• 📌 Remove unused Tools icon for Drupal 8 Active
• 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review
• 📌 PHPUnit: Improve tests for admin_toolbar_tools Active
• 📌 Automated tests: Simplify admin_toolbar_search FunctionalJavascript tests Active

 
This should allow the tests coverage of the module to be greatly increased so we should then be able to start taking on refactoring tickets 👌

Let me know if you spot anything, have any questions or suggestions, as usual, your feedback is always very helpful! 🙏

Thanks again for all your great help with all the tickets! 🙂

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !175 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.

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!

dydave → created an issue.

Long overdue follow-up on this issue 😅
But since I've been working on fixing module's tests, I thought we might as well get this one done 👍

All the changes detailed in the issue summary have been implemented and described in the merge request MR !174 above at #4.
Added the necessary logic to change form settings values so the expected results could be tested. 👌

• Very basic check of an extra link ('All menus') added for 'menu_ui' to confirm the 'max_bundle_number' was properly updated.
• Test the display of the 'Local Tasks' tab and links when the 'show_local_tasks' setting is enabled, tests:
  \Drupal\admin_toolbar_tools\AdminToolbarToolsHelper::buildLocalTasksToolbar()

 
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.

Overall, this merge request should complete the tests coverage for the admin_toolbar_tools module, along with related 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review . 🥳

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!