See #1722882-38: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items.

The cache decorator definition needs to contain a cache tag, then it should work combined with the change in the issue above. Should be a one line change.

This will require test coverage:
- Enable two languages (see earlier patch in the other issue)
- Create an aggregator feed using one language and check the blocks
- Switch language and check blocks again.

CommentFileSizeAuthor
#12 drupal8.block-language-cache-test.12.patch1.34 KBsun
#8 aggregator-1893818-8.patch713 bytestim.plunkett
#4 block-language-cache-tag-1893818-4.patch3.77 KBBerdir
#4 block-language-cache-tag-1893818-4-interdiff.txt1.2 KBBerdir
#3 block-language-cache-tag-1893818-3.patch3.93 KBBerdir
#3 block-language-cache-tag-1893818-3-interdiff.txt1.4 KBBerdir
#2 block-language-cache-tag-1893818-2.patch3.94 KBBerdir
#2 block-language-cache-tag-1893818-2-test-only.patch2.69 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Blocks-Layouts, +Plugin system, +Block plugins
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
block-language-cache-tag-1893818-2-test-only.patch2.69 KB
block-language-cache-tag-1893818-2.patch3.94 KB

Ok, here we go. Test coverage and the fix, which is trivial now that the cache decorator is fixed.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
block-language-cache-tag-1893818-3-interdiff.txt1.4 KB
block-language-cache-tag-1893818-3.patch3.93 KB

Documentation fixes for the test. the wording there can probably be improved a bit :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
block-language-cache-tag-1893818-4-interdiff.txt1.2 KB
block-language-cache-tag-1893818-4.patch3.77 KB

And removed that commented out code, that wasn't actually necessary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: -Needs tests

Has tests :)

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Thnx!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Wow! Great work on the test lines of code: fix line of code ratio! :D

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Fixed » Reviewed & tested by the community
FileSize
aggregator-1893818-8.patch713 bytes

This wasn't rerolled/retested after #293318: Convert Aggregator feeds into entities :(

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

bzzzt.

Committed and pushed to 8.x. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Why did we add this test to Aggregator module?

The code, logic, and fix belongs to Block module's plugin manager, so essential test coverage for Block module's own functionality lives in Aggregator module now.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Well, we can move it. Aggregator was one of the obvious real use cases, so that's what I picked it for writing the test.

If #1888702: Use configuration selection instead of derivatives for some blocks happens, then we kinda need to move it, although I'm not sure if any real derivate examples will remain, is there a test derivate block plugin implementation that we could use?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Needs review
FileSize
drupal8.block-language-cache-test.12.patch1.34 KB
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Yes, let's do this :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Opened #1907434: Add a helper for clearing of multiple (but finite) variations of cache IDs - cache tags might not be the best choice for stuff that only varies by language.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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