Local action plugins do not have any way to provide cacheability metadata

Note: We have the same issue with contextual links ...

Working on this one for a bit

One of the problems I can figure out right now is that links depend on the route cache context, but its only set on the block. There probably should be additional parameter ::getActionsForRoute that sets the cacheability metadata for the whole link set.

I'm also not sure (in terms of BC) if we should assume links that don't implement CacheableDependencyInterface should have max-age of 0 or forever. I guess 0 is the more "conservative" approach in terms of not over-caching, even though if there are any contrib modules with custom plugin classes they'd need to be updated.

What do we do in other places in core? Views for example doesn't deal with it, see \Drupal\views\Plugin\views\display\DisplayPluginBase::calculateCacheMetadata

So right now we have this in the build method of the block:

    if (empty($local_actions)) {
      return [];
    }

and that will remove the cache context away from the render array. That logic has to change so that #cache item wont get removed and then we can remove the ::getCacheContexts from the block.

After we've remove the method we should try to investigate whether there is already existing test coverage for this, or if we should add new tests.

Not working on the issue right now so unassigning

Local actions are used on admin pages, which aren't cached. I don't think this is a critical issue but rather a major one.

Local actions are used on admin pages, which aren't cached. I don't think this is a critical issue but rather a major one.

+1

Finally got a chance to review the tests and the patch. Looks good for me!

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalActionManagerTest.php
@@ -206,6 +209,11 @@ public function getActionsForRouteProvider() {
+            'max-age' => 0,

@@ -240,6 +251,11 @@ public function getActionsForRouteProvider() {
+            'max-age' => 0,

@@ -275,6 +294,11 @@ public function getActionsForRouteProvider() {
+            'max-age' => 0,

@@ -285,6 +309,11 @@ public function getActionsForRouteProvider() {
+            'max-age' => 0,

@@ -322,6 +354,11 @@ public function getActionsForRouteProvider() {
+            'max-age' => 0,

@@ -332,6 +369,11 @@ public function getActionsForRouteProvider() {
+            'max-age' => 0,

Will this change result in things being uncachable that once were?

I think this is still RTBC and ready for the commiter feedback again

+++ b/core/modules/system/src/Tests/Menu/LocalActionTest.php
@@ -48,6 +48,22 @@ public function testLocalAction() {
+    // Test a local action title that changes based on a config value.

+++ b/core/modules/system/tests/modules/menu_test/menu_test.links.action.yml
@@ -11,6 +11,14 @@ menu_test.local_action5:
+  cache_tags:
+    - config:menu_test.links.action

But this is not actually tested AFAICT, because of the default of max-age = 0.

The menu_test.local_action.cache_check local action should specify cache_max_age: 1000000 or something like that.

Or what am I missing?

Is there a way we can prevent adding more array items, and use interfaced objects for this, just like elsewhere in core? #2633878: Finalize cacheability for plugins solves a different problem by also adding cacheability metadata to plugin definitions, but in the form of CacheableDependencyInterface like we use elsewhere in core. Since we should convert plugin definitions to objects in the future anyway, this seems like a good place to start. The managers could convert YAML arrays to objects in this case.

After talking to @pwolanin on IRC I now understand the exact reason for these additional keys. It would not make sense to use a generic cache key just like in the other definition. Maybe we can use render_cache_* to emphasize those values are for render array cacheability only?

I don't understand your comment. The default is only 0 if the plugin doesn't implement CacheableDependencyInterface.

The answer to this is:

Or what am I missing?

… clearly I was missing something :) I misread. Sorry.

Since we should allow plugin definitions to expose their own cacheability metadata (in another issue), can we prefix the definition keys here so it's absolutely clear those influence render array cacheability and not something else?

@Xano - based on our discussion in IRC, possibly we should not put the cache info in the YAML at all, but rather require it to be defined in the PHP?

I don't think we should in this case, because it would undo the benefits of YAML discovery (not having to write your own class). It's more suitable when used with annotated class discovery.

I checked the code again, and see that LocalTaskDefault does the same thing already. We should probably use the same approach for LocalActionDefault and not prefix the definition keys for consistency with existing code.

I'm committing this to 8.1.x first. I'm not sure this should go into 8.0.x as it is adding an interface and public methods to a class. Committed 1a71d3b and pushed to 8.1.x. Thanks!

diff --git a/core/lib/Drupal/Core/Menu/Plugin/Block/LocalActionsBlock.php b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalActionsBlock.php
index 98427aa..eda4a87 100644
--- a/core/lib/Drupal/Core/Menu/Plugin/Block/LocalActionsBlock.php
+++ b/core/lib/Drupal/Core/Menu/Plugin/Block/LocalActionsBlock.php
@@ -8,7 +8,6 @@
 namespace Drupal\Core\Menu\Plugin\Block;
 
 use Drupal\Core\Block\BlockBase;
-use Drupal\Core\Cache\Cache;
 use Drupal\Core\Menu\LocalActionManagerInterface;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
diff --git a/core/modules/system/src/Tests/Menu/LocalActionTest.php b/core/modules/system/src/Tests/Menu/LocalActionTest.php
index beb8870..c876a25 100644
--- a/core/modules/system/src/Tests/Menu/LocalActionTest.php
+++ b/core/modules/system/src/Tests/Menu/LocalActionTest.php
@@ -51,7 +51,7 @@ public function testLocalAction() {
     // Test a local action title that changes based on a config value.
     $this->drupalGet(Url::fromRoute('menu_test.local_action6'));
     $this->assertLocalAction([
-      [Url::fromRoute('menu_test.local_action5'), 'Original title']
+      [Url::fromRoute('menu_test.local_action5'), 'Original title'],
     ]);
     // Verify the expected cache tag in the response headers.
     $header_values = explode(' ', $this->drupalGetHeader('x-drupal-cache-tags'));
@@ -62,7 +62,7 @@ public function testLocalAction() {
     $config->save();
     $this->drupalGet(Url::fromRoute('menu_test.local_action6'));
     $this->assertLocalAction([
-      [Url::fromRoute('menu_test.local_action5'), 'New title']
+      [Url::fromRoute('menu_test.local_action5'), 'New title'],
     ]);
   }

Fixed up a few coding standards on commit.

I think the disruption here is super minimal: a default plugin manager implementation now implements one additional interface. No interfaces are changed. So I don't see why this couldn't go into 8.0. Even existing subclasses of this plugin manager would continue to work just fine.

This is a straight bugfix, with no BC impact, so can be safely committed to 8.0.x.

Adjusting status for #40.

Could we provide a class in core which allows you to define cache metadata as part of the yml file, which itself extends from the default class

@dawehner - well, that's basically what's in 8.1.x? Or you are thinking as a shim for 8.0.x?

Well yeah I was thinking about making it at least easy by providing that class, so people can specify it in the yml file.

I think it would be nice because it also tells people that they should be thinking about it.

Yeah potentially this is not that much needed in reality.

Thanks @pwolanin for working on a backportable version for 8.0.x. Committed 99158a9 and pushed to 8.0.x. Thanks!