Render cache should be cleared when menu link or entity is updated

Thanks @acbramley for the specific method of reproducing the problem. I am still coming up to speed on D8 caching and trying to clear some time in my schedule for making a more detailed study of it (the official documentation out there at this time isn't enough to provide that).

Any recommendations from someone with more experience with D8 caching would be welcome in the meantime. I have tried to keep the caching rules in one place in the code along with comments about why those were decided upon. Certainly they are not set in stone & could be replaced with revised code, or tags & context, suggested here.

Came here to find this issue.

I initially posted about it here:

http://drupal.stackexchange.com/questions/222615/breadcrumbs-are-cached

4k4 mentions:

In this case you need $breadcrumb->addCacheableDependency($entity) for MenuLinkContent where this module gets the title from.

Thanks to 4k4, changing this will cause the cache to update if a menu link entity is updated:

    // Generate basic breadcrumb trail from active trail.
    // Keep same link ordering as Menu Breadcrumb (so also reverses menu trail)
    foreach (array_reverse($this->menuTrail) as $id) {
      $def = $this->menuLinkManager->getDefinition($id);
      $entity = $this->entityTypeManager->getStorage('menu_link_content')->load($def['metadata']['entity_id']);

      // Invalidate cache if a menu link title changes
      // @see http://drupal.stackexchange.com/a/222667/57
      $breadcrumb->addCacheableDependency($entity);

      if ($translated) {

      .... redacted

I moved $entity = ... out of the translated block so this could be possible.

I tested this locally and was unable to reproduce my original issue regarding breadcrumb title not updating.

I'm not able to make an official patch at the moment, will later today when I get a chance.

through the code at least, you have connected with the recent translation issue, for which the fix caused another pathological case (2831084).

Looking up the full entity, thought only to be needed to translate the menu title, was only done on multilingual sites for performance reasons. Yet thanks to you & 4k4 we now know it must be done every time, in order to add the menu link content as a cacheable dependency.

I would be ready to update the module that way right now, if not for the bug pointed out in that issue which would then affect every site: specifically, views added to a menu only through their config settings won't have a way of finding their entity through their metadata. In fact I don't think such views even have MenuLinkContent (that's why I originally wrote it to read the settings from the menu plugin definition).

So we need both:

1. either a 100% reliable way of converting a menu plugin ID to MenuLinkContent or a way of getting an active trail full of cacheable and translatable entities
2. to be sure this will work on Views, even those that don't have MenuLinkContent.

Please correct me wherever I may be wrong. I am summarising this in a comment to the thread on drupal stackexchange to see if we can get a full solution.

I am not on a translated site or using any locale stuff, I am probably not the best test in that case. I also am not well versed in the new Cache subsystem but would love to know more (about it).

actually I can't comment on that StackExchange thread due to their high "reputation" bar, so posted the request as a separate "answer" here.

I just gave you a boost. You're at 54 now.

dear @kevinquillen - your code above, with the entity lookup brought out of the "translated?" condition, should make the problem visible on your own site. According to the related issue, you should see it on any views that you mount on a menu through the Views UI rather than the Menu UI, as well as any builtin views (like Archive & Glossary).

Thanks for the boost up, will clean up that thread accordingly & watch for any answers from 4k4 & others...

Hmm, I don't have any Views yet that set a menu link. I only have one View outputting a block. Probably why I don't see it.

I've found the following menu link providers:

MenuLinkContent
This is the content entity mentioned in #6. Used for menu links entered in ui in node and menu forms. Caching is solved acc. #7.

ViewsMenuLink
Depends on the view config, so the config entity from the view needs to be the cache dependency.

LoginLogoutMenuLink
The title is generated dynamically: "log out" if the current user is authenticated, otherwise "log in".

InaccessibleMenuLink
The title is always 'Inaccessible'.

DestinationMenuLink
Modifies the menu link to add a destination, doesn't change the title.

And finally there are menu links provided in yaml files. Didn't found the class for this.

So it seems that only 2 menu link provider plugins depend on entities which can be added as cache dependencies.

I'm afraid that is all I can help here, because I've never worked with menus in D8.

Instead of using the Drupal functions directly to get the active trail, we can use the menu tree functions like this sandbox version of an earlier module release. That will return all of the menu link providers above, via MenuLinkTreeElement->link, which can each then be added as a cacheable dependency.

This will be a bit of a rewrite so will push out as dev version as soon as possible. Once sure this fixes the caching problem then we will have to address the translation issue from scratch, since none of those plugin items are translatable objects (unless the menu tree itself is already translated or translatable).

The sandbox version looks good. It works for all menu link plugins, also for future ones. But caching can still be a problem. Although the menu link interface has cacheable dependencies, there is only a placeholder in MenuLinkBase:

  public function getCacheTags() {
    return [];
  }

None of the essential menu link plugins define a more specific getCacheTags() method. So if I didn't miss something, this code doesn't work:

$breadcrumb->addCacheableDependency($element->link);

For the MenuLinkContent plugin you could think about this code, but this doesn't work either:

if ($element->link instanceof MenuLinkContent) {
  $entity = $element->link->getEntity();  // not working, because getEntity is protected
  $breadcrumb->addCacheableDependency($entity);
}

What is missing, is the getCacheTags() method in the class MenuLinkContent, which gets the tags from the protected entity. Hope there is somewhere a solution, but I'm out of options.

#17 was the solution I was just about to get working on ($breadcrumb->addCacheableDependency($element->link);). Though we know it won't fix the problem immediately, I wonder if it would still be acceptable to revise the code as such and mark this issue as "blocked" pending a full caching implementation for MenuLinkBase & its descendants.

We had to do something similar for another issue (2820206) after relating it explicitly to an unresolved core issue. I'm not in the Drupal community long enough to know to what extent contrib modules are expected to work around bugs or incompletions in core, so any veterans please feel free to chime in. In the meantime I'll look for or file a core issue about this as well.

Guys, I kind of see the problem in the code and understand your discussion and concern about cacheability metadata, but I couldn't reproduce the issue. I came here from #2831084: Undefined index: entity_id in the MenuBasedBreadcrumbBuilder::build() method so my original concern was that ugly PHP notice.

I would propose to keep it reasonable: the job of menu_breadcrumb module is to build breadcrumb and not to bookkeep internal cacheability/data structure of whoever/whatever implementations of MenuLinkInterface.

I propose the following: let menu_breadcrumb just take care of building the breadcrumb, along the way polling all "MenuLink" participants of the breadcrumb for cacheabilty, but going deeper than invoking ::addCacheableDependency() shouldn't be menu_breadcrumb's business. If some MenuLink does not property report its cacheable metadata then it's a bug in that code and again, none of menu_breadcrumb business.

I attach a patch where I follow this approach. The "heart" of breadcrumb building went from:

    foreach (array_reverse($this->menuTrail) as $id) {
      $def = $this->menuLinkManager->getDefinition($id);
      if ($translated) {
        $entity = $this->entityTypeManager->getStorage('menu_link_content')->load($def['metadata']['entity_id']);
        if ($entity instanceof TranslatableInterface && $entity->hasTranslation($langcode)) {
          $parent = $entity->getTranslation($langcode);
          $def['title'] = $parent->getTitle();
        }
      }
      $def_route_name = $def['route_name'];
      if ($def_route_name) {
        $url_object = Url::fromRoute($def_route_name, $def['route_parameters']);
      }
      else {
        // If no route, it's an external URI (issue 2750821):
        $url_object = Url::fromUri($def['url']);
      }
      $links[] = Link::fromTextAndUrl($def['title'], $url_object);
    }

to

    foreach (array_reverse($this->menuTrail) as $id) {
      $plugin = $this->menuLinkManager->createInstance($id);
      $links[] = Link::fromTextAndUrl($plugin->getTitle(), $plugin->getUrlObject());
      $breadcrumb->addCacheableDependency($plugin);
    }

The 2nd way looks much clearer and logical to me: just get a MenuLink plugin instance, query the plugin instance for a link, add its cacheability metadata into our breadcrumb. Done.

Along the way I introduced a small Unit test that makes sure MenuLink plugin instances are actually queried for their cacheability metadata when breadcrumb is being built. I had to alter a few things here and there within MenuBasedBreadcrumbBuilder to make it more adapted for unit testing, but 90% of those "here and there" changes was really cosmetic (going from specific class to an interface or alike).

As I said, for some reason I couldn't reproduce the bug on my localhost, so I am not 100% sure I've really addressed the issue, but I did test overall breadcrumb building in this alternative version and it worked fine.

If somebody could share more precise steps to reproduce, I'd be happy to follow them and double-check on that end.

I'm also somewhat away from my coding environment. The cleaner code to get at those plugin instances is helpful & I've asked loads of people on Drupal forums how to do exactly that, so thanks very much.

Yet if memory serves me correctly (while I am getting my environment back) those plugin instances are not translatable. In this model, how would we extract the translated menu title?

Hi, rphair!

The plugin instances the foreach iterates over will be implementations of MenuLinkInterface. Looking at phpdoc of MenuLinkInterface::getTitle() we see:

interface MenuLinkInterface extends PluginInspectionInterface, DerivativeInspectionInterface, CacheableDependencyInterface {

  // ...

  /**
   * Returns the localized title to be shown for this link.
   *
   * @return string
   *   The title of the menu link.
   */
  public function getTitle();

  // ...
}

So, if some implementation does not return the correct translation then it's a bug in that implementation :)

But to be sure, let's examine all existing implementations of MenuLinkInterface.

InaccessibleMenuLink goes as

  public function getTitle() {
    return $this->t('Inaccessible');
  }

Translation-safe

MenuLinkDefault goes as

  public function getTitle() {
    return (string) $this->pluginDefinition['title'];
  }

Translation safety: not really sure. I couldn't quickly find how to create a menu item that will use this implementation to check it in practice, however, I would believe that Plugin API is smart enough to be translation aware. If you know how to create a menu item that maps into this implementation, please, let me know :) Otherwise I'll test by triggering the process somehow programmatically.

MenuLinkContent

  public function getTitle() {
    // We only need to get the title from the actual entity if it may be a
    // translation based on the current language context. This can only happen
    // if the site is configured to be multilingual.
    if ($this->languageManager->isMultilingual()) {
      return $this->getEntity()->getTitle();
    }
    return $this->pluginDefinition['title'];
  }

Translation safe

LoginLogoutMenuLink

  public function getTitle() {
    if ($this->currentUser->isAuthenticated()) {
      return $this->t('Log out');
    }
    else {
      return $this->t('Log in');
    }
  }

Translation safe

ViewsMenuLink

  public function getTitle() {
    // @todo Get the translated value from the config without instantiating the
    //   view. https://www.drupal.org/node/2310379
    return $this->loadView()->display_handler->getOption('menu')['title'];
  }

Translation safe: I just tested it and asserted the returned menu link title is translated.

So in theory it must be translated and in practice I've already tried all but 1 implementation and confirmed it is actually translated.

thanks so much @bucefal91, this is thrilling & I can't wait to check it in, just for now wanted all to know that I agree with your points. I apologise to have so little time to progress this right now & as soon as possible with get this into dev, then a bit of testing on my end, then new Beta release: hopefully all in short succession. Will also post this to translation issues reported so far, so those cases can be re-tested.

sorry about holiday-related delays, have tested OK here & will keep an eye on it pending a release candidate if continues to hold up in community testing; patch included in 8.x-1.0-beta6. Also this definitively fixes https://www.drupal.org/node/2831084 since there is no need for the entity lookup. Thanks for the great community support on this one.

Thanks to you! :) I know it's holidays season and often module maintainers even in normal "business" seasons are slow on reviewing/accepting patches, so I am pleased to see such involvement from your side :)

It seems like this issue is back in 8.x-1.1, I am updating the menu link title on a node form and saving. The breadcrumb does not reflect the change until I clear the site cache.

It can't be "back in 8.x-1.1" since none of the caching code changed from 8.x-1.0 ... though reports of breadcrumb caching coming and going have happened in the meantime. As far as I can tell these are due to changes on the back end.

I'm attaching another issue where this problem manifested, then disappeared in a further Drupal version, during this recent period without any caching changes to Menu Breadcrumb.

I would welcome a report of Menu Breadcrumb using the wrong caching strategy, after the comprehensive review above, but without that I would conclude that this problem is upstream.

It happens to me the same than Kevin, Breadcrumb is not updated until I clear cache. I tried in a fresh Drupal 8.3.2 installation using simpletest.me

I will try later with different versions of Drupal.

Thanks!

#30: Menu Breadcrumb is using the correct caching strategy. But the MenuLinkContent plugin still returns no cache tags. Just checked this in 8.4 HEAD. So this affects all D8 versions.

Menu Breadcrumb could add the tags with some code like this, until this is fixed in core:

foreach (array_reverse($this->menuTrail) as $id) {
  $plugin = $this->menuLinkManager->createInstance($id);
  $links[] = Link::fromTextAndUrl($plugin->getTitle(), $plugin->getUrlObject());
  $breadcrumb->addCacheableDependency($plugin);
  // In the last line the MenuLinkContent plugin is not providing cache tags.
  // Until this is fixed in core add the tags here:
  if ($plugin instanceof \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent) {
    $uuid = $plugin->getDerivativeId();
    $entities = $this->entityTypeManager->getStorage('menu_link_content')->loadByProperties(['uuid' => $uuid]);
    if ($entity = reset($entities)) {
      $breadcrumb->addCacheableDependency($entity);
    }
  }
}

thanks @k4, will put this into dev as soon as I have a chance to properly test what I am doing; others please stay tuned...

opened up separate issue (#2887053: Manually add cache tags for MenuLinkContent) to treat this workaround separately from regular logic of the module (which I believe is robust). Please test on affected sites, either with dev version or with 8.x-1.2 plus patch at 2887053->#3. If reports of fixing all caching problems will release as 8.x-1.3.

nearly 2 weeks with no reported problems from dev users, so have released this single change as 8.x-1.3. Dear @k4, when you notice that this problem is fixed upstream please drop us a line & I will strip out the relevant patch, thanks again...

I'm having this issue again, logged in and anon, on a multilingual site. I'll have to look into it more, but wanted to at least report it.

As soon as we either find a test case that reproduces this problem in a current version, or get confirmation here that comment #32 is no longer true (i.e. MenuLinkContent plugin is finally returning cache tags), I could try backing out the patch for child issue #2887053 from dev to see if Drupal 8.5 will finally do the right thing on its own.

Any scrutiny of the code in that patch would also be welcome in case there is a better approach. Also anyone who knows a core issue corresponding to the missing cache tags please post here. To attract all these kinds of attention I'm opening this issue up again.

Though this module isn't responsible for fixing caching fundamentals it has been nice to have that workaround in place to support this module's adoption. Still I would prefer to remove the patch if it's no longer doing the right thing.

Issue 2977598 changes the caching rules to invalidate cached breadcrumbs when either the path or the active trail changes. I believe this should encompass the cache issues reported here: please reopen if not.

#39: I don't think there is a connection. Issue 2977598 reverts the change New 'path parent' cache context for breadcrumbs. This issue on the other hand is about cache tags, so that the breadcrumbs can be invalidated in cache when menu links are modified in database, for example change the menu link title as described in the steps to reproduce.

Thanks @k4 - you are right - and I have stated badly my desire to draw out any remaining caching problems for this module now that a few versions of Drupal have come out since this issue was opened long ago. I hope anyone seeing cache problems will therefore link it here even if a new issue is opened, so the people have been helpful so far can help certify the cache behaviour of the module going forward.