- π«π·France dydave
Thanks a lot Moshe (@moshe weitzman)!
The module still seems to be using
admin_toolbar_tools_entity_insert
implementations.Would you have any concrete suggestions or examples on how this type of logic could be taken out of the module?
Otherwise, your initial suggestion in the issue summary seems like a very good step forward:
Would you still recommend we proceed with the code changes?How about BC?
Would there be any risks of breaking backwards compatibility?
Wouldn't such a change require a new minor release at the least?Any advice suggestions or recommendations would be greatly appreciated.
Thanks in advance! - Merge request !131Issue #3319001 by moshe weitzman, dydave: Restrict entity CRUD hooks to config... β (Open) created by dydave
- π«π·France dydave
Thanks a lot Moshe (@moshe weitzman) for attracting our attention on this piece of code, in particular introduced in related issue #3063356: Admin toolbar doesn't update when menus or bundles change β with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/4324836d984520...These hooks seem to be way too generic for the problem they are trying to correct:
If I understood properly the initial request, we'd like to rebuild menu links whenever a config entity bundle (such as a content type, media type, etc...) or menu is created, deleted or updated, see:- Deleting a menu or adding a menu that would appear in the admin toolbar should trigger a cache clear on the admin menu.
- Deleting a bundle or adding a bundle that would appear in the admin toolbar should trigger a cache clear on the admin menu.
Problem:
But looking at the code currently: Every time an entity of any type (whether content or config) is created, updated or deleted, the menu links will be rebuilt, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...In other words, whenever a:
- node page item is created, deleted, updated,
- comment is created, deleted or updated,
- media item, etc...
These hooks will be fired and menu links rebuilt, whatever the role of the user triggering the operation could be.
This seems like a Major performance impact on the module that could potentially be greatly improved by:
Restricting entity CRUD hooks to config entity bundles and menus to prevent rebuilding menu links every time the hooks are fired.
Created an initial merge request MR!131 above at #4 which should fix the issue by only rebuilding menu links if the entity that was changed is a config entity bundle or a menu.
Since the Tests all seem to still be passing π’ moving issue to Needs review and bumping to Major as an attempt to get more reviews and testing feedback.
Any questions, comments or feedback on this comment, issue or merge request would be greatly appreciated.
Thanks in advance! - π«π·France dydave
I'm not really seeing how we could be using ENTITY_TYPE hooks...
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...For example:
function hook_ENTITY_TYPE_createActs when creating a new entity of a specific type.
Should we create hooks for all the bundle types that could be created ? for example, node_type, media_type, comment_type, etc... ?
Any advice, help or suggestions would be greatly appreciated.
Thanks! - π«π·France dydave
Found: hook_entity_bundle_create
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...That "could" work for config entity bundles but doesn't with menus....
Additionally, there doesn't seem to be a
hook_entity_bundle_update
....