Problem/Motivation
None of Forum module's responses set cache tags, i.e. none of ForumController
.
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 ForumIndexTest
fail. Update ForumController
, and perhaps also ForumManager
to add cache tags. Note that also list cache tags are missing. Look at \Drupal\book\Controller\BookController::bookRender()
for an example.
Remaining tasks
Work on the proposed resolution.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#37 | interdiff.txt | 1.66 KB | lauriii |
#37 | forum_responses_don_t-2484619-37.patch | 7.48 KB | lauriii |
#34 | interdiff.txt | 1.26 KB | Wim Leers |
#34 | forum_responses_don_t-2484619-34.patch | 6.44 KB | Wim Leers |
#30 | forum_responses_don_t-2484619-30.patch | 6.15 KB | borisson_ |
Comments
Comment #1
Wim LeersComment #2
larowlanSure
Comment #3
borisson_I forgot to assign this to myself before I started working on this, I attached a patch that should add the correct cache tags. Test passes locally anyway. I have yet to write a specific test to see if the tags added.
Comment #4
borisson_Adds a test to check this as well.
Comment #5
Wim LeersCould you inject this?
ForumController
already has this:So most of the facilities are already there :)
Comment #6
borisson_Comment #8
borisson_Previous patch file was an interdiff. This one should be correct.
Comment #10
borisson_This should finally be the correct patch.
Comment #11
larowlanWe're also using taxonomy hierarchy in the index page - should we be adding tags for that too?
Comment #12
dawehnerThis seems to be rather we should fetch on runtime, doesn't it?
For that we probably also need the node type list tags or at least the node types cache tags.
Comment #13
Wim LeersNote that the "x new comments" stuff doesn't need to fixed here; that's a separate can of worms that already has its own issue: #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user.
Comment #14
borisson_I've added the node type list cache tags as dahwehner suggested in #12.2 and as per #12.1 we're now fetching the cache tags on run time.
I'm not sure which cache tag to add for taxonomy hierarchy, is this in the same way we're getting them for nodes / node list?
It feels really wrong to have such a big amount of arguments in the constructor.
Comment #16
dawehnerLet's not use private stupidness.
$tags is not used here.
Comment #17
borisson_Fixed both remakrs from #16.
Comment #18
dawehnerWe don't want that be part of the patch, don't we?
Comment #20
borisson_You are absolutely right, looks I should stop writing patches for today :-D. Attached what should be the correct patch.
Comment #21
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedAssigning to Wim for review.
Comment #22
larowlanI have no idea what either of these are. I think they're the 'The Node entity-type definition' and the 'The Node-type entity-type definition' - can we make the variable names and comments clearer?
I still think this lacks a reference to the term and comments. The number of comments will change the cached build.
Comment #23
Wim LeersSorry for taking so long to review.
Comments here need some love.
The first one makes sense, the second does not.
The second was added because of @dawehner's comment earlier:
@dawehner was right that it depends on the node type, but this is currently depending on the *list* of node types. That's wrong. We only need to depend on the node types that are actually depended upon.
So, in
::buildActionLinks()
, we want something like this:Then this would be replaced with something like
config:node_type.forum
Comment #24
Wim LeersComment #25
borisson_Fixes all the remarks made in #23 and #22.1
Comment #26
Wim Leers#25 fully addresses #23 and #22.1.
That only leaves #23.2:
By this, @larowlan was referring to
In there,
$forums
,$topics
,$parents
and$term
are all arrays of content entities. You basically want to iterate over all of them, and invoke$this->renderer->addCacheableDependency($build, $the_entity_being_iterated)
on all of them.And then, finally, regarding the "number of comments" part of @larowlan's comment, that needs the
comment_list
cache tag. Which means you want to inject the Comment entity type definition just like the Node entity type definition, and use its list cache tags.Comment #27
borisson_I added the extra cacheable dependencies. I noticed that
$term
is not an array but aTermInterface
, so I didn't add this in a loop.I added the comment list cache tag and changed the test so that it looks at at the
comment_list
cache tag. This might need extra work on the test to check for cache tags that are added trough the newaddCacheableDependency
calls.Comment #29
Wim LeersThe exceptions in the test runner show that this is also not an array, but a single entity.
Comment #30
borisson_Not happy with this solution but this does makes ForumTest pass locally.
Comment #31
Wim LeersCan
$topics
beNULL
? That's the only reason I can think of for that strange failure. Can't you then just change the logic that provides$topics
so that it defaults to the empty array instead?Comment #32
borisson_$topics
already defaults to an empty array.protected function build($forums, TermInterface $term, $topics = array(), $parents = array(), $header = array()) {
Comment #33
Wim LeersThen... huh? I can't use a debugger ATM, not at my workstation.
Comment #34
Wim LeersI did the necessary debugging to figure out the answer to #30/#31/#32/#33.
The root cause was silly & simple: in one branch,
\Drupal\forum\Controller\ForumController::forumPage()
was setting$topics
to the empty string. That caused it.That sole tiny fix doesn't prevent me from RTBC'ing this patch IMHO, so: RTBC! :)
Comment #35
alexpottIs there test coverage of this?
Comment #36
Wim LeersOops, you're right, those bits are missing from test coverage. Sorry.
Comment #37
lauriiiComment #38
Wim LeersAwesome, thanks!
Comment #39
alexpottNot exposing the correct cache tags is a bug no? This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 41560b3 and pushed to 8.0.x. Thanks!
Comment #41
Wim LeersHehe, right :)
Thanks!