Stop placeholdering comment links

Isn't this a duplicate of #2579021: Prevent comment forms from marking rendered nodes as uncacheable?

This is the basic change we need.

Let's see what fails.

This ensures the necessary cacheability metadata is present.

Note that this needs #2579021: Prevent comment forms from marking rendered nodes as uncacheable to be in.

Without that patch, the entire rendered node is not cached, because of the comment form's CSRF token's max-age=0 that is bubbled. If you apply both that patch and this patch, you'll see render cache entries like entity_view:node:1:full:* again, and you'll see that without this patch, the comment links are placeholders, and with this patch, they are included right in the node's render cache entry (one set of comment links per comment on the node), meaning they aren't rendered after everything else anymore.

Finally, for clarify, let me repeat part of what I wrote at #2556767: Remove placeholdering of node links, but adjusted for this issue. So great to see that almost two years after #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user, we are now able to simplify it enormously thanks to #2429257: Bubble cache contexts :)

Marking those as related issues to simplify future archeological work.

#4 unsurprisingly shows that we have almost no test coverage for this. #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user added some test coverage (specifically to check that altering in additional comment links actually works), but ideally we'd have more.

Feel free to contribute test coverage.

One less fail.

Oh, links, I misread the title, sorry.

+++ b/core/includes/common.inc
@@ -856,9 +856,11 @@ function drupal_pre_render_links($element) {
-    // Merge attachments.
-    if (isset($child['#attached'])) {
-      $element['#attached'] = BubbleableMetadata::mergeAttachments($element['#attached'], $child['#attached']);
+    // Merge bubbleable metadata.
+    if (isset($child['#cache']) || isset($child['#attached'])) {
+      BubbleableMetadata::createFromRenderArray($element)
+        ->merge(BubbleableMetadata::createFromRenderArray($child))
+        ->applyTo($element);
     }

This change causes test failures in StandardTest.

Why this change? To bubble the cacheability metadata from individual hook_comment_links_alter() implementations. And consequently, also for hook_node_links_alter(). Yes, drupal_render_pre_links() is very strange, and it is optimized specifically for hook_(comment|node)_links_alter()'s needs. Without this change, the cacheability metadata of the additions/alterations made by those hooks are lost, and therefore things are varied insufficiently.

Why this change causes a failure? Because node links (and thus also links added by hook_node_links_alter()) are already no longer being placeholdered always, since #2556767: Remove placeholdering of node links. And comment_node_links_alter() (the comment.module implementation of hook_node_links_alter()) is specifying cacheability metadata, which was being lost. No specific test coverage exists for this, because until #2556767: Remove placeholdering of node links, this was always being placeholdered, i.e. was always being rendered late, i.e. was never cached, and hence we never considered it necessary to write explicit test coverage for the node links.

Since #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache, comment_node_links_alter() is specifying the user cache context, which is the precise reason for this test failure. Since that issue the individual links are not placeholdered anymore, because the overall links were already being placeholdered. #2556767 then changed that, but failed to make the code change quoted above, so actually, the per-user "last read data" is being cached as part of nodes right now.

This reroll fixes the fail in StandardTest. It lets us once again rely on the JS that is responsible for rendering this anyway. Since that JS then no longer receives the data it needs to do the client-side rendering, it instead needs to do a request to the server. But the results of that are cached on the client-side anyway.

Fixing the last fail.

Note that #14 is an important bugfix that should go in regardless of this issue — it can be done in a separate issue if we want.

Finally, let's prove the full node page is still cacheable by the Dynamic Page Cache.

We actually had a test for this, but #2511516: Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable commented it out, and it should've been uncommented in #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags, but that was forgotten, because both were in progress simultaneously. Uncommenting that test shows that Dynamic Page Cache considers the full node page UNCACHEABLE. Uh-oh.

But actually, we already kinda know why — see the #9 interdiff:

+++ b/core/modules/comment/src/Tests/CommentCacheTagsTest.php
@@ -139,7 +139,7 @@ public function testCommentEntity() {
+    return ['user', 'user.roles:authenticated'];

The edit own comments permission is causing the comment links to be cacheable per user. Therefore the comment links still vary by user.

Next, because entities are not yet rendered using #lazy_builder (they're still rendered using #pre_render), they cannot yet be auto-placeholdered. If they used the #lazy_builder pattern, this patch would cause all commented entities to be auto-placeholdered because of that edit own comments permission. Of course, that also wouldn't help us at all: we'd then be rendering the entire commented entity including all comments and all comment links on every page load… which is even worse than the problem this issue tries to solve (avoiding rendering all comment links on all page loads).

And so because the user cache context bubbles up to the page in the current patch, that means that the full node page can not be cached by Dynamic Page Cache.

But it's quite clear that due to the edit own comments permission, this can never work in a performant way, unless we defer the rendering of the "edit" comment link to the client-side. But that's quite a bigger undertaking.

Conclusion: I don't think it actually makes sense what the issue title says. The "edit" comment link unfortunately gets very much in the way.

Finally, let's prove the full node page is still cacheable by the Dynamic Page Cache.

We actually had a test for this, […]

This reroll gives us that test. Per the explanation in #17, this will fail.

BUT! Most of the work done for this issue is still important/relevant. Let's look at each hunk in #18:

1. common.inc: this causes the actual cacheability metadata for node links to be bubbled, see explanation in #14
2. CommentLazyBuilders: it is better (but not essential) for placeholdered content to provide correct cacheability metadata
3. CommentLinkBuilder: this is currently a bug in how nodes are cached, ever since #2556767: Remove placeholdering of node links landed, in two ways even: A) the history metadata becomes stale very quickly, for it is not invalidated by cache tags, B) the history metadata is actually per user, and it has the user cache context but in HEAD (see point 1), that cacheability metadata is discarded/ignored, so we cache user-specific data across users. This is wrong (the "X new comment" counts will be for a different user). The hunk in point 1 fixes that, but causes nodes to be cached per user. That's also very bad. So this hunk is necessary to keep nodes cacheable across users.
4. CommentViewBuilder: was for this issue, can be discarded, because this issue is impossible to fix while we have the "edit own comments" permission, see #17.
5. CommentCacheTagsTest: was for the previous point, can be discarded.
6. CommentDefaultFormatterCacheTagsTest: was also for the previous point, can also be discarded.
7. StandardTest: brings back important test coverage proving that the full node page is indeed still cacheable by Dynamic Page Cache, important combined with the other hunks that are important to keep.

So, repurposing this issue for those things. Patch is the same as #18, but with irrelevant hunks removed (see points 4, 5 and 6 above).

Ok, so as the CommentNewIndicatorTest failure in #14 and #15 shows, we do have some test coverage for CommentLinkBuilder's logic (that we're changing since #14). That's great. We'll still need to fix that test for the patch in #23, but all the things I wrote in the past few comments are still all true.

This does what I said in #24.

+++ b/core/profiles/standard/src/Tests/StandardTest.php
@@ -193,11 +193,10 @@ function testStandard() {
+    $this->drupalGet($url);
+    $this->drupalGet($url);

Does this need to happen twice?

I suppose it does to prove that you hit the cache.

Nothing to see here!

If it's not a security issue or causing data loss, it shouldn't be critical.

I was wondering also if this was critical. https://www.drupal.org/core/issue-priority

putting the template in the summary, that should help (as it has a place to fill in if there would be api changes and data model changes)

note, the original report said "Marking as critical just to be conservative. I don't actually think this needs to block RC1." which supports the not being critical move.

I think this is something we'd commit to a patch release (unless I missed something) so agreed on major.

I agree; this is a pure bugfix since #23, so can go in after RC1, e.g. for RC2.

The "Performance" issue tag is also no longer applicable since #23.

As already spoken in IRC:

The best way out is to make comment rendering itself a lazy builder - the whole block - BUT also add the same cache keys as the node.

Then the node is cacheable and the page with blocks is cachable, but the comments and comment links are rendered and cached independently - e.g. per user.

This also makes a lot of sense going forward when thinking of BigPipe / streaming styling things, the most important things first, then load the comments later - which is already the case on 90% of the pages e.g. using disqus.

Another maybe less invasive change is to just add #cache keys to the comment links, but keep the #create_placeholder.

There is another trick we can do here ;).

We can use #lazy_builder, which generates #pre_render + #cache in a child element.

Then the cache keys could be dynamically generated by the lazy builder based on e.g. the owner vs. current uid information.

Of course it would only work for the standard cases we know about and a new link could still make it cacheable per user only.

But it would work ... (the same as comments are render cached on drupal.org now)

#39: we can't use #lazy_builder for comment links because of the way drupal_pre_render_links() and #theme => links work. At least not last time I looked.

The other idea I had, explained at a high (not 100% accurate) level: every comment has a data-comment-user-id attribute. We could match that up with drupalSettings.user.uid, and then only show the edit link if the user has the edit own comments permission.
EDIT: but obviously this immediately breaks down when you have custom access control for comments, e.g. group-based. I was just posting the idea, perhaps it'll inspire a better solution :)

One idea I had which would handle the placeholdering for us was reversing what we did on #611642: Roll back comment contextual links (temporarily). However we'd need some kind of fallback for sites that don't have contextual module enabled then.

catch had a great insight that allows us to still address the original issue. Currently working on that. But first lunch.

So the simple yet genius insight is this:

-        return AccessResult::allowedIf($account->id() && $account->id() == $entity->getOwnerId() && $entity->isPublished() && $account->hasPermission('edit own comments'))->cachePerPermissions()->cachePerUser()->cacheUntilEntityChanges($entity);
+        // If the user doesn't have the permission to edit their own comment,
+        // return early with better cacheability.
+        if (!$account->hasPermission('edit own comments')) {
+          return AccessResult::neutral()->cachePerPermissions();
+        }
+        // Otherwise, if they do have the permission, do the further necessary
+        // checks, at the cost of poorer cacheability.
+        return AccessResult::allowedIf($account->id() && $account->id() == $entity->getOwnerId() && $entity->isPublished())->cachePerPermissions()->cachePerUser()->cacheUntilEntityChanges($entity);

For #42:

One idea I had which would handle the placeholdering for us was reversing what we did on #611642: Roll back comment contextual links (temporarily). However we'd need some kind of fallback for sites that don't have contextual module enabled then.

I've opened #2580407: Consider using contextual links for Comments again with an initial patch, against 8.1.

The JS based solution sounds interesting but I don't see how we can implement that in a practical timeframe. This is all fairly generic code that relies on entity access, which in turn has hooks that you can implement or even replace the access control handler completely. We can't just hardcode some assumptions there, now.

Group based access might not be common, but time based edit access, that only allows edit access for a few hours or until you have responses are not that uncommon and will be implemented by sites (stackexchange does the time thing, for example).

On #44, That only helps if you have no role with that permission. One user accessing a page with that permission is enough to force that cache context for all users, as they are merged together. Unfortunately, we have no way to specific something like "cache by permissions, unless it's *that* specific permission hash, then it is per user". I don't even want to imagine how complicated that would become ;)

I still think we should send the comment links out of band and cache them individually.

That is way better than to force the node to be per user.

This is not only smart-cache, but the node render cache being affected, too.

So like:

#create_placeholder => TRUE,
+#cache => [
+ 'keys' => [...],
+],

instead of removing #create_placeholder.

Edit: Also our auto-placeholdering implementation really missed the case that something does not have cache keys, but a lazy builder and such can be placeholdered, but adding #cache keys would solve it and we could even remove the #create_placeholder because as it is cacheable it would correctly be auto-placeholdered.

I still think we should send the comment links out of band and cache them individually.

But that will create many, many, many cache entries!

#49: Yes, but possibly in combination with your permissions patch way less?

Also could consider a max-age on those ...

On #44, That only helps if you have no role with that permission. One user accessing a page with that permission is enough to force that cache context for all users, as they are merged together.

It's not quite as bad as this.

All users that have the permission will get the comments rendered per-user.

All users that don't have the permission get the same copy (per permission).

So the performance hit with #44 only applies to roles with that permission. This will help some sites, not those where the authenticated role has edit own comments permission though of course.

#51:

That only works when we have auto-placeholdering for entries without cache keys, which should be a 3 line patch by now, but which we don't have at this present moment.

Because else if in any variation 'user' bubbles up then the node will vary based on that. (The self-healing part of the Cache Redirect RenderCache system.)

IRC log with more explanation:

11:08 < Fabianx-screen> berdir: WimLeers: I think this is a case where we really need auto-placeholdering to work for something that does not have #cache keys.
11:08 < Fabianx-screen> ( in the case we don't want to micro cache it )
11:09 < Fabianx-screen> Because then:
11:10 < Fabianx-screen> a) if a user does not have the permission => no ['user'] cache context => not placeholdered
11:10 < Fabianx-screen> b) if a user does have the permission, 'user' cache context => auto placeholdered => same as in HEAD right now
11:12 < Fabianx-screen> But auto-placeholdering after rendering concentrated on ensuring it was cached in the same item (on my initiative). We missed that we 
                        should always auto-placeholder after rendering - regardless if we can cache or not. because an upper level would cache it - in this case 
                        the node.
11:12 < Fabianx-screen> And then auto-bubbling would not lead to the problem.
11:13 < Fabianx-screen> as the node varies on the user.permissions.
11:13 < Fabianx-screen> hash
11:14 < Fabianx-screen> and depending on that hash (has permission), it would have placeholders or not have placeholders.
11:14 < Fabianx-screen> which is fine.
11:14 < catch> Fabianx-screen: could we also per-user only if has permission?
11:14 < catch> Fabianx-screen: i.e. everyone else gets per-permission.
11:15 < Fabianx-screen> catch: yes, that is the idea Wim implemented.
11:15 < Fabianx-screen> catch: But we need to ensure that we auto-placeholder, too.
11:15 < catch> Fabianx-screen: doh that was my idea :P
11:15  * catch not awake yet.
11:15 < catch> Fabianx-screen: so the auto-placeholdering is only for those users that have the permission.
11:15 < Fabianx-screen> catch: yes
11:15 < catch> Then they get the same content, and only the placeholder is per-user.
11:15 < Fabianx-screen> catch: yes
11:16 < Fabianx-screen> catch: users with permission get core HEAD performance, users without the permission get better performance.
11:16 < Fabianx-screen> So we can only win.
11:16 < Fabianx-screen> catch: Because if we don't auto-placeholder, then the node would eventually vary by user, too - per our implementation of bubbling.

I don't think it's reasonable to expect access modules to re-implement CommentLazyBuilder. The whole point of cache contexts was to avoid that, i.e. #2099137: Entity/field access and node grants not taken into account with core cache contexts.

Contextual links already has a js solution that works generically regardless of access implementation, so we know it's possible to do generically (even if we decided not to actually use contextual links here - but that was added to core with the initial contextual links patch and only rolled back due to performance).

Another idea, similar to blocks, cache/handle access separately from actually building the links, and cache a variation for each visible link permutation (e.g. all links, or only edit but not delete). I'm not sure how easy it is, but it might be worth thinking about.

We're also working on #2580551: Optimize getCommentedEntity(), which should make rendering those links faster.

#55 and #56 both seem worth looking at.

Neither #55 nor #56 can be done before RC1. It'll need to be either an RC target or a minor version target. Tagging both; a committer can remove either.

#55: Yes, the problem is that we need to change cache keys in that case and need to bubble that up - which is impossible.

Dynamic cache keys however are possible when the lazy builder generates those on run-time and goes via a child element + #pre_render / #cache pattern.

e.g. checking all links for access first - but we don't have a language to express that (Fred / NonFred), yet.

That was the idea of #39 to allow Fred / NonFred and cache just the comment links, but you need knowledge of the information here - so it would only work for core.

Likely #52 is the most simple fix, would just need a simple if statement in Renderer. Unfortunately the static cache for placeholdered fragments is in PlaceholderingRenderCache and not in an independent service - like a static cache render strategy.

So this needs some more changes - e.g. making creatingPlaceholderAndCache public or moving somewhere else.

---

Edit:

Turns out user.is:5 would totally work - using a kinda binary map approach to this problem and compatible with reducing to user if needed.

I still think we should placeholder it and cache it independently, e.g. all the comments together.

That would avoid that binary map to bubble up to node cache and dynamic page cache - so less unneeded variation there.

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary. I gather the TLDR is "performance", but what about the disruption?