Problem/Motivation

Drupal caches blocks explicitly by language. BlockViewBuilder has these default cache keys:

        $default_cache_keys = array(
          'entity_view',
          'block',
          $entity->id(),
          $this->languageManager->getCurrentLanguage()->getId(),
          // Blocks are always rendered in a "per theme" cache context.
          'cache_context.theme',
        );

However it is still possible to configure the blocks to NOT vary per the language cache context, which is not actually adhered to (because its hardwired above).

Proposed resolution

Make the UI honest about the use of language in cache being always on for blocks. Fix the code to refer to the cache context instead of a specific language code.

Remaining tasks

Commit.

User interface changes

None.

API changes

None.

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
646 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,564 pass(es). View

No test coverage yet, but this should fix the problem: we make sure that this block is always cached per language.

Gábor Hojtsy’s picture

FileSize
1.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es). View

Thats not the right approach, the content block is already render-cached per language, we need the main block to be tagged for its language, and then we need to clear caches with that tag. This patch will clear the cache for tags for that language when submitting config translation.

Wim Leers’s picture

FileSize
2.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,569 pass(es), 16 fail(s), and 0 exception(s). View

Turns out that using the language entity is not an option, because it's only available when the language module is enabled. Which would mean we can only add cache tags when the language module is enabled, which is less than great.

So I added a getCacheTag() method to LanguageInterface.

Perhaps this issue should be broadened in scope; it could become the "cache tags for languages" issue.

Status: Needs review » Needs work

The last submitted patch, 3: block_content_vary_by_language-2318437-3.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,623 pass(es), 2 fail(s), and 0 exception(s). View
4.74 KB

Now that cache tag needs to be expected at several tests. These were in the fails.

Gábor Hojtsy’s picture

BTW I think it would be good to start to practically introduce language cache tags here, and then spread them instead of starting even one more level up...

Status: Needs review » Needs work

The last submitted patch, 5: block_content_vary_by_language-2318437-5.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
1.7 KB

Fix one missing cache tag and one misplaced. This will now pass I think. Still needs its own test for translating the block itself.

dawehner’s picture

I wonder whether we could just add language somehow

+++ b/core/modules/block/src/BlockBase.php
@@ -447,7 +447,10 @@ public function getCacheTags() {
+    return array_merge(
+      array('block_plugin' => array($block_plugin_cache_tag)),
+      \Drupal::languageManager()->getCurrentLanguage()->getCacheTag()
+    );

You could argue that we could put this even higher in the chain. Could the cache.render cache service always include it?

Gábor Hojtsy’s picture

FileSize
7.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,578 pass(es). View

Reuploading patch because it was not sent for testing. It was removed as part of @dawehner's comment for some odd reason.

Wim Leers’s picture

Title: Block cache not cleared when editing block configuration translation » Introduce language cache tag and use it where necessary (was: "Block cache not cleared when editing block configuration translation")
Issue tags: +D8 cacheability, +Performance
FileSize
11.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,558 pass(es), 39 fail(s), and 27 exception(s). View
4.39 KB

#9: I've thought about putting it higher in the chain too. It couldn't live in the cache.render cache back-end though, but it could live in cache_render_cache_set().
But that would make it almost impossible to choose not to render cache per-language. It's really the responsibility of each renderable thing to declare by what it needs to be invalidated, by setting the proper cache tags.


BTW I think it would be good to start to practically introduce language cache tags here, and then spread them instead of starting even one more level up...

Agreed, so I'm starting to do that now.

I updated the patch to vary each block per language (using the "language" cache context), because if we're going to tag every block with the current language's cache tag, we should also vary each block per language. This was already done "kind of": each block got the LanguageManager->getCurrentLanguage()->getId() key, but we might just as well use the cache context, because otherwise we already cache blocks per UI language, but still allow users to configure blocks to be cached per language. That's rather confusing.
This caused one subtle change: blocks no longer get the en part of their cache ID while there is only a single language on the website, they'll get the empty string instead. I think it's clearer for everybody if we always include the language explicitly, so I'm updating LanguageCacheContext slightly.

We'll also want to bring the language cache tag to entities (nodes etc.), because they may be showing field labels. And by default there's the Submitted by %author% on %locale-formatted-date% text, which also depends on the UI language. We may also want to introduce cache_context.language there, but EntityViewBuilder::getBuildDefaults() currently has

      if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
        $build['#cache']['keys'][] = $langcode;
      }

So I'd love input from e.g. Gábor on how to correctly handle all that.

We generally should be careful about UI language, content language, and available translations of entities when doing adding cache tags and varying by language.

Gábor Hojtsy’s picture

Once again the smaller scope we can start with the better chance this has to get in without lingering for months on and on. As for content cache tags, the content language used for rendering the content may be whatever (eg. in views, you may filter to German all the time even for English UI pages). I'm not sure we are doing a fabulous job yet to somehow coordinate the field labels for content with the content language, but we should. So if you display a German entity on an English UI page, the field labels *may* show in English (did not test but I assume so :/). Yet one more reason we don't want to delve too deep into this at first.

Status: Needs review » Needs work

