MenuTreeStorage shouldn't invalidate cache tags if the menu didn't change

Created on 1 November 2024, about 1 month ago

Problem/Motivation

While debugging an issue with a menu cache tag I noticed that the tags will be invalidated even if the menu didn't change. As menus are usually used on most pages on the site this can result in a large number of invalidations in the CDN for no reason. MenuTreeStorage::doSave already checks to see if the menu item changed, so we can leverage this information to only clear the cache tags if it did.

Steps to reproduce

Set a conditional breakpoint in CacheTagsInvalidator::invalidate for the config:system.menu.main cache tag. Save the menu without changing anything, and the cache tag will be invalidated.

Proposed resolution

Only save if it has changed.

Remaining tasks

User interface changes

N/A

Introduced terminology

N/A

API changes

Changing the return value of MenuTreeStorage::doSave to return a boolean value instead of the menu name in the array. Currently it returns an associative array like main => main. Change to main => TRUE. This is a protected method so should be ok to change as it's internal?

Data model changes

N/A

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.1 πŸ”₯

Component

menu system

Created by

achap πŸ‡¦πŸ‡Ί

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

Merge Requests

Comments & Activities

  • Issue created by @achap
  • Pipeline finished with Failed
    about 1 month ago
    Total: 791s
    #326479
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Even though categorized as a task, maybe could be feature request, seems like a change that needs test coverage

  • achap πŸ‡¦πŸ‡Ί

    I'm happy to write tests, however before I do maybe someone familiar with the menu system can review the approach and see if this is a sensible change?

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    First and foremost, the elephant in the room: None of the IS is true unless you have a menu link that didn't change being saved frequently. So my first question to you would be: Why do you have menu links being saved without changes so often?

    MenuForm::submitOverviewForm() seems to prevent a menu link from being saved in the tree storage if nothing has changed. So that seems like a double layer of protection there (one in the form, one in the storage) that tries to reduce work.

    So what is causing a menu link without changes to be saved so frequently outside of the menu form? If we can't answer that, then I fear we might be optimizing something that may not need optimizing just because a website has some other code running that is actually causing havoc.

    I'd err on the side of "Closed (won't fix)" if we can't get some better insight into this.

    As to the MR:

    I'm not sure we can simply change the return of MenuTreeStorage::doSave() like that. We didn't declare the service as internal and it's definitely not unthinkable that there are custom extensions out there that implemented their own ::doSave() to do some extra work in other (perhaps external) systems.

    Also not sure about the code flow here:

        if ($original) {
          $affected_menus[$original['menu_name']] = $original['menu_name'];
          // ...
          if (array_diff_assoc($fields, $original) == [] && array_diff_assoc($original, $fields) == ['mlid' => $link['mlid']]) {
            $affected_menus[$original['menu_name']] = FALSE;
            return $affected_menus;
          }
        }
    

    Why keep the original assignment?

    I'd go into even further detail, but as I said at the start of my comment, I'm not even sure anything needs to be done here.

Production build 0.71.5 2024