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.

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

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

Status: Active » Needs review
StatusFileSize
new1.97 KB
new3.16 KB
moshe weitzman’s picture

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’s picture

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

wim leers’s picture

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

Status: Needs work » Reviewed & tested by the community
StatusFileSize
new3.35 KB

Rebased, one small conflict had to be solved.

wim leers’s picture

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

Status: Postponed » Needs review
StatusFileSize
new2.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

Status: Needs review » Reviewed & tested by the community

Trivial fix. Please wait for green before merge,

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new2.12 KB
new1.05 KB

I think this is clearer. What do you think?

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

Clearer indeed! :)

effulgentsia’s picture

Adding credit to Moshe for reviewing.

effulgentsia’s picture

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.