Once #1605290: Enable entity render caching with cache tag support lands we'll have cache tags applied for all entities on a page.

This leaves two places that do render caching that don't support them yet:

#1712456: How to leverage cache tags in Views for Views.

And this issue for the page cache. I couldn't find an issue for this, but there might be an old one somewhere.

So for the page cache, we need to collect all the cache tags added throughout the request and store them when we save it, all the invalidation will be handled by the existing validation.

Files: 
CommentFileSizeAuthor
#19 drupal8.base-system.2094241-19.patch6.19 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 59,864 pass(es).
[ View ]
#19 interdiff.diff1.72 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_11.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 page-cache.diff6.53 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 59,782 pass(es).
[ View ]
#15 page-cache.diff6.53 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 59,438 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 drupal8.base-system.2094241-13.patch6.55 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2094241-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 drupal8.base-system.2094241-8.patch2.82 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,154 pass(es).
[ View ]
#9 interdiff.txt1.22 KBamateescu
#7 drupal8.base-system.2094241-7.patch3.06 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,583 pass(es).
[ View ]
#7 interdiff.txt1.48 KBWim Leers
#4 drupal8.base-system.2094241-4.patch2.9 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 59,525 pass(es).
[ View ]
#3 drupal8.base-system.2094241-3.patch3.54 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 59,531 pass(es).
[ View ]

Comments

catch’s picture

Component:routing system» base system
Wim Leers’s picture

Yes yes yes yes yes +1000.

moshe weitzman’s picture

Assigned:Unassigned» moshe weitzman
Status:Active» Needs review
StatusFileSize
new3.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,531 pass(es).
[ View ]

This turned out to be pretty straightforward. We only have access to the processed page array during drupal_render_page() so we have to calculate and stash away our list of tags at that time. I chose to add a #post_render callback at the top level of the $page array and do collection and storage of tags there. We then fetch those collected tags later during drupal_page_set_cache().

We were not correctly tagging nodes with their author's uid so I fixed that line in this patch. It was giving an error.

moshe weitzman’s picture

StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,525 pass(es).
[ View ]

Better code comments.

Status:Needs review» Needs work
Issue tags:-Performance, -D8 cacheability

The last submitted patch, drupal8.base-system.2094241-4.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
Issue tags:+Performance, +D8 cacheability

#4: drupal8.base-system.2094241-4.patch queued for re-testing.

Wim Leers’s picture

Issue tags:+Needs tests
StatusFileSize
new1.48 KB
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,583 pass(es).
[ View ]

Fixing nitpicks.

amateescu’s picture

  1. +++ b/core/includes/common.inc
    @@ -4455,6 +4455,38 @@ function drupal_render_collect_cache_tags($element, $tags = array()) {
    +    $tags = &drupal_static('system_cache_tags_page', FALSE);

    Shouldn't we default to an array here instead of false? Just like in drupal_cache_tags_page_get().

  2. +++ b/core/modules/node/lib/Drupal/node/NodeRenderController.php
    @@ -88,7 +88,7 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityDisp
    +    $build['#cache']['tags']['user'][] = $entity->uid->value;

    Since $entity is actually a class that implements NodeInterface, we should use $entity->getAuthorId().

amateescu’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.22 KB
new2.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,154 pass(es).
[ View ]

My review points are trivial, I think this is ready to go.

Wim Leers’s picture

Status:Reviewed & tested by the community» Needs review

Sorry, to un-RTBC, but this still needs tests.

amateescu’s picture

My view on testing was that we don't need them at the moment because there's nothing to break here, we either get some cache tags from a render array or we don't.

Now, if we want to go further and remove the 'content' cache tag, that will definitely need testing.

catch’s picture

It would be lovely to remove the 'content' cache tag, not sure if that should happen in this issue. And we'd need to ensure that cache tags for all entities rendered get correctly added to the page cache and that it gets cleared by two or more in a functional test I think, but yeah that somewhat depends on removing the content tag if it'll be at all realistic.

moshe weitzman’s picture

StatusFileSize
new6.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2094241-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks for the feedback and rerolls. Here is a new patch with a test.

Status:Needs review» Needs work

The last submitted patch, drupal8.base-system.2094241-13.patch, failed testing.

moshe weitzman’s picture

Status:Needs work» Needs review
StatusFileSize
new6.53 KB
FAILED: [[SimpleTest]]: [MySQL] 59,438 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reroll

moshe weitzman’s picture

StatusFileSize
new6.53 KB
PASSED: [[SimpleTest]]: [MySQL] 59,782 pass(es).
[ View ]

Actually this one

moshe weitzman’s picture

We are green now, with a proper web test. Anyone care to review and RTBC?

amateescu’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
    @@ -42,6 +42,29 @@ function setUp() {
    +   * Assure that cache tags are properly persisted.

    Really minor point :) "Tests that cache tags ..."

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
    @@ -42,6 +42,29 @@ function setUp() {
    +    cache_invalidate_tags($tags);

    This procedural wrapper is marked as deprecated, we should use Cache::invalidateTags() directly instead.

  3. +++ b/core/modules/system/tests/modules/test_page_test/test_page_test.module
    @@ -22,5 +22,6 @@ function test_page_test_page() {
    +    '#cache' => array('tags' => array('test-page' => TRUE)),

    This new tag doesn't seem to be used or invalidated anywhere, shouldn't we remove it?

moshe weitzman’s picture

StatusFileSize
new1.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_11.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
new6.19 KB
PASSED: [[SimpleTest]]: [MySQL] 59,864 pass(es).
[ View ]

Good catches. Fixed all those.

Status:Needs review» Needs work

The last submitted patch, interdiff.diff, failed testing.

amateescu’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs tests

Looks good now :)

Wim Leers’s picture

Yep, looks good :)

catch’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

I opened #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly for content cache tag removal.

Status:Fixed» Closed (fixed)

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