Problem/Motivation

None of Tracker module's responses set cache tags, i.e. none of tracker.pages.inc.

Uncovered by #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

Proposed resolution

Apply the patch in #2429617-57: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), notice how the tests in TrackerTest fail. Update tracker.pages.inc to add cache tags.

Remaining tasks

Work on the proposed resolution.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#24 tracker_responses_don_t-2484611-24.patch4.33 KBborisson_
#24 interdiff.txt1.4 KBborisson_
#20 2484611-tracker_cache_tags-20-interdiff.txt5.08 KBBerdir
#20 2484611-tracker_cache_tags-20.patch4.93 KBBerdir
#20 2484611-tracker_cache_tags-20-smartcache.patch40.79 KBBerdir
#16 2484611-tracker_cache_tags-16.patch3.65 KBfgm
#13 interdiff-2484611-8-13.txt1.33 KBfgm
#13 2484611-tracker-cache-13.patch3.86 KBfgm
#8 tracker-cache-tags-2484611-08.patch3.59 KBjhedstrom
#8 tracker-cache-tags-2484611-08-TEST-ONLY.patch2.24 KBjhedstrom
#8 interdiff.txt3.13 KBjhedstrom
#4 tracker-cache-tags-2484611-04.patch913 bytesjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Tracker doesn't set cache tags » Tracker responses don't set cache tags
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +blocker
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Active » Needs review
FileSize
tracker-cache-tags-2484611-04.patch913 bytes

Here's a start, it doesn't get all of the tracker tests green with the patch from #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), but drops the fails from 7 to 5.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work

Thanks, great start! :)

+++ b/core/modules/tracker/tracker.pages.inc
@@ -120,6 +121,9 @@ function tracker_page($account = NULL) {
+      $cache_tags[] = 'node:' . $node->id();

This can be simplified to:
$cache_tags = Cache::mergeTags($cache_tags, $node->getCacheTags()).

i.e. no need to manually construct cache tags, especially not if you have the entity objects already anyway. Just call $entity->getCacheTags() :)

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented

That makes sense, I thought manually creating the tag seemed weird :)

The remaining fails appear to be related to a) new nodes, and b) comment changes. I'm not sure how to bubble up cache tags for either of those (especially since the comments aren't all loaded, just counts of new ones for these pages).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

To track new nodes being created, you'll want to associate the "list" cache tag for the Node entity type. Look at \Drupal\book\Controller\BookController::bookRender() for an example.

The comment count tracking may be a difficult bit; let's hold off on that for now.

What you can also already do in the mean time, is update the test coverage to verify that the expected cache tags exist. For WebTestBase-based tests, we have the assertCacheTag() method to make that easy.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.13 KB
tracker-cache-tags-2484611-08-TEST-ONLY.patch2.24 KB
tracker-cache-tags-2484611-08.patch3.59 KB

This adds tests, and the list cache tag. Remaining 3 fails seem related to comments.

The last submitted patch, 8: tracker-cache-tags-2484611-08-TEST-ONLY.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user

Awesome, thanks!

For the comment stuff, we have #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user. Let's get this in already; that comment stuff is a different can of worms :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Should we not also add the information coming from node access cache contexts?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Tracker responses don't set cache tags » Tracker responses don't set cache tags & contexts
Status: Reviewed & tested by the community » Needs work

Let's do that in this issue, yes.

@jhedstrom:

  $tracker_data = $query
    ->addTag('node_access')
    ->fields('t', array('nid', 'changed'))
    ->condition('t.published', 1)
    ->orderBy('t.changed', 'DESC')
    ->limit(25)
    ->execute()
    ->fetchAllAssoc('nid');

Note the ->addTag('node_access') bit — that means access checking happens. Which means the overall result must also get

$page['#cache']['contexts'][] = 'user.node_grants:view'

i.e. "the output depends on the 'view' node grants" cache context.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
2484611-tracker-cache-13.patch3.86 KB
interdiff-2484611-8-13.txt1.33 KB

Rerolled for node access context.

Status: Needs review » Needs work

