Updated: Comment #0

Problem/Motivation

If we want to improve the page cache by removing the "content" cache tag (see #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly), then we must ensure that cache tags exist for all user-modifiable pieces of content on the page.

Placing a new block does not cause the affected page cache entries to be invalidated. In #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags, this is being fixed by invalidating the "content" cache tag, since that's the only feasible solution for now. But as explained above, we want to get rid of that tag!

Steps to reproduce

  1. Install Drupal 8 in the Standard profile.
  2. Enable page caching (go to admin/config/development/performance, check the "Use internal page cache" setting and set the "Page cache maximum age" setting to e.g. 3 minutes).
  3. Visit the content as the anonymous user (e.g. the front page)
  4. Place another block (e.g. a second instance of the "Powered by Drupal" block).
  5. Visit the content as the anonymous user (e.g. the front page)
    • Expected: the placed block shows up.
    • Actual: the placed block doesn't show up, because the stale page cache entry is being served.

Proposed resolution

Set a theme:<theme name> cache tag on each page cache entry.

Remaining tasks

Update drupal_page_set_cache() (or maybe system_page_build()) to set the cache tag, and Block::postSave() in the case of a new block.

The latter is blocked on #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#17 interdiff.txt2.68 KBwim leers
#17 theme_cache_tag-2185617-17.patch18.86 KBwim leers
#15 theme_cache_tag-2185617-15.patch16.24 KBwim leers
#15 interdiff.txt7.36 KBwim leers
#13 interdiff.txt11.86 KBwim leers
#13 theme_cache_tag-2185617-13.patch10.44 KBwim leers
#12 2185617-12.patch8.01 KBdrifter
#10 add-theme-cache-tag-to-pages-2185617-10.patch3.04 KBvoidberg
#7 add-theme-cache-tag-to-pages-2185617-7.patch2.14 KBvoidberg
#4 add-theme-cache-tag-to-pages-2185617-4.patch1.87 KBvoidberg

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Major » Critical

Marking critical since the parent issue is critical and this blocks the parent.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Active

#2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags is in, which adds base test classes that make it easier to test this :)

voidberg’s picture

voidberg commented
StatusFileSize
new add-theme-cache-tag-to-pages-2185617-4.patch1.87 KB

Attaching patch. Did not change the test file which means the current tests will fail.

voidberg’s picture

voidberg commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: add-theme-cache-tag-to-pages-2185617-4.patch, failed testing.

voidberg’s picture

voidberg commented
StatusFileSize
new add-theme-cache-tag-to-pages-2185617-7.patch2.14 KB

Reroll patch.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: add-theme-cache-tag-to-pages-2185617-7.patch, failed testing.

voidberg’s picture

voidberg commented
Status: Needs work » Needs review
StatusFileSize
new add-theme-cache-tag-to-pages-2185617-10.patch3.04 KB

Added the theme tag to the tests as well.

Status: Needs review » Needs work

The last submitted patch, 10: add-theme-cache-tag-to-pages-2185617-10.patch, failed testing.

drifter’s picture

drifter commented
Status: Needs work » Needs review
StatusFileSize
new 2185617-12.patch8.01 KB

Rerolling and updating the tests to make them pass. I must admit I'm not very familiar with what goes on behind the scenes, my test fixes will need reviewing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new theme_cache_tag-2185617-13.patch10.44 KB
new interdiff.txt11.86 KB

Great work, voidberg & drifter! :)

The place the cache tags were being added was less than ideal, so I moved it to the ideal place: #type = page's #pre_render callback. Also, we must not just yet delete the content:1 cache tag, because too many things still depend on it.

What was still missing, is a cache tag that corresponds to the global theme settings — I noticed this while working on making SystemBrandingBlock cacheable, which will come with the next reroll.

Status: Needs review » Needs work

The last submitted patch, 13: theme_cache_tag-2185617-13.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt7.36 KB
new theme_cache_tag-2185617-15.patch16.24 KB

Here's a reroll with a cacheable SystemBrandingBlock! Tests fixed also.

(Also fixed the link to the appearance settings. And fixed a typo in LanguageCacheContext.)

Status: Needs review » Needs work

The last submitted patch, 15: theme_cache_tag-2185617-15.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new theme_cache_tag-2185617-17.patch18.86 KB
new interdiff.txt2.68 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

We have tests and patch is green. I am unable to find anything questionable in the patch so changing status to RTBC. Thank you @Wim Leers.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 407f1d1 and pushed to 8.x. Thanks!

diff --git a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
index ef7ba68..e9555b7 100644
--- a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
@@ -219,7 +219,7 @@ public function getCacheTags() {
     return NestedArray::mergeDeep(parent::getCacheTags(), $tags);
   }
 
-    /**
+  /**
    * {@inheritdoc}
    */
   protected function getRequiredCacheContexts() {

Fixed during commit.

  • Commit 407f1d1 on 8.x by alexpott: 
    Issue #2185617 by Wim Leers, voidberg, drifter: Each page cache entry...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

#19: darn, sorry about that :(

Status: Fixed » Closed (fixed)

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