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

freelock created an issue. See original summary.

freelock’s picture

Issue summary: View changes

Updated 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.

  • freelock committed 3ad2a476 on 1.0.x
    fix: #3622844 Vary thread cache keys by the field commenting status
    

  • freelock committed 65c7dfe6 on 1.0.x
    fix: #3622844 Tolerate stubbed entities in the slice render cache...

  • freelock committed 133fd3f2 on 1.0.x
    test: #3622844 Expect the commenting status in the thread cache keys
    

  • freelock committed d06c5a23 on 1.0.x
    test: #3622844 Stub the cache contexts manager in the slice renderer...
freelock’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.