The last submitted patch, 11: block_content_vary_by_language-2318437-11.patch, failed testing.

Gábor Hojtsy’s picture

I think the current title Introduce language cache tag and use it where necessary may be valid for a META issue but not for an issue where you actually want to get something done. Where language is used and which languages exactly is probably an epic question. I'd rather see we start introducing language cache tags on blocks and then going forward on other elements and/or refine our understanding of what a block's language is.

Gábor Hojtsy’s picture

Title: Introduce language cache tag and use it where necessary (was: "Block cache not cleared when editing block configuration translation") » Language is not considered in block caching

Retitling for a scope that would not be a meta :) Agreed?

Gábor Hojtsy’s picture

Clearly not being worked on :/

Gábor Hojtsy’s picture

jhedstrom’s picture

Issue tags: +Needs reroll
Wim Leers’s picture

Title: Language is not considered in block caching » Enable the 'language' cache context by default for blocks
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,203 pass(es), 1 fail(s), and 0 exception(s). View

The STR in the IS are fixed with this patch. Still needed: a test that follows the STR.

(Patch now simplified because of #12/#14. No interdiff because the patch is now tiny, thanks to its focus. A new issue should be filed to deal with the cache tags aspect that was handled in #11.)

Wim Leers’s picture

FileSize
5.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,208 pass(es), 1 fail(s), and 0 exception(s). View
1.01 KB

Forgot to include one hunk in #19.

The last submitted patch, 19: block_content_vary_by_language-2318437-19.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: block_content_vary_by_language-2318437-20.patch, failed testing.

Gábor Hojtsy’s picture

Fix looks good. Not sure how to best test this, but a test for the STR may be good.

Gábor Hojtsy’s picture

Issue tags: +sprint
Wim Leers’s picture

Issue tags: +Novice, +php-novice

Let's see if somebody wants to write this test :)

geertvd’s picture

Assigned: Unassigned » geertvd

I'll write a test for this.

Wim Leers’s picture

Wonderful, thank you!

geertvd’s picture

I'm actually not able to reproduce this following the STR in the IS.
Not sure if I'm overlooking something, can someone confirm that the STR in the IS are still relevant?

rodrigoaguilera’s picture

What is STR and IS?

geertvd’s picture

@rodrigoaguilera
STR: Steps to reproduce
IS: Issue summary

Wim Leers’s picture

Assigned: geertvd » Gábor Hojtsy
Issue summary: View changes

Confirmed, I also can't reproduce this anymore.

It feels like something is missing from the STR. Hopefully Gábor can enlighten us :)

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

Ok I cannot reproduce this anymore either. Neither by visiting the page first. I wonder why is this fixed now? I mean the language context is *clearly* not selected on the block for caching.

Wim Leers’s picture

But blocks have always been cached by language already, just in a hardcoded, less-than-great way:

+++ b/core/modules/block/src/BlockViewBuilder.php
@@ -84,7 +84,8 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
-          $this->languageManager->getCurrentLanguage()->getId(),
+          // Blocks are always rendered in a "per language" cache context.
+          'cache_context.language',

I think this is what happened originally:

  1. Custom block "Hello" created, in English.
  2. All the translation stuff is enabled. Let's say we added French as a site language.
  3. Visit a page with the "Hello" block, in French. But there's no translation. So we see "Hello". So far so good.
  4. Only *now* we translate the block to "Bonjour". When going to the same page as the previous step, we still see "Hello".

What has changed since the above, is simple: config cache tags. We now consistently invalidate cache tags for config: #2040135: Caches dependent on simple config are only invalidated on form submissions — even when not using the config entity, but config itself directly. AFAICT, config_translation uses config "directly" (not config entities). So those CRUD operations now trigger the cache tag invalidations for the placed block config entity, whose cache tags live on the page, and voila: it all works!

If Gábor can confirm the above explanation, I think we should repurpose this issue to not be a bugfix, but a task, to still do what #20 does, because it's still a valid cleanup.

geertvd’s picture

I also think #20 is still a valid change, I can still write a test to confirm that the cache tag is actually invalidated on form submission what will still happen when I apply this issue if I'm not wrong.

Wim Leers’s picture

Title: Enable the 'language' cache context by default for blocks » Replace the hardcoded langcode key on blocks with the 'language' cache context
Issue tags: -Needs tests

I don't think a test is necessary. This is now basically a code clean-up. Retitled.

Gábor Hojtsy’s picture

I agree, this looks like a cleanup and also makes our UI more honest about its caching per language. Still needs the existing test fails resolved though :/ Only the expected form elements were present. in the BlockInterfaceTest.

geertvd’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
5.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,882 pass(es). View

This fixes that.

Status: Needs review » Needs work

The last submitted patch, 37: block_content_vary_by_language-2318437-37.patch, failed testing.

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Looks good, thanks! :) Great cleanup. Let's not call it a novice though.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0d869b1 and pushed to 8.0.x. Thanks!

  • alexpott committed 27c9167 on 8.0.x
    Issue #2318437 by Wim Leers, Gábor Hojtsy, geertvd: Replace the...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks all!

Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.