Content translation local tasks are not getting displayed due to caching

I really think we should just clear the render cache when a new module is installed and remove block_install().

Clearing at module install sounds logical to me as well.

You probably want the same logic and clear the tag.

I'm guessing what you're running into in that test fail is smart cache which afaik uses a separate bin.

I don't understand why we are deleting that hook_install.

Here is a patch with the same logic but for the form we are interested.

Is this still repeatable?

Yes, as of yesterday HEAD I was able to reproduce the bug.

I will try to write the test at the drupalcon sprint.

Here is the patch with the failing test.

The test only patch had a wrong file numbering.

Here is the complete patch.

The latest patch has lost the hunk that removes block.install. See earlier patche.s

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -41,7 +52,8 @@ public function __construct(EntityManagerInterface $entity_manager) {
+      $container->get('cache.render')

@@ -156,6 +168,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    // Invalidate the render cache because we need to show new actions in blocks
+    // with tabs. eg. "Translate".
+    $this->renderCache->deleteAll();

This is wrong. This only invalidates the default render cache bin. Any cache bin can be used for render caching.

You must instead invalidate the 'rendered' cache tag.

I understood that the hook_install was out of scope for this issue. I don't know how it fits the proposed resolution because the cache stall doesn't happen on any module install but when changing translation settings.

Here is the patch that invalidates the tag.

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -170,7 +159,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    Cache::invalidateTags(['rendered']);

Let's inject the cache_tags.invalidator service instead.

Thank you for the reviews :)

I thought it didn't mattered but I guess I have to learn about the advantages of injecting.

New patch attached

small mistake

1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationOperationsTest.php
   @@ -103,9 +106,38 @@ function testOperationTranslateLink() {
   +    // Ensure that when I disable translation for a content type I don't
   +    // see a translate link anymore in the actions block.
   

   We don't use "I" in comments. Prefer to use "we", or even better, neither.

   e.g. Ensure the 'Translate' local task does not show up anymore when disabling translations for a content type.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationOperationsTest.php
   @@ -103,9 +106,38 @@ function testOperationTranslateLink() {
   +    // Warm cache.
   +    $this->drupalGet('node/' . $node->id());
   +    // Check that we can see the translate link.
   +    $this->assertLinkByHref('node/' . $node->id() . '/translations');
   +    // Make article not translatable.
   +    $this->drupalPostForm('admin/config/regional/content-language', ['settings[node][article][translatable]' => FALSE], t('Save configuration'));
   +    // Go back to the node.
   +    $this->drupalGet('node/' . $node->id());
   +    // Check that there is no link to translate anymore.
   +    $this->assertNoLinkByHref('node/' . $node->id() . '/translations');
   

   One comment per statement is excessive. Please group them logically.

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationOperationsTest.php
   @@ -103,9 +106,38 @@ function testOperationTranslateLink() {
   +   * Test the access to the overview page for translations.
   

   Method names must use 3rd singular person, so "Tests", not "Test".

4. +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
   @@ -156,6 +168,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // Invalidate the rendered tag cache because we need to show new actions in
   +    // blocks with tabs. eg. "Translate".
   

   s/rendered/'rendered'/

   "new actions in blocks with tabs" does not make sense.

   "eg." is not a valid abbreviation.

Added corrections.

looks possibly related to: #2624594: Local action plugins do not have any way to provide cacheability metadata

Clearing the whole render cache here seems rather like overkill? we don't have a cache tag that generically covers all local tasks?

Looks like the local task plugin should have a custom class and expose a cache tag according to what entity and bundle it relates to?

Unfortunately, the local task plugin is associated with a route that applies to the canonical entity view, but the config cache tags are like:
language.content_settings.node.page

so specific to a bundle (e.g. node type). So the local tasks should emit cache tags for all bundles of the entity type, which means they also probably need to include the entity_bundles cache tag? Or does adding or removing a bundle do a rather blanket wipe on caches?

Or does adding or removing a bundle do a rather blanket wipe on caches?

I think adding or removing a bundle wipes caches. I know it happens for say changing the manage display or form display. Those aren't daily actions, so I think wiping the render cache here is fine since you won't be changing those content language settings often.

Re #29: Do you want a dedicated cache tag because you think clearing the rendered clears too much that wouldn't actually have to be cleared? If that's the case I disagree because the language configuration is mostly a one-time operation for a site so I don't think we should optimize for that.

In general we should never invalidate caches in a form submission, because that means importing config or drush config-set doesn't get the same treatment. Instead we should override EntityInterface::getCacheTagsToInvalidate() in ContentLanguageSettings to return the rendered cache tag. The DateFormat config entity does this, which similarly is one of those things you set up once and then mostly never touch, so I think that's a pattern we can re-use. Looking at DateFormat::getCacheTagsToInvalidate() I see that it actually just returns ['rendered'] without delegating to the parent method for the generic config:date_format... cache tag. I'm not sure if that is intentional or an oversight (perhaps even a bug?). Looking further DateFormat also has list_cache_tags = { "rendered" } in its annotation. Not sure whether we need that for ContentLanguageSettings as well. But in general I think that would be a better approach.

That's by design for date formats, we want to avoid adding lots of date format cache tags when we don't need them for invalidation.

Ahh thanks, that makes sense. So I guess since for content language settings that's not the case (since they generally will not be part of any output, at least I guess so) it would be best practice to call the parent in cacheTagsToInvalidate()?

I think so, yes.

What's a bit different for them is that we usually save a lot of them on the overview page, possibly dozens. So we'd invalidate the rendered cache tag dozens of times. But we have optimizations in place that will ignore any additional invalidation of the same cache tag in a single request, so that should be OK.

Ahh, I had looked at Entity::invalidateTagsOnSave() and had missed the override of ConfigEntityBase::invalidateTagsOnSave(). It does seem a little suspect that getCacheTagsToInvalidate() is ignored for config entities, but that's not introduced here, so I think this is good to go. Thanks!

Related issue with config translation, also probably misses a cache tag somewhere.

Committed/pushed to all three 8.x branches, thanks!

Thanks, this was a tough one for users, good to see it fixed!