Problem/Motivation

#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) uncovered a cache tag we forgot for Tracker module's responses:

  1. the tracker shows the comment counts for all listed nodes, which means we also need the Comment entity's list cache tag
  2. history (new/updated markers) is only supported for authenticated users, so Tracker only adds the JS library for that for authenticated users. This is good. But it was not yet setting the corresponding cache context.

Proposed resolution

Fix it.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#15 interdiff.txt1.05 KBeffulgentsia
#15 tracker_cacheability_fixes-2554585-15.patch2.12 KBeffulgentsia
#13 tracker_cacheability_fixes-2554585-13.patch2.1 KBwim leers
#11 tracker_cacheability_fixes-2554585-11.patch3.35 KBwim leers
#3 tracker_cacheability_fixes-2554585-3.patch3.16 KBwim leers
#3 tracker_cacheability_fixes-2554585-3-test-only-FAIL.patch1.97 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Forum index response is missing the vocabulary cache tag » Tracker responses are missing a cache tag and a cache context
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new tracker_cacheability_fixes-2554585-3-test-only-FAIL.patch1.97 KB
new tracker_cacheability_fixes-2554585-3.patch3.16 KB
moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Trivial fixes. Thanks for the test coverage. One bot came back green so setting to RTBC early.

The last submitted patch, 3: tracker_cacheability_fixes-2554585-3-test-only-FAIL.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: tracker_cacheability_fixes-2554585-3.patch, failed testing.

Status: Needs work » Needs review

Wim Leers queued 3: tracker_cacheability_fixes-2554585-3.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Random testbot fail it seems. DrupalCI *does* mark it as green.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Re-RTBC'ing per #4 — only was un-RTBC'd because of a testbot fluke.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: tracker_cacheability_fixes-2554585-3.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new tracker_cacheability_fixes-2554585-11.patch3.35 KB

Rebased, one small conflict had to be solved.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Postponed

Let's wait for #2530846: Fix and improve comment cache tag usage to land first, I think that may remove the need for the cache tag changes here.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Needs review
StatusFileSize
new tracker_cacheability_fixes-2554585-13.patch2.1 KB

#2530846: Fix and improve comment cache tag usage improved HEAD: it ensures that comment created/edited/deleted on entities also invalidates the cache tags of those entities. Which means this patch no longer needs to add the comment_list cache tag :)

(No interdiff because there was a rebase conflict too and it's only a tiny patch anyway. Basically, the patch is identical to the previous patch, with just the cache tag changes removed.)

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Trivial fix. Please wait for green before merge,

effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new tracker_cacheability_fixes-2554585-15.patch2.12 KB
new interdiff.txt1.05 KB

I think this is clearer. What do you think?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Clearer indeed! :)

effulgentsia’s picture

effulgentsia commented

Adding credit to Moshe for reviewing.

effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x. Thanks!

  • effulgentsia committed f0ba2d7 on 8.0.x 
    Issue #2554585 by Wim Leers, effulgentsia, moshe weitzman: Tracker...

Status: Fixed » Closed (fixed)

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