The last submitted patch, 13: 2484611-tracker-cache-13.patch, failed testing.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review

Actually, context is really there when checked manually, but most of the contexts disappear in the test, possibly in relation with #2450993: Rendered Cache Metadata created during the main controller request gets lost, so rerolled without this assertion.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
2484611-tracker_cache_tags-16.patch3.65 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#15: hrm, there is no drupal_render() happening in the Tracker code, so 'user.node_grants:view' should be showing up.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented

The thing is, the header do show up when looking a the page in a browser, but during a webtest, only about half of the cache-contexts actually make it to the test browser, so this is apparently unrelated to this issue, but to something with the cache contexts handling in general.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented

The thing is, the header do show up when looking a the page in a browser, but during a webtest, only about half of the cache-contexts actually make it to the test browser

I can confirm in manual testing of #13 that the appropriate cache context does appear, but during a webtest, only 4 cache contexts are present. I tried adding both user and node modules to TrackerTest::$modules, but neither resolved the issue.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
FileSize
2484611-tracker_cache_tags-20-smartcache.patch40.79 KB
2484611-tracker_cache_tags-20.patch4.93 KB
2484611-tracker_cache_tags-20-interdiff.txt5.08 KB

Actually, the contexts work fine for me, but there's a user cache context on the user tracker (I have no idea what is adding it, though, seems like it should exist on both pages?) and the node grants cache context is part of that and merged.

Also changed the asserts to use the multiple methods from the trait. I find those much easier to work with. Note that the assert(No)CacheTag() actually doesn't have a second argument, so all those assertion messages were ignored anyway.

And added cache tags for the node owners as well, in case e.g. the user name changes.

Note that there's very limited test coverage for the per-user stuff, for example, depending on user access, the user name is linked or not. which is fine as long as it's just permission but if you have custom access checks about something else...

And... like @jhodgdon said in the big meta issue, this uses an "time ago" output. Which means that we really can't cache it if we're honest.

Issues/rabbitholes like this one are why I'm still very worried about smartcache being enabled by default. Maybe we should just mark tracker pages as non-cacheable.

Anyway, here's what I have ;)

The last submitted patch, 20: 2484611-tracker_cache_tags-20-smartcache.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Issues/rabbitholes like this one are why I'm still very worried about smartcache being enabled by default. Maybe we should just mark tracker pages as non-cacheable.

It'll force us to actually fix these issues, rather than ignore them. Being forced to think about (and fix) cacheability is a great problem to have.

  1. +++ b/core/modules/node/src/Cache/NodeAccessGrantsCacheContext.php
@@ -75,6 +75,7 @@ protected function checkNodeGrants($operation) {
+    debug($operation . '.' . implode(';', $grants_context_parts));

    We'll want to remove this :) 

  2. +++ b/core/modules/tracker/src/Tests/TrackerTest.php
@@ -130,13 +136,24 @@ function testTrackerUser() {
+    // Assert cache contexts; the node grant context is not directly visible due
+    // to the user context.

    Due to it being implied by the user context.

Once those are fixed, this is RTBC IMO.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.4 KB
tracker_responses_don_t-2484611-24.patch4.33 KB

Fixed both issues from #22.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

RTBC, Looks great!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: tracker_responses_don_t-2484611-24.patch, failed testing.

Status: Needs work » Needs review

Fabianx queued 24: tracker_responses_don_t-2484611-24.patch for re-testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Re-testing due to random test failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: tracker_responses_don_t-2484611-24.patch, failed testing.

Status: Needs work » Needs review

Berdir queued 24: tracker_responses_don_t-2484611-24.patch for re-testing.

Berdir’s picture

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

Setting back to RTBC, testbot was buggy again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
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 b497d61 and pushed to 8.0.x. Thanks!

  • alexpott committed b497d61 on 8.0.x 
    Issue #2484611 by jhedstrom, fgm, Berdir, borisson_, Wim Leers: Tracker...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Hurray, this brings #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) closer again!

Status: Fixed » Closed (fixed)

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