Problem/Motivation
Advanced Comment Threads bypasses several of Drupal's cache layers even though its content is well-suited to caching. The most significant is per-comment render caching: the slice renderer strips the comment view builder's build pre-render and invokes it eagerly to relocate links, so the renderer never sees a cacheable comment element and the guard that should add an act_comment cache variant never fires.
Measured on a 50-comment slice: an ACT slice goes 367.3 ms cold to 365.2 ms warm (about 1% saved), whereas the same comment through core's normal path goes 299.7 ms cold to 10.9 ms warm (a render-cache hit). ACT is paying the full render cost on every request. Additional misses: the thread build itself has no cache keys, redundant comment cache tags are added per comment, the versions map serializes and hashes every comment's toArray() on each view, and the fragment routes are marked no_cache.
Steps to reproduce
- Request a comment fragment twice and compare warm vs cold time; observe almost no warm savings.
- Inspect the rendered comment element: no cacheable entity element is present because CommentSliceRenderer.php:70-76 invokes the view builder's build eagerly.
- Observe the versions map serializing and hashing every comment's toArray() on each node view.
Proposed resolution
- Restore per-comment render caching. Stop invoking the view builder's build eagerly at CommentSliceRenderer.php:70-76, or wrap the comment element so it retains a cacheable entity element with its own cache keys; ensure the act_comment cache variant at :86-88 actually applies.
- Give the thread build a cache key and correct tags, contexts, and max-age so it is render-cached rather than rebuilt.
- Make version tracking cheap: derive the version from fields already in the index item (cid:changed:status:childCount) instead of serialize(toArray()), which also avoids loading full entities and avoids exposing admin-only fields (mail, hostname) in the hash.
- Memoize the comment index at request level so build() and the history lazy builder share one load; core forbids passing an object through a lazy builder.
- Drop the redundant per-comment cache tags at CommentThreadBuilder.php:114-116. The page currently carries 223 tags, 216 of them comment:NNN, and comment_list (already added by CommentIndexRepository.php:129) covers every invalidation path: EntityBase::invalidateTagsOnSave() invalidates the list tags on create and on update. Per-comment tags belong on the per-comment render cache entry, where they already are.
- Make fragment responses render-cacheable by removing the unconditional setCacheMaxAge(0) at CommentFragmentController.php:282 and letting the cacheability already collected in CommentSliceRenderer decide. Do not hand-assert a context set: every access result is added as a cacheable dependency precisely so the correct contexts bubble.
- Conditionally enable anonymous page cache where it is not already blocked: the history lazy builder currently sets max-age 0 unconditionally; for anonymous readers the reading-state features are inert, so it can return a real max-age plus user/user.permissions contexts.
Fragment markup is not neutral today
An earlier draft of this issue assumed fragment markup was identity-neutral and could therefore be shared at user.permissions granularity. It is not. Rendering the same fragment (cid 7829 on a 216-comment node) as three accounts:
- anonymous: 5,086 bytes
- comment owner (uid 5): 5,655 bytes
- admin (uid 1): 5,655 bytes
The edit and delete links are present for the owner and the admin and absent for anonymous, and no BigPipe placeholder is present in any of the three. CommentSliceRenderer.php:79-82 rewrites core's comment.lazy_builders:renderLinks to ACT's own callback, and the eager build() at :70-76 then resolves it inline, so there is no placeholder isolating per-user content. Anything cached at user.permissions granularity would serve one user's edit/delete links to another. The routes still enforce access, so this is a correctness defect rather than an exploit, but it will read as a security defect in review.
The slice already bubbles the right contexts (user.permissions, user, route.name.is_layout_builder_ui), so removing the forced max-age 0 is safe on its own; it simply yields a per-user cache rather than a shared one.
Two of the bullets above are ordered, not independent
Fragments can only become shareable rather than merely cacheable-per-user once the action row is a placeholdered lazy builder again. That is blocked by the same eager build() the per-comment render caching bullet addresses. Restore per-comment render caching first; fragment shareability is a consequence of it, not a parallel task.
Remaining tasks
- Preserve access enforcement: keep the bubbled user/user.permissions contexts and the per-comment access check so unpublished or pending comments never enter a shared cache.
- Verify markup neutrality empirically before widening any cache granularity: render the same fragment as anonymous, a comment owner, and a moderator, and diff the output. Treat identical output as the precondition for sharing, not an assumption.
- Verify against a real 200+ comment thread, not only unit tests.
User interface changes
None. The fixes make the same page and fragment loads faster without changing behavior.
API changes
- The slice renderer and fragment endpoints return cacheable metadata instead of a hard no-cache, retaining user and user.permissions cache contexts.
- The version map is computed from lightweight index fields rather than full entity serialization.
Data model changes
None anticipated. No schema or configuration change is required.
Comments
Comment #2
freelockUpdated the issue summary after further analysis from Opus, DeepSeek -- the original summary did not properly distinguish caches between users with the same permissions -- users with the same permissions would cache edit links for comments they owned, and other users may then see the wrong links.
Comment #8
freelockShipped in alpha3 - followup issue #3623891: Cached summary cards retain an author's old name after a rename