Change entity CRUD hooks to entity_TYPE hooks

Created on 3 November 2022, over 2 years ago
Updated 25 February 2025, 2 months ago

Problem/Motivation

The tools submodule implements admin_toolbar_tools_entity_insert() and similar. These fire frequently and cause a lot of menu rebuilding. These were added in #3063356: Admin toolbar doesn't update when menus or bundles change β†’ . It looks to me like we picked the wrong hooks. should have used the entity_TYPE hooks like https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21.... These fire far less frequently.

πŸ“Œ Task
Status

Active

Version

3.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States moshe weitzman Boston, MA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡«πŸ‡·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!

  • Pipeline finished with Success
    about 1 month ago
    Total: 377s
    #456746
  • πŸ‡«πŸ‡·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_create

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

Production build 0.71.5 2024