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

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

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?

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.

In my case it happens because of the "Menu Settings" tab on node edit forms. If you click "Provide a menu link" for example and make that node part of a menu, on subsequent saves of the node the menu cache tag gets invalidated even if nothing about the node changed. Seems to happen when directly editing the menu too. That might be unique to my setup but I had a colleague confirm on a different site.

I guess the best thing to do would be write a test proving that that is indeed the case. I'll try and find some time this week to do that.

There was a recent slack thread that I somehow can't find anymore about this.

Rendered menus contain access cacheability information, that includes node cache tags for menu links to nodes. This will not change anything in practice IMHO for the editing node use case because that still has the node cache tags in there as well.

The slack thread discussed alternative options building and cleaning up the render array cache tags (for example replacing all the node cache tags with a single custom cache tag that is only invalidated when there is a relevant change to the referenced nodes) but that's hard to do as a generic thing and this optimization is likely only feasible for anonymous users.

Just following up on the comment by @berdir
The thread was here: https://drupal.slack.com/archives/C079NQPQUEN/p1731584309641329
And the related issue is here: https://www.drupal.org/project/drupal/issues/3486604 🐛 Node edit form re-saves menu items even if no change has occurred Active (now closed as a duplicate of this)

I added a test case to show that this is happening without changing anything else, and addressed some feedback on MR. My use case is pretty simple because we don't have complex access controls like those mentioned in the slack thread. I'm not sure I fully understand the implications described there but I guess it would be good to solve this for anonymous users, while not breaking it for those more complex access scenarios if that's possible.

@andy_w: the menu_uncache_preprocess_block() will need to be more complex than that. you just add one more tag. You will need to process the finished and bubbled-up result before it gets written into the cache to *remove* all other cache tags. Will need to look into where exactly that needs to happen.

Just had a quick look at the MR, but as mentioned, this will not change much in practice for most sites with menu links pointing to nodes because of the bubbling of the access cacheability metadata.

to see this, enable render cache debug output ( https://www.drupal.org/node/3162480 → ) and then it's important to visit the page as an anonymous user, because users with bypass node access wont' have them, then you get this output:

<!-- START RENDERER -->
<!-- CACHE-HIT: No -->
<!-- CACHE TAGS:
   * block_view
   * config:block.block.BLOCK_ID
   * config:system.menu.main
   * node:3
   * node:4
   * node:34
   * node:30
-->
<!-- CACHE CONTEXTS:
   * route.menu_active_trails:main
   * languages:language_interface
   * theme
   * user.permissions
-->
<!-- CACHE KEYS:
   * entity_view
   * block
   * BLOCK_ID
-->
<!-- CACHE MAX-AGE: -1 -->
<!-- PRE-BUBBLING CACHE TAGS:
   * block_view
   * config:block.block.BLOCK_ID
   * config:system.menu.main
-->
<!-- PRE-BUBBLING CACHE CONTEXTS:
   * route.menu_active_trails:main
   * languages:language_interface
   * theme
   * user.permissions
-->
<!-- PRE-BUBBLING CACHE KEYS:
   * entity_view
   * block
   * BLOCK_ID
-->
<!-- PRE-BUBBLING CACHE MAX-AGE: -1 -->
<!-- RENDERING TIME: 0.004674911 -->

As long as these node:ID cache tags are there, the only scenario this solves is manually going to the menu edit form and save without making a change, that's not that common and useful. What's common is saving a node with a referenced menu link, and that will invalidate the node cache tag and with that all menus and pages using that node.

Also, instead of this I think it would more interesting to not save the menu link in the first place from the node form if no change is detected. That seems more reliable than this, because there are scenarios when things on the menu link entity are changed that might influence the menu that isn't stored in menu tree, like additional fields.

In terms of not saving the link in the first place from the node form, I just did a quick debug on my own site and noticed that MenuTreeStorage::save is triggered by 3 modules. First is menu_ui, the other is token module and the third is our own custom submit handler. So even if we preventing saving in menu_ui, a lot of people use token so it would still occur for them.

Is something like what @andy_w posted feasible to do in core for sites that don't have complex access control?

Multiple saves sounds like it's own separate problem, the token stuff is a hack, but I thought that we only save when necessary, if you see something else, check for an existing issue or open one. And your own submit could maybe replace the default menu_ui one then.

I don't think #10 can be done in core, a separate contrib module maybe, where it can document what kind of limitations it has. The access check comparison is not trivial, I'm pretty sure the presave() + update check there doesn't really work like that, that makes a number of assumptions and likely relies on static cache to already have those access results.

Two things, we are trialling the approach above and it seems to be working in our scenario, but obviously couldn't account for all possible options.

Secondly I totally agree that fixing this in menu_ui would certainly be the best approach, I failed to achieve this last time (it was my first area of interrogation, but I'll take another look).

So this is the original route I was looking at here: https://git.drupalcode.org/project/drupal/-/merge_requests/10144 (from https://www.drupal.org/project/drupal/issues/3486604#comment-15852875 🐛 Node edit form re-saves menu items even if no change has occurred Active )

So, I did two things now.

1. A new merge request that implements my suggestion of not saving menu links, which is similar to the linked issue and merge request in #16. I did explicit checks because I'm not sure if hasTransationChanges() behaves correct on untranslatable fields like the menu name.

2. I created new contrib project menu cache: https://www.drupal.org/project/menu_cache → . This is a proof of concept that removes node cache tags from menu blocks (this was not trivial to figure out, needed to add a pre_render callback in hook_block_view_alter() and adds explicit invalidation of the existing menu cache tag if the node is published or unpublished. It's very much a proof of concept, but it has a fairly extensive test that shows that it works for the scenario that it supports when combined with the new MR here.

I did not yet look into combining it with token module and the apparent separate save.

Great stuff, that was exactly how I started out when investigating it (see https://git.drupalcode.org/project/drupal/-/merge_requests/10128/diffs#n...), I know there was an initial suggestion that we could use hasTranslationChanges (see https://www.drupal.org/project/drupal/issues/3486604#comment-15851542 🐛 Node edit form re-saves menu items even if no change has occurred Active ).

Not sure about your thoughts on that approach.

So I spent quite some time looking into that test fail.

And it's a mess. I don't know the people in #3041326: Remove 'title' and 'description' from MenuSettingsConstraintValidator when used with content moderation by creating a draft of menu link content when a draft of it's parent content is created → were thinking with that change, that doesn't work at all. (hint: my name is in the commit credits. well done past me, truly well done). It resaves default-revisions as non-default and even attempts to create new non-default-revision entities. The way MenuLinkContent::postSave() works does in fact result in not immediately pushing that change to the menu tree storage, but it's still half-stored (only in the revision tables and not the data table) and any future resave will result in it being updated.

I was able to fix the first fail by switching menu_ui to saving a non-default revision as a new revision. But then it got even weirder, with the expectation that you can create a new menu link during creating a non-default node draft revision. It's one thing to add a menu link for an initially unpublished but default revision node, but not a pending draft.

The reason it becomes visible with this change is that this new-and-kinda-non-default-revision menu link is actually the default revision again when being loaded. So I'm unable to detect a change since the node I'm saving also is the default revision now.

I don't see how I can make this work properly, I think we need to remove and disallow that possibility. I could probably pull some shenanigans, like saving the menu link twice, so we actually get a draft, but that just feels so wrong.

Also. The fact that you can resave an existing default revision of an entity as a non-default revision is IMHO a critical entity bug, will see if there's an issue about that one.

🐛 Disallow saving the current default revision as a non-default revision Active