Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable

See #507488-319: Convert page elements (local tasks, actions) into blocks, this makes /user/1 10% faster for the anonymous user.

Holy shit, what a clusterfuck.

…

We have:

/**
 * Alter local tasks plugins.
 *
 * @param array $local_tasks
 *   The array of local tasks plugin definitions, keyed by plugin ID.
 *
 * @see \Drupal\Core\Menu\LocalTaskInterface
 * @see \Drupal\Core\Menu\LocalTaskManager
 *
 * @ingroup menu
 */
function hook_local_tasks_alter(&$local_tasks) {
  // Remove a specified local task plugin.
  unset($local_tasks['example_plugin_id']);
}

which is fine.

But we also have:

/**
 * Alter tabs and actions displayed on the page before they are rendered.
 *
 * This hook is invoked by menu_local_tasks(). The system-determined tabs and
 * actions are passed in by reference. Existing tabs or actions may be altered.
 *
 * @param array $data
 *   An associative array containing tabs and actions. See
 *   hook_menu_local_tasks() for details.
 * @param string $route_name
 *   The route name of the page.
 *
 * @see hook_menu_local_tasks()
 *
 * @ingroup menu
 */
function hook_menu_local_tasks_alter(&$data, $route_name) {
}

… which is … actually fine, we can cache it per route, but still. It's not actually fine, because e.g. contact_menu_local_tasks_alter() not only looks at the route, but depends on yet more state: it loads the user being contacted on the contact page, and checks if they have an e-mail address. Still cacheable, but yeah…

…

It gets better!

…

The above is why we DON'T use LocalTaskManager::getLocalTasksForRoute() to determine the local tasks (as one would expect)… no, we use menu_local_tasks(), which calls LocalTaskManager::getLocalTasksForRoute(), but THEN still alter it FURTHER using both menu_local_tasks() and menu_local_tasks_alter().
So… in trying to decouple local tasks from the menu system, we've introduced a new alter hook, but we've still kept the two old hooks, and we still have to use the old API. It's pretty clear that this API is far, far from completed. I think we should mark all of it as @internal NOW, so that if we don't fix it before release, we still can do so later. The current API is all sorts of confusing and broken.

I'm gonna work on something else, because this makes me wanna punch myself and — worse — kill kittens.

Here's a partial patch of what I did before getting too frustrated.

I'm also willing to bet that most of the slowness of local tasks/actions reported in #507488: Convert page elements (local tasks, actions) into blocks is directly caused by the confusing amalgamation explained in #4.

Here is a description in way less words.

• hook_menu_local_tasks(): Dynamic local tasks on runtime
• hook_menu_local_tasks_alter(): Runtime alter hook for local tasks
• hook_local_tasks_alter(): Static alter the plugins defined in yml files

Done

It seems like local tasks, local actions, and contextual links should implement CacheableDependencyInterface ?

#8: +1

And then the ones that just depend on links.*.yml files, like ContextualLinkDefault, would simply return no tags, no contexts, and a permanent max-age.

Should we still wait to start on this until the block conversion goes in?

IMHO this can start already.

Showing up as a performance suck in current flamegraphs. Tagging.

#507488: Convert page elements (local tasks, actions) into blocks just landed. Let's do this!

The IS is clear, but I think we need a plan how to resolve the cacheability concerns around this.

All Plugins Are Created Equal™

If we collect plugin definition cacheability metadata through a separate object, we will not only have to add an argument to the alter hook, but add it to the original plugin discovery, and derivers as well. This would be a BC break for discovery methods and derivers. This would also limit this feature to local tasks only, as we'd only convert that alter hook.

Instead, this patch proposes to include cacheability metadata inside plugin definitions. This way, discovery methods, derivers, *and* alter hooks can add/alter cacheability, and we can use the metadata outside plugin managers as well (useful for Plugin Selectors, for instance, which build render arrays based on plugins). It also automatically makes this available for any plugin manager that extends DefaultPluginManager, so it's not just limited to local tasks.

This should fix the fatals.

This approach does not break BC, but it does break the tests for (almost) all plugin managers.

Never mind those patches. I misunderstood the problem. It's really just about hook_menu_local_task_alter() implementations not setting #cache properly.

See #2564011: Use local task/action cacheability metadata when displaying them for an improved, more in-scope version of what I was trying to do earlier.

Looking good!

1. +++ b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalTasksBlock.php
   @@ -88,7 +89,8 @@ public function defaultConfiguration() {
   +    $cacheable_metadata->addCacheContexts(['route.name']);
   

   Why route.name instead of route?

2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -9,6 +9,7 @@
   +use Drupal\Core\Cache\CacheableMetadata;
   

   Unused use statement.

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -329,6 +330,7 @@ public function getTasksBuild($current_route_name) {
   +          '#cache' => ['contexts' => ['route']],
   

   Should be route.name, because the local tasks determined to be shown only depends on the current route' name:

   $tree = $this->getLocalTasksForRoute($current_route_name);
   

   Also, it should not add that here, to an individual local task, but to $build. If $build does not get any local tasks, then that is because no local tasks exist for that route name, and that emptiness varies by the route.name cache context.

2. +++ b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalTasksBlock.php
   @@ -88,7 +89,8 @@ public function defaultConfiguration() {
   +    $cacheable_metadata->addCacheContexts(['route.name']);
   

   This is not necessary. LocalTasksBlock::build() calls LocalTaskManager::getLocalTasks(), which calls LocalTaskManager::getTasksBuild(), which is the method I commented on above, and that always adds that cache context.

3. +++ b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalTasksBlock.php
   @@ -166,7 +141,7 @@ public function blockForm($form, FormStateInterface $form_state) {
   -    $form['levels']['secondary'] = [
   +    $form['leves']['secondary'] = [
   

   Oops :)

4. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -9,6 +9,7 @@
   +use Drupal\Core\Cache\CacheableMetadata;
   

   Unnecessary, can be reverted.

5. +++ b/core/modules/contact/contact.module
   @@ -98,7 +99,8 @@ function contact_entity_extra_field_info() {
   +  $cacheable_metadata->addCacheContexts(['user', 'route']);
   

   s/route/route.name/

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -302,9 +303,7 @@ public function getTasksBuild($current_route_name) {
+    ¶

Trailing whitespace.

The local tasks might be based on the route name, but e.g. different nodes obviously have different route parameters and resulting, different links/HTML even though the route name is the same?

The local tasks might be based on the route name, but e.g. different nodes obviously have different route parameters and resulting, different links/HTML even though the route name is the same?

D'oh, yes.

That is why:

$route_parameters = $child->getRouteParameters($this->routeMatch);

receives the route match. Because \Drupal\Core\Menu\LocalTaskDefault::getRouteParameters() extracts the route parameters to use for the task from the route match.

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskInterface.php
   index 7055628..8cfaca0 100644
   --- a/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   
   --- a/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   

   If you change the LocalTaskManager class you should also expand the unit tests of this class.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -177,6 +178,17 @@ public function getTitle(LocalTaskInterface $local_task) {
   +  public function getCacheableMetadata(LocalTaskInterface $local_task) {
   

   Is there a reason why this method needs to be public? I don't see any use of it in the public ...

3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -323,13 +333,16 @@ public function getTasksBuild($current_route_name) {
   +        $cacheable_metadata->merge($this->getCacheableMetadata($child));
   

   ->merge just returns a new instance, so you need to update $cacheable_metadata itself

4. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -340,22 +353,27 @@ public function getTasksBuild($current_route_name) {
   +    $cacheable_metadata = new CacheableMetadata();
        if (!isset($this->taskData[$route_name])) {
   +      $cacheable_metadata = new CacheableMetadata();
   

   I don't see the point in initializing it twice.

5. +++ b/core/modules/comment/src/CommentStorage.php
   @@ -337,4 +337,17 @@ public function getUnapprovedCount() {
   +  public function getUnapprovedCids() {
   +    return $this->database->select('comment_field_data', 'c')
   +      ->fields('c', ['cid'])
   +      ->condition('status', CommentInterface::NOT_PUBLISHED, '=')
   +      ->condition('default_langcode', 1)
   +      ->execute()
   +      ->fetchCol();
   +  }
   
   +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
   @@ -62,4 +63,19 @@ public function getTitle() {
   +      foreach ($cids as $cid) {
   +        $cacheable_metadata->addCacheTags(['comment:' . $cid]);
   +      }
   

   Urgs, I think its wrong to add all the unpublished comments(given there could be 10k or more), can't we use the comment_list tag instead for the invalidation?

+++ b/core/lib/Drupal/Core/Menu/LocalTaskInterface.php
@@ -90,4 +90,11 @@ public function setActive($active = TRUE);
+  public function getCacheableMetadata();

Why this instead of making LocalTaskInterface extend CacheableDependencyInterface like #2564011: Use local task/action cacheability metadata when displaying them does? #8 also suggested this.

@lauriii asked me to take a look at this. I spent about 3 hours debugging this probably. The two remaining failures here are the combination of two bugs:

1. An oversight in this patch: just like we add the comment_list cache tag because the singular local task generated by UnapprovedComments depends on all the comments in the system …
   

   +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
   @@ -62,4 +63,12 @@ public function getTitle() {
   +    return $cacheable_metadata->setCacheTags(['comment_list']);
   

   … FieldUiLocalTask depends in a similar way on config:entity_view_display_list: it returns a set of local tasks that depends on the entity view displays enabled for a certain entity bundle.

   Fixing this fixes the problem :)

   Same goes for config:entity_form_display_list.

2. A long pre-existing bug that indicates some missing test coverage: #2384163: Entity render cache is needlessly cleared when an Entity*Fom*Display is modified added \Drupal\Core\Entity\Entity\EntityViewDisplay::postSave() but failed to call the parent method, which means the default cache tag invalidation logic for config entities (or even entities in general) no longer is invoked! Hence config:entity_view_mode_display_list is never invalidated. (Which you can easily prove by saving any entity view display, then looking at the cachetags table and note that that cache tag still does not appear!)

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -181,6 +181,7 @@ public function __construct(array $values, $entity_type) {
   +    parent::postSave($storage, $update);
   

   So this still needs test coverage. That's why I added that tag in #49.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -341,21 +359,25 @@ public function getTasksBuild($current_route_name) {
   +        $this->taskData[$route_name]['cacheable_metadata'] = $cacheable_metadata;
   

   +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
   @@ -143,4 +144,15 @@ protected function routeProvider() {
   +  public function getCacheableMetadata() {
   

   IMHO: s/cacheable_metadata/cacheability/ and getCacheability().

   Because not the metadata is cacheable, it's metadata *about* cacheability.

   Also: see #2561773: CacheableMetadata is misnamed.

3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -288,7 +305,7 @@ public function getLocalTasksForRoute($route_name) {
   +  public function getTasksBuild($current_route_name, CacheableMetadata &$cacheable_metadata) {
   

   Similarly, IMHO we shouldn't typehint to CacheableMetadata, but to RefinableCacheableDependencyInterface.

4. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
   @@ -62,4 +63,12 @@ public function getTitle() {
   +    return $cacheable_metadata->setCacheTags(['comment_list']);
   

   Nit: it's better to use the getListCacheTags() method like I did in #49.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
@@ -143,4 +144,15 @@ protected function routeProvider() {
+   */
+  public function getCacheableMetadata() {
+    $cacheability = new CacheableMetadata();
+    if (isset($this->pluginDefinition['cache_tags'])) {
+      $cacheability->addCacheTags($this->pluginDefinition['cache_tags']);
+    }
+    return $cacheability;
+  }
+

Needs also expansion of the unit test.

#51++

The two previous fails fixed, two new ones, because Dynamic Page Cache landed in the mean time:

Frontpage is cached by Dynamic Page Cache.
Full node page is cached by Dynamic Page Cache.

So… turns out this patch is causing the local task block on the front page to have the user context, which means the entire page gets the user cache context, which means Dynamic Page Cache doesn't cache it anymore.

Why is the user cache context being added here?!

I agree with xjm, I thought we were going to use CacheableDependencyInterface here.

@pwolanin, @xjm, and me already asked this before, but why do we add an new method to LocalTaskInterface instead of just reusing CacheableDependencyInterface? Also see #2564011: Use local task/action cacheability metadata when displaying them.

@pwolanin, @xjm, and me already asked this before, but why do we add an new method to LocalTaskInterface instead of just reusing CacheableDependencyInterface?

+1 We should absolutely do that. I should've remarked the same in #50.

(It is possible because we have one LocalTaskInterface object per local task — i.e. a single LocalTaskInterface implementation does NOT correspond to multiple local tasks. We only use a getCacheableMetadata() method in cases where a single object returns multiple cacheable things. So, a simple rule of thumb is that if the getCacheableMetadata() method does NOT need a parameter, then CacheableDependencyInterface should be used instead.)

I've been chatting with @lauriii while he was looking for the root cause of the remaining failure in #57. That remaining failure is simply an expectation for the Standard install profile, that the Dynamic Page Cache is able to cache node pages.

So this patch makes it so that the Dynamic Page Cache is no longer able to cache node pages.

Why? Because in HEAD, the local tasks block sets max-age=0, which means it gets placeholdered automatically. This patch removes that, because the whole point of this issue/patch is to start caching the local tasks block. But in doing so, the user cache context bubbles on node pages.

Why? Because authenticated (non-admin) users don't have the edit any article permission, only the edit own article permission. This means that for every article node, we need to check if the owner of the article matches the current user. Hence the user cache context is necessary. Hence that bubbles. And this is entirely correct.

Why is that then not being placeholdered? Because #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags hasn't been committed yet. Once that is committed, a bubbled user cache context will cause the local tasks block to be placeholdered, so that the user cache context doesn't bubble to the response level. Then the Dynamic Page Cache will once again be able to cache the response (and render the placeholder after retrieving the vast majority of the response from the cache).

Conclusion: this should just comment out the failing assertion in StandardTest and add a @todo to enable it again after #2543334 lands :)

So, again, in summary: Dynamic Page Cache expectations in Standard install profile fail, because correct cacheability metadata bubbles, which then correctly causes DPC to not cache the response. HEAD only has simple auto-placeholdering, and it is that auto-placeholdering that was allowing DPC to cache node pages in HEAD. Since we now are bubbling the *actual* cacheability metadata, and we don't have auto-placeholdering for bubbled cacheability metadata yet (but it's RTBC!), DPC is no longer caching it. Everything works as designed :)

Remaining things:

1. Comment that failing test, add a @todo.
2. Convert to CacheableDependencyInterface.

Oh, and to be clear: that makes #62 a fine way of proving that indeed the edit own permission causes the user cache context to bubble, which causes StandardTest to fail in #57. But it's absolutely a wrong change.

Here are some unit tests in the meantime. Do them first, otherwise you miss the point of them.

Conclusion: this should just comment out the failing assertion in StandardTest and add a @todo to enable it again after #2543334 lands :)

Yeah let's ensure that we don't forgot that line though in there.

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -181,6 +181,7 @@ public function __construct(array $values, $entity_type) {
        // Reset the render cache for the target entity type.
   +    parent::postSave($storage, $update);
   

   Let's swap those two lines, its easier to read that way.

2. +++ b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalActionsBlock.php
   @@ -91,35 +91,11 @@ public function build() {
   +  /**2g
   

   Is that some magic vim trick?

1. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
   @@ -62,4 +63,12 @@ public function getTitle() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheableMetadata() {
   +    $cacheable_metadata = new CacheableMetadata();
   +    return $cacheable_metadata->setCacheTags(['comment_list']);
   +  }
   

   In an ideal world we would have value objects just like for breadcrumbs, well, there is still stuff to do for Drupal 9

2. +++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
   @@ -7,6 +7,7 @@
    
   +use Drupal\Core\Cache\CacheableMetadata;
   

   This is not needed

This is starting to look close :)

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
   @@ -143,4 +145,34 @@ protected function routeProvider() {
   +    return $this->pluginDefinition['cacheability_metadata']->getCacheTags();
   

   This doesn't seem wise, because LocalTaskDefault is also what *.links.menu.yml files are mapped to, and so this is actually going to be scalar data, not objects, 99% of the time.

   This is only passing because the only plugin definitions that are setting cacheability_metadata do so from code that generates plugin definitions. Try using it from a *.links.menu.yml file, and it will fail.

   So the unit test coverage is wrong. Ideally, we'd have integration tests for this, but I think fixing the unit tests should be sufficient.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -341,21 +347,25 @@ public function getTasksBuild($current_route_name) {
   +        'cacheable_metadata' => $cacheability_metadata,
   ...
   +        $this->taskData[$route_name]['cacheable_metadata'] = $cacheability_metadata;
   
   @@ -363,12 +373,14 @@ public function getLocalTasks($route_name, $level = 0) {
   +        'cacheable_metadata' => $this->taskData[$route_name]['cacheable_metadata'],
   ...
   +      'cacheable_metadata' => $this->taskData[$route_name]['cacheable_metadata'],
   

   See #50.2.

3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManagerInterface.php
   @@ -7,6 +7,7 @@
   @@ -51,7 +52,7 @@ public function getLocalTasksForRoute($route_name);
   
   @@ -51,7 +52,7 @@ public function getLocalTasksForRoute($route_name);
       * @return array
       *   A render array as expected by menu-local-tasks.html.twig.
       */
   -  public function getTasksBuild($current_route_name);
   +  public function getTasksBuild($current_route_name, RefinableCacheableDependencyInterface &$cacheable_metadata);
   

   Docblock needs to be updated also.

4. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
   @@ -62,4 +73,11 @@ public function getTitle() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheTags() {
   +    return Cache::mergeTags(parent::getCacheTags(), $this->entityManager->getDefinition('comment')->getListCacheTags());
   +  }
   

   Instead of this, you could specify comment_list in comment.links.task.yml. Then literally nothing would have to change in this class :)

5. +++ b/core/modules/contact/contact.module
   @@ -98,7 +99,7 @@ function contact_entity_extra_field_info() {
   +function contact_menu_local_tasks_alter(&$data, $route_name, CacheableMetadata &$cacheable_metadata) {
   

   Should also be RefinableCacheableDependencyInterface.

6. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   @@ -435,4 +437,21 @@ public function testOnDependencyRemoval() {
   +  /**
   +   * Ensures that entity view mode changes invalidates cache tags.
   +   */
   +  public function testEntityDisplayInvalidateCacheTags() {
   

   Yay! :)

7. +++ b/core/modules/system/tests/modules/menu_test/menu_test.module
   @@ -29,7 +29,7 @@ function menu_test_menu_links_discovered_alter(&$links) {
   +function menu_test_menu_local_tasks_alter(&$data, $route_name, \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata) {
   

   Should also be RefinableCacheableDependencyInterface.

8. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -11,7 +11,9 @@
   +use Prophecy\Argument;
   

   I don't think this is used :)

So the unit test coverage is wrong. Ideally, we'd have integration tests for this, but I think fixing the unit tests should be sufficient.

I agree that local task default should just use with raw values, having an object in there just doesn't make sense.

I don't think this is used :)

Ha indeed it isn't, but we could in this helper method to make it a bit more clear. Currently no method is actually provided.

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -323,13 +326,16 @@ public function getTasksBuild($current_route_name) {
   +        $cacheability->addCacheableDependency($access);
   +        $cacheability->addCacheableDependency($child);
   

   Nit: can be chained.

2. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
   @@ -8,6 +8,8 @@
   +use Drupal\Core\Cache\Cache;
   +use Drupal\Core\Entity\EntityManagerInterface;
   

   Unused.

3. +++ b/core/modules/contact/contact.module
   @@ -98,7 +99,7 @@ function contact_entity_extra_field_info() {
   +function contact_menu_local_tasks_alter(&$data, $route_name, CacheableMetadata &$cacheable_metadata) {
   

   See #74.5.

4. +++ b/core/modules/system/tests/modules/menu_test/menu_test.module
   @@ -29,7 +29,7 @@ function menu_test_menu_links_discovered_alter(&$links) {
   +function menu_test_menu_local_tasks_alter(&$data, $route_name, \Drupal\Core\Cache\RefinableCacheableDependencyInterface &$cacheability) {
   

   If there was a use statement at the top, we wouldn't need to have the FQCN here.

5. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalTask/TestTasksSettingsSub1.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Cache\CacheableMetadata;
   

   Unused.

6. +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskDefaultTest.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Cache\CacheableMetadata;
   

   Unused.

7. +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.php
   @@ -393,5 +407,79 @@ protected function getLocalTasksCache() {
   +    // Setup some cacheablity metadata and ensure its merged together.
   

   Nit: s/Setup/Set up/

   s/cacheablity/cacheability/
   s/its/it is/

8. +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.php
   @@ -393,5 +407,79 @@ protected function getLocalTasksCache() {
   +    // Test the cacheable metadata of access checking.
   ...
   +    // Ensure that all cacheable metadata is merged together.
   

   s/cacheable/cacheability/

I only found nits. :)

1. +++ b/core/modules/contact/contact.module
   @@ -5,6 +5,7 @@
   +use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
   

   Unused.

2. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   @@ -435,4 +437,21 @@ public function testOnDependencyRemoval() {
   +   * Ensures that entity view mode changes invalidates cache tags.
   

   s/Ensures/Ensure/
   s/mode/display/

3. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -264,4 +265,13 @@ protected function getClassResolverStub() {
   +  protected function setupNullCachebleMetadataValidation() {
   

   s/Cacheble/Cacheability/

   Also: you're adding this to UnitTestCase, to avoid future repetition. I understand that.

   But:

   1. that feels out of scope for this issue, because if we do that, we should also update all existing occurrences of this
   2. … many of those existing occurrences do set some expectations on the cache context manager, so it wouldn't work in those cases
   3. Additions to the base class should be more polished.

   So, let's remove this, and instead move this logic into LocalTaskManagerTest::setUp() and file an separate issue for this instead.

One that's done, I think this is RTBC!

Patch looks great.

See #507488-319: Convert page elements (local tasks, actions) into blocks, this makes /nn/1 10% faster for the anonymous user.

This will probably be The Last Big Win :)

If you wonder how that's possible in combination with Dynamic Page Cache that went in last week: it's because the Local Tasks block is currently NOT being cached by Dynamic Page Cache, precisely because it is uncacheable (Dynamic Page Cache is smart like that). So we're still calculating and rendering the Local Tasks on every page load. With this patch in, that will no longer be the case.

:)

This looks great. Committed/pushed to 8.0.x, thanks!

Created #2572125: Content translation local tasks are not getting displayed due to caching as follow up.