Use 'menu:<menu name>' as cache tag for menus instead of 'config:system.menu:<menu name>' to avoid coupling

+++ b/core/modules/system/src/Entity/Menu.php
@@ -77,4 +77,15 @@ public function isLocked() {
+  /**
+   * {@inheritdoc}
+   *
+   * Override the default cache tags to use the cache tags that are not coupled
+   * to Menu config entities (i.e. this class), to not force other parts of the
+   * menu system to couple themselves to Menu config entities.
+   */
+  public function getCacheTags() {
+    return ['menu:' . $this->id()];

Should we also add config: as well, just in case someone needs it.

Should Config::save() not check if the entity it is saving implements CacheableDependencyInterface and then merge those cacheTags in, too?

Spoke with @alexpott in IRC:

There are 2 ways:

- Use the normal entity hooks and implement them in menu module, to invalidate tags on save
- Do it like views or VocabularyStorage and subclass ConfigEntityStorage and override the save() / resetCache() methods to do our own invalidation

After thinking about it, its still worth it to have clearer semantics here as this cache tags will be output by almost every Drupal 8 site online, so it would be good to have less coupling ...

CNW for the above changes, needs a beta evaluation.

1. +++ b/core/modules/system/src/Entity/Menu.php
   @@ -77,4 +78,15 @@ public function isLocked() {
   +   * {@inheritdoc}
   +   *
   +   * Override the default cache tags to use the cache tags that are not coupled
   +   * to Menu config entities (i.e. this class), to not force other parts of the
   +   * menu system to couple themselves to Menu config entities.
   +   */
   +  public function getCacheTags() {
   +    return ['menu:' . $this->id()];
   +  }
   

   AFAIK, it's officially still not allowed to mix @inheritdoc and additional docblock comments. I'd recommend to make this an inline comment to avoid discussions about this...

2. +++ b/core/modules/system/src/MenuStorage.php
   @@ -0,0 +1,31 @@
   +   *
   +   * The menu API doesn't require one to use Menu config entities. Hence the
   +   * Menu config entity should not use config-specific cache tags, but generic
   +   * ones instead. That's what this code guarantees.
   

   Same.

Tests pass, the llama is happy, solution looks good, RTBC!

Seems fine

CNW per #16, cross-post (also to avoid commit conflicts with the more important cache menu links issue).

Did that, since it's my fault that it's needed ;)

Also not sure why the cache tag invalidation happens in MenuStorage. Trying something....

Clearing the cache tag in the method that we also use everywhere else seems like a much more logical thing to do? :)

berdir++ - We only used the custom storage, because of #9(I spoke with alexpott and he saw no other ways), but this way is obviously better.

RTBC!

I think we also need to implement invalidateTagsOnDelete. Also how about not calling the parent to save a query and couple the classes less?

+++ b/core/modules/system/src/Entity/Menu.php
@@ -100,6 +101,16 @@
+  protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_type, array $entities) {
+    parent::invalidateTagsOnDelete($update);

Yeah, that won't fly ...

I guess I was _little_ tired last night. Fixing my own mess

+++ b/core/modules/system/src/Entity/Menu.php
@@ -77,4 +78,39 @@ public function isLocked() {
+    parent::invalidateTagsOnSave($update);
+    // The menu API doesn't require one to use Menu config entities. Hence the
+    // Menu config entity should not use config-specific cache tags, but generic
+    // ones instead. Drupal\Core\Config\Entity\ConfigEntityBase explicitly
+    // overrides the default implementation and does not invalidate the specific
+    // cache tag, this adds that again.
+    if ($update) {
+      Cache::invalidateTags($this->getCacheTags());
+    }
...
+    parent::invalidateTagsOnDelete($entity_type, $entities);
+    foreach ($entities as $key => $value) {
+      Cache::invalidateTags($entities[$key]->getCacheTags());
+    }

It super sub-optimal do have multiple calls to Cache::invalidateTags(). Each call is a query I think. So we can merge the arrays of tags first and do a single call to Cache::invalidateTags() for both save and delete. Plus the green patch in #21 shows we're missing test coverage.

Just tagging first

I doubt to be honest.

I can think of theoretical ways we could add bc for this, but it seems like the kind of @internal bc break we could save up for a major. There's no actual coupling/dependency here, it's just a string, so bumping down to minor.

This would be a minor-only addition and deprecation. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

There's nothing specific this is postponed on, so marking active.

Putting back to "needs work" as there is some code here, it needs to be turned into a merge request.

The code needs some further tidying up.

I have my doubts that this is worth doing. Everything that uses these strings would break unless we add some elaborate BC layer for it: https://search.tresbien.tech/search?q=config%3Asystem.menu%20-r%3Adrupal....

Also adding the return type is a BC break and unrelated. if phpstan complains, just add it to the baseline.

We could rework the menu_ui module and create two separate forms. One would be used to edit the menu settings (config entity) and the other would be used to rearrange the menu links within the menu. Editing the menu settings would then become a local task.