Avoid saving menu links through node form when they do not change

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

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.

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 (now closed as a duplicate of this)

My proposal at that point was checking if access to the saved node had changed and if it had using that to invalidate a custom menu cache tag.

use Drupal\Core\Cache\Cache;
use Drupal\user\Entity\User;
use Drupal\node\NodeInterface;

/**
 * Implements hook_preprocess_block().
 *
 * Add a menu_published cache tag to any menu block, this allows the menu
 * block to be cleared in the cache if a change is made to a published menu.
 */
function menu_uncache_preprocess_block(&$variables) {
  if (!empty($variables['base_plugin_id']) && $variables['base_plugin_id'] === 'menu_block') {
    $variables['content']['#cache']['tags'][] = 'menu_published:' . $variables['elements']['#derivative_plugin_id'];
  }
}

/**
 * Implements hook_node_presave().
 *
 * If a node is in the menu structure then store its current view access, this
 * can then be used a comparison to see if the view access has changed for the
 * current node, if this has changed we can then clear the menu cache.
 *
 * When view access is requested the latest stored default revision is used,
 * therefore this is fine to use even if a non-default revision is being
 * updated.
 */
function menu_uncache_node_presave(NodeInterface $node) {
  if ($node->isDefaultRevision()) {
    $links = \Drupal::service('plugin.manager.menu.link')->loadLinksByRoute('entity.node.canonical', ['node' => $node->id()]);
    if (!empty($links)) {
      // A check for anonymous view access will use the latest default revision.
      $node->previous_default_anonymous_revision_access = $node->access('view', User::getAnonymousUser());
    }
  }
}

/**
 * Implements hook_node_update().
 *
 * If a node in the menu has changed its view operation access
 * then invalidate the cache for the menu, therefore forcing the menu to be
 * rebuilt.
 *
 * See https://www.drupal.org/project/drupal/issues/3486604
 */
function menu_uncache_node_update(NodeInterface $node) {
  if ($node->isDefaultRevision()) {
    $links = \Drupal::service('plugin.manager.menu.link')->loadLinksByRoute('entity.node.canonical', ['node' => $node->id()]);
    if (!empty($links)) {
      if ($node->previous_default_anonymous_revision_access !== $node->access('view', User::getAnonymousUser())) {
        $affected_menus = [];
        foreach ($links as $link) {
          $definition = $link->getPluginDefinition();
          if (!empty($definition['menu_name'])) {
            $affected_menus[$definition['menu_name']] = $definition['menu_name'];
          }
        }
        if ($affected_menus) {
          $cache_tags = Cache::buildTags('menu_published', $affected_menus, ':');
          \Drupal::service('cache_tags.invalidator')->invalidateTags($cache_tags);
        }
      }
    }
  }
}

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

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)

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

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.

#3499181: Disallow saving the current default revision as a non-default revision

The blocker went in, this needs a rebase.

Rebased, the menu_ui/content_moderation test is now green!

Next step is writing tests that ensure that the menu link isn't saved/cache tags aren't invalidated in this scenario. Maybe in the form of a performance test.

Also could 1 MR be closed or hidden.

Added some test coverage for the cache tag invaidation.

> Also could 1 MR be closed or hidden.

Well. My MR is a completely different implementation, no decision has been made.

I also rebased that MR, bunch of random fails but still seems to pass.

I'm unsure myself after thinking about it some more. The earlier MR requires changes to what the protected method returns, that would need discussion on what we consider an API change. On the plus side, it requires fewer changes and works for anyone existing additional code saving menu links as well, such as apparently token.

On the other hand, completely skipping the menu_link_content entity save seems like a sensible optimization on its own, avoids multiple delete/insert queries. Maybe we could even do both and combine it.

Either way, for this to achieve the actual goal for non-admin users, my contrib project is needed on top of it.

After thinking about it even more, I think changing the storage like that is not an option. The problem is that those menu links are plugins, they can load and do stuff that might not be in the definition. For example menu_link_content and translations. Those are not stored in in the menu_tree table but multilingual sites load the menu_link_content entities and then get the title and description from that.

What I was confused about is why this doesn't fail then. But the fun fact is that what wrote above is technically not true. \Drupal\menu_link_content\Entity\MenuLinkContent::postSave() and getPluginDefinition() don't care about languages at all. On every postSave() they write the title and description of the currently active language into the menu_tree storage. So the test works because when you save/update a menu link translation, it persists that specific language into menu_tree and then ignores it at runtime. So this only works as log as the changed language is the active one while saving. Created #3511766: Store menu_link_content translations in menu_tree about that.

There are other scenarios that might not work, such as menu_link_content fields that aren't stored in the definition.

I think that's too much of a behavior change.

Also created #3511768: Avoid incorrect and unecessary menu_link_content entity saving

Who's a good decision to make the call? Maybe both MRs could be placed in the proposed solution section with what they're doing differently so someone could review faster.

Well, per #26, the older MR is a behavior change, so IMHO, mine is the only feasible option. But would be good if someone else would confirm my arguments.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased.

I think this is pretty close the concern in 7 has been addressed with the change in direction.

The needs save path is a little awkward but I don't see a better way to do it. And when reviewing the actual file with the change it's pretty straightforward.

I wonder if this needs a cr for people with custom fields on the menu items to update their save implementation.

needs save: There are API's like hasTranslationChanges(), but I don't fully trust them to reliably do what we want, they are also quite inefficient.

Yes, a CR certainly makes sense, I'll prepare that. menu_ui doesn't have any official extension points/support for additional fields. But we do have a custom solution in our project, I'll review that in scope of this, maybe I can sneak in an extension point to cover that scenario.

This looks a lot better than what we had before. Yes it's awkward with the large amount of individual checks, but at least it ensures we only save the menu link when there's a reason to save it, taking way unnecessary cache invalidation.

I wonder if there's no easier way to detect changes using toArray(). Might be less performant and may require looping over translations, but then you could get rid of the individual checks and numerous $needs_save flags. Just a thought, though, don't let that stop you from moving ahead.

As mentioned, hasTranslationChanges() might be an option, especially if we also add an extension point (in our case, we expose the published checkbox within the node form as well and need to sync that and it's really tedious right now). There are fields that might change that we don't consider a real change. I could try that. If we set the origin explicitly it might not be a performance concern.

I rebased and switched to hasTranslationChanges(). Works well for the scenario in the test.

Has test fails.

Yay tests, hasTranslationChanges() does not handle changing the default revision change, so needs an explicit check for that, but only that.