Problem/Motivation

After #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user the situation for comment render caching got better. But in #1882482-105: [meta] Unify editing approaches in Drupal and #1605290-153: Enable entity render caching with cache tag support it's been discussed to further improve it by displaying the comment op links (delete, edit, reply etc.) over AJAX, so that we can apply full render caching of comments. I couldn't find an issue for this, so here it is.

Proposed resolution

Run comment op links through #post_render_cache so that they are cached the same for all users..

It would be nice to come up with an entity type agnostic way to do this, so that we can apply the same pattern for node links etc. There's plenty of contrib modules that adds user/role dependent stuff to those links that would break full render cache.

Remaining tasks

User interface changes

None.

API changes

use hook_comment_links_alter() instead of hook_entity_view_alter().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Category: feature » task
Priority: Normal » Critical
Anonymous’s picture

again, i'm kinda afraid of this approach.

i can see many site configurations where this adds a lot of extra http requests.

i don't know if this is fixable in D8, just waving the flag again that if there's any way to do this in one request, lets do it.

moshe weitzman’s picture

I wonder if we can batch up these links, and history markers, and whatever else and do them as one 'secondary' http request. i'm thinking of one controller that batches up up a few subrequests into a single json response. perhaps thats an optimization for a later day.

catch’s picture

I don't think this needs to add any http requests at all (except possibly the one for the js, but that's already there for new markers), we need the following information:

1. The uid of the author of each comment.

2. The uid of the current user.

3. Whether the current user has permission to edit/delete their/any comments and post comments.

Between those three bits of information the visibility of each link can be figured out. If you did some crazy access override on comment editing/deleting router access the links might be wrong unless you also updated the logic here, but that's also the case now so no regression.

Only the third one could possibly end up being an HTTP request but I don't think it has to be.

Anonymous’s picture

cool, so this is just a title change we need?

catch’s picture

Title: Display comment op links (delete, edit, reply) over AJAX for improved render caching » Display comment op links (delete, edit, reply) via js to prevent render caching granularity being per-user

How's this?

catch’s picture

With reply, if we cache per role (which I think the current patch does), we might not need to do that via js. That's purely permissions based so not per-user.

This also means that non-js users get to reply to threaded comments, lucky them (I hate threaded comments).

Wim Leers’s picture

Request batching would be nice to have. But that's out of scope here.
Also note that we have #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash to get rid of 2 HTTP requests/page (Edit + Contextual).


catch's outline in #4 is spot-on. Thanks to #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, we already have the UID of the current user (drupalSettings.user.uid). So then:

  • "reply" does not need any JS at all (as catch says in #7, because render caching is already per role). Which implies no JS for anon user — yay, speed!
  • "edit" and "delete" require some JS plus something like drupalSettings.user.commentPermission to indicate which permissions they have that are user-specific ("edit own comments")
  • users with the "administer comments" permission don't need any JS, they can perform all actions anyway

The most complex thing here hasn't been brought up yet: contrib modules adding their own links.

Rough, not thought through idea: if we would render all links on the server side, but then hide them by default on the front-end if they are user-specific (i.e. "edit" in the case of a role having the "edit own comments" permission), then we can make it work in a generic way, if we let the front-end know which comment-related permissions it has, and for each link which permission(s) it requires. That needs to get proper security review of course.

Wim Leers’s picture

Title: Display comment op links (delete, edit, reply) via js to prevent render caching granularity being per-user » Display comment op links (delete, edit, reply, approve + contrib) via JS to prevent render caching granularity being per-user
catch’s picture

I'm about to commit #1605290: Enable entity render caching with cache tag support - are we OK enabling node/comment render caching in this patch, or should that be in a new issue on this one? I think as long as it's a 2-liner it's OK here.

Wim Leers’s picture

I think as long as it's a 2-liner it's OK here.

+1 and that's what it should be: just removing a few unset($element['#cache'])-like lines of code introduced in #1605290: Enable entity render caching with cache tag support, and setting render_cache = TRUE in (Comment|Node).php.

amateescu’s picture

Nope, those unset($element['#cache']) lines are there for a good reason, for example not caching the output if we display a revision of the node because we don't deal with revision ids in cache tags.

I'm afraid it will be a bit more work than just removing 'render_cache' => FALSE from the node and comment annotations, we will also need to add back some manual cache clearing in tests that don't go through the regular rendering steps. I think there will be just a few of those, basically re-adding the interdiff from #1605290-135: Enable entity render caching with cache tag support.

moshe weitzman’s picture

I was just thinking that one shortcut to solving this is to move edit and delete into the contextual links system. That got moved to ajax in #914382: Contextual links incompatible with render cache. Then we are left with just reply link which can be done in php according to #8. I know that this will cause more contextual links to be shown on the site but those are not as expensive as they used to be. Also, edit/delete are rare operations. Just a thought.

dixon_’s picture

@moshe That's definitely a doable shortcut. But imo, we should at least explore the idea of providing a mechanism to solve this properly. There are many use cases for a cacheable entity links system for comments, node etc. Many contrib modules relies on links, and it'd be great if they didn't break caching.

dixon_’s picture

#1266306: Add a shortcut to unapprove a comment for consistency and usability is another use case that's adding links to comments.

larowlan’s picture

Correct me if I'm wrong but #731724: Convert comment settings into a field to make them work with CMI and non-node entities could fix this (assuming there is role-support for render-cache) if we move links to a formatter option (which is a follow up for that issue).

dixon_’s picture

@larowlan I'm not sure how you mean? If we make entity links configurable that should be configurable on the entity display itself. I'm not sure how #731724: Convert comment settings into a field to make them work with CMI and non-node entities would solve that, since that introduces a field on the "parent" entity rather that the entity carrying the relevant links.

larowlan’s picture

Ignore me, you are right

andypost’s picture

Wim Leers’s picture

Status: Postponed » Active
Issue tags: +Needs tests, +API addition
FileSize
7.04 KB

At first glance, I thought this issue would involve a lot of JavaScript. However, after actually working on a patch, that turns out to be not the case :) Zero HTTP requests and almost always zero JS seem feasible :)

The solution

Most links depend on the role only
Only the edit own comments permission also depends on the current user. Every other permission is solely role-based.
Consequence: role-dependent-only comment links + per-role render caching = most links don't need JS
The consequence is that the "delete", "reply", "approve" and "translate" links are entirely role-dependent, hence these can simply be render cached. The only special case is the "edit" link, and even then only when the user does not have the "administer comments" permission and does have the "edit own comments" permission: then we need to add a data- attribute and some JS to only show the "edit" link when the author of a comment is viewing the comment, and then the "edit" link should appear through JS.
Anon = no-js
And continuing our great trend of making sure anon page views are as fast as possible: this ensures zero JS is needed for anon users!

The patch

  • Thanks to andypost's comment in #19, I started working against #1975962: Move comment_links() into CommentRenderController, rerolled that patch and this patch is now based on top of that. That patch can probably land very soon, and in doing so, it keeps this patch very small and simple :)
  • API addition: the patch is conceptually simple, but I ran into implementation detail problems. For example, #attached assets on a #theme = links array that has #pre_render = drupal_pre_render_links set, won't work. So I had to fix that too, and to fix that, I had to introduce drupal_merge_attachments() — which odd enough didn't exist yet…
  • Concern: permission checking (i.e. user_access() calls for now) inside the CommentRenderController doesn't feel right. I tried to create an additional custom access check, like the "create" check (for which CommentAccessController::checkCreateAccess() exists): "role can update"/CommentAccessController::checkRoleCanUpdateAccess(). But that doesn't work; the way checkCreateAccess() works is by having a hardcoded check deep in the entity base classes… So I'm forced to use user_access(), which I don't like. I don't see a better way though. I hope somebody else does.

The generic solution

The generic solution (for comment and node links, and for anything else that has links) is simple:

  1. Do everything like you used to do before.
  2. BUT when there's a link that depends on anything else than just the user role, then add the "hidden" class (that Drupal core provides) and add a custom data- attribute of your own. Finally, #attach the JS file that is going be removing that "hidden" class when appropriate.

Thoughts for an automated generic solution (that happens to apply to render caching in general)

I considered working on a automated solution that would work not only for comment links, but also node links, and any custom links that contrib modules may add.

But to implement an automated solution that requires zero extra work on behalf of the contrib developer, we need more metadata. Such as: "this permission is user-specific" (cfr. "edit own comments"). We don't have that metadata right now. Requiring that would be a pretty big API change.

Furthermore, it's entirely possible that there are other "context-dependent permissions" ("edit own comments" depends on the "current user" context). We'd need a way to express all of them. That's D9 material.

Finally, all the node links in core are role-dependent only. Only the "edit" comment link depends on the current user. So, for now, contrib modules will simply have to duplicate the bit of JS I added and modify it for their specific needs.
The sole exception is book.module's "Add child page" node link, which is not context-dependent, but "arbitrary logic"-dependent because node access hooks can alter this! See #2099137-1: Entity/field access and node grants not taken into account with core cache contexts for the full explanation — that is also the issue where that general problem is being addressed..

P.S.: note that the fundamental problem here is that it's far, far too easy to break render caching. That's because render caching is an afterthought to the entire render system. What we need in an ideal world of happy kittens, ecstatic unicorns and relaxed llamas is that declaring the context dependencies of every renderable is an inherent part of the system. Much like classes implementing ContainerFactoryPluginInterface make it explicit which things are needed from the DIC, we also need a way of reasoning about each renderable, so that we can be much smarter about caching.

Conclusion

  1. The problem is elegantly solvable for comment links.
  2. An automated generic solution is not feasible in Drupal 8: we don't have sufficient metadata.
  3. Node links in core don't suffer from the same problem as the comment links do: they don't break render caching.
  4. The book module's "Add child page" link is impossible to cache, and either must disable (break) the render cache or perform an AJAX request for every page load. The general underlying problem is being tackled in #2099137: Entity/field access and node grants not taken into account with core cache contexts.

Next steps

  1. Reviews of the approach in this patch needed, only then write tests.
  2. Get #1975962: Move comment_links() into CommentRenderController committed.

EDIT: fixed three big confusing typos.

andypost’s picture

Assigned: Unassigned » Wim Leers
Issue tags: +Needs tests, +API addition, +sprint, +Spark

Suppose API addition should live in separate issue, probably related #1762204: Introduce Assetic compatibility layer for core's internal handling of assets

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
    @@ -162,12 +163,38 @@ protected function buildLinks(CommentInterface $entity, EntityInterface $comment
    -      if ($entity->access('update')) {
    

    This access check have more checks inside, see CommentAccessController::checkAccess()

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
    @@ -162,12 +163,38 @@ protected function buildLinks(CommentInterface $entity, EntityInterface $comment
    +      if (user_access('edit own comments') || user_access('administer comments')) {
    ...
    +        if (!user_access('administer comments')) {
    ...
    +                    'canEditOwnComments' => user_access('edit own comments'),
    

    This code should use \Drupal::currentUser()->hasPermission()

catch’s picture

Status: Active » Needs work
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.19 KB

#21: thanks, fixed those two flaws. Before starting to work on a separate issue with the API addition, I want approval of the approach.

Wim Leers’s picture

FileSize
1.99 KB

Forgot to upload the interdiff :/

catch’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
@@ -162,12 +163,38 @@ protected function buildLinks(CommentInterface $entity, EntityInterface $comment
+        if (!\Drupal::currentUser()->hasPermission('administer comments')) {

This works because render caching is per-role, but it looks iffy because it's using Drupal::currentUser(). I think we could use an extra line of comment to point out this is fine because we're checking a permission and caching by role handles that. This is also a case where if we eventually wanted to do better than per-role caching we could look at caching by those specific permissions but not for here.

The overall logic here looks great.

I can see the ability to merge attached arrays being useful, but also the fact we have to use it at all confuses me a bit - what specifically is breaking drupal_render_collect_attached() in that case? Is it because the children are pulled out custom?

I'm OK with handling the API addition in a separate patch, but 1. it should be critical with this issue postponed on it 2. it shouldn't be bundled in with the assetic integration #attached isn't going away and that patch has much wider scope.

Wim Leers’s picture

The overall logic here looks great.

Okay :)

I can see the ability to merge attached arrays being useful, but also the fact we have to use it at all confuses me a bit - what specifically is breaking drupal_render_collect_attached() in that case? Is it because the children are pulled out custom?

It's because:

  1. Only a specific link may need to have something attached, in our case: the "edit" link.
  2. However, the comment links are attached using this:
          $entity->content['links'] = array(
            '#theme' => 'links__comment',
            '#pre_render' => array('drupal_pre_render_links'),
            '#attributes' => array('class' => array('links', 'inline')),
          );
    

    Please look at drupal_pre_render_links(). It allows us to separate multiple logical groups of links into children, that drupal_pre_render_links() will again merge into a single list of links. Because of that merging, we also need attachment merging.

(I'm not even sure why this drupal_pre_render_links() fanciness is necessary. I'd prefer to get rid of it. But drupal_merge_attachments() is useful to have in general, I think. The only reason we haven't needed it before is because we usually call drupal_render_collect_attached() on render arrays with attachments, which then just collects and hence merges all attachments in a global…)

I'm OK with handling the API addition in a separate patch, but 1. it should be critical with this issue postponed on it 2. it shouldn't be bundled in with the assetic integration #attached isn't going away and that patch has much wider scope.

Oh, definitely — this is entirely unrelated to Assetic! I'm not even sure why we're still mentioning that. :)

I'll work on a stand-alone drupal_merge_attachments() issue first, then come back here and write tests.

Wim Leers’s picture

Status: Needs review » Postponed
Issue tags: -Needs tests, -API addition
FileSize
3.47 KB
8.8 KB
  1. #2113015: Add drupal_merge_attached() function, to merge #attached assets created, includes test coverage. Please go and review that :)
  2. The other blocker for this patch was just RTBC'd: #1975962: Move comment_links() into CommentRenderController.
  3. Rerolled this patch on top of the two above, and included test coverage. This patch is now ready to be RTBC'd, in theory :) (But postponed for now until the two above have made it in.)

P.S.: CommentLinksTest is one hell of a heavy and slow test! I think we could cut a few minutes of the test run time if we were able to convert this into a unit test. Though I don't see just yet how we're going to achieve that.

larowlan’s picture

Status: Active » Postponed
Issue tags: -Needs tests, -API addition

This looks really good.
Do the hidden links get read by screen-readers?
At first thought we'd need to add nofollow, but no need because robots won't get a session of course.

Wim Leers’s picture

#28: no, .hidden stuff doesn't get read by screen readers. See system.module.css, read the differences between .hidden, .visually-hidden and .invisible :)

Dave Reid’s picture

To go from using the entity access check to hard-coded checks against a role and user IDs seems like a big step backwards. Why?

Wim Leers’s picture

One could argue that this should leverage #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache. I thought so too for a second. But we'd both be wrong :)

As explained in #20, the only problematic case is the "edit" link, and even then only when the user's role has the "edit own comments" permission and not the "administer comments" permission. In that problematic case, we must only show the "edit" link for a comment in case:

  1. the comment's user ID (stored in data- attribute) matches the current user ID (stored in drupalSettings.user.uid), and
  2. the user's role has the "edit own comments" permission, only in this case does the necessary JS get attached

Since the necessary JS only is #attached for a certain permission, permissions are per role, and render caching happens per role (i.e.: JS per role, role per render cache entry), this patch already cleanly solves the problem, #2118703 does not help with that at all!


The only thing #2118703 enables us to do, is to implement a no-JS solution, but that is actually worse for most types of hosting. Please read #2118703 to understand why.


Once the blockers for this patch land (#2113015: Add drupal_merge_attached() function, to merge #attached assets & #1975962: Move comment_links() into CommentRenderController), I should do a reroll to remove drupalSettings.comment.canEditOwnComments; that's an unused leftover that should be removed.

Wim Leers’s picture

#30:

  1. I don't like the code either, see #20 → The Patch → Concern. But that's not a problem of this patch/solution, it's a failure on behalf of the rest of Drupal core.
  2. Even though it looks different, it's exactly the same logic as before! CommentAccessController::checkAccess('update') runs this:
    ($account->id() && $account->id() == $entity->uid->value && $entity->status->value == COMMENT_PUBLISHED && user_access('edit own comments', $account)) || user_access('administer comments', $account)
    

    Note that in the patch, we retain exactly that, with the exception of the first two comparisons:

    $account->id() && $account->id() == $entity->uid->value
    

    These now happen in JavaScript. Why in JavaScript? Because if it happens in PHP, it breaks the render cache, because it's user-specific.

DamienMcKenna’s picture

Status: Postponed » Needs review

Lets revew the patch, then bump it back to 'postponed'.

Dave Reid’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
@@ -161,12 +162,38 @@ protected function buildLinks(CommentInterface $entity, EntityInterface $comment
-      if ($entity->access('update')) {
+      if ($entity->status->value == COMMENT_PUBLISHED && \Drupal::currentUser()->hasPermission('edit own comments') || \Drupal::currentUser()->hasPermission('administer comments')) {

This is the change that concerns me. Wouldn't this skip hook_entity_access() from being invoked? So possibly we'd show or hide links when it isn't correct (EDIT: yes I know clicking those links should still check entity access properly on the next page load)?

Status: Needs review » Needs work

The last submitted patch, comment_ops_links-2090783-27.patch, failed testing.

Dave Reid’s picture

If this is indicative of how other entity types in contrib will have to display admin links when it is being viewed, I need to add the DX tag here.

Wim Leers’s picture

Status: Needs work » Postponed

#3: ugh, now the patch is red :(

It was postponed for a reason: the patch depends on two other patches being committed first. You can still review this issue while it's marked as postponed. Setting it to NR will cause test failure, which will cause NW, which is worse than postponed.


#34: Yes, you're right, sorry that I didn't confirm that explicitly in #32. The same logic is used when using just Drupal core, but you're right that it robs modules from being able to interfere with hook_entity_access().

Entity access in general is the cause for a HUGE amount of performance problems. The only way we can make things fast is if we have predictable caching (and not per-user caching, which could indeed more easily be predictable, but it would require a huge cache and result in extremely low cache hit ratios).
Since entity (and field) access altering can introduce arbitrary logic, we inherently cannot cache anymore. You could literally implement an entity access system where you can only access a node if the outside temperature is 17ºC and it's 17:03.
Without having explicitly realized it, all of the above implies indeed that what I'm proposing is to disallow crazy entity access altering out-of-the-box in favor of much better performance out-of-the-box.

In essence, I think this is where we need to move to: make fast by default, hence make caching predictable by default, at the expense of no longer being able to do CRAZY things (like 17ºC + 17:03). If you want CRAZY flexibility, you'll have to override the whole thing.

I don't think a nice solution is possible in Drupal 8. It's either performance, or CRAZY flexibility. See #2090783-20: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user, "Thoughts for an automated generic solution (that happens to apply to render caching in general)" for a general solution: we need more metadata, but we can't do that in D8.

The general issue for "entity/field access altering plus render caching" is: #2099137: Entity/field access and node grants not taken into account with core cache contexts. So probably that is in fact the issue where you want to add the "DX" tag? :)

cosmicdreams’s picture

If we go with the "fast default" solution would it be possible to extend the system with more flexible access later?

Dave Reid’s picture

If the choice is performance vs crazy flexibility, then I pick crazy flexibility. It's what brought me to Drupal in the first place.

catch’s picture

You still get crazy flexibility, you just have to be crazily flexible in the right place.

andypost’s picture

More flexible solution could be done by moving permissions to field level #1903138: Move global comment permissions to comment-type level

Wim Leers’s picture

Status: Postponed » Needs review

#27: comment_ops_links-2090783-27.patch queued for re-testing.

Wim Leers’s picture

Now we can discuss the "crazy entity access alterations" problem further. But let's wait for amateescu to chime in, because he seems to have an idea for a magical solution :)

amateescu’s picture

Wim Leers’s picture

Issue tags: +Performance

Adding "Performance" tag for tracking purposes.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

I'd love to use #post_render_cache for this, as amateescu would like. But unfortunately that's harder than it ideally would be: the comment ops links are not render arrays, but are just plain arrays that are returned for use with drupal_pre_render_links().
To be able to use #post_render_cache, we'd need to make relatively big changes. And we won't be able to use #type = render_cache_placeholder due to the fact that we must use plain arrays rather than render arrays, so we'll have to create our own placeholder, and not set #post_render_cache on individual links, but on the logical group of links. Finally, we'll need to update drupal_pre_render_links() to ensure it merges the #post_render_cache callbacks for each group into the final collection of links.

If that sounds like a convoluted solution, you're not alone. However, it turns out that the code is not too bad at all. I think we should consider using DOMDocument rather than preg_match() though.

The only flaw is that until #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors lands, any time the "edit comment" list item gets removed because the current user cannot access it, all list items after it will have the wrong odd|even class!

See the attached patch. No interdiff because it's completely different.

This addresses Dave Reid's entity access concerns and amateescu's JS concerns.

Wim Leers’s picture

Status: Needs review » Needs work

The last submitted patch, 46: comment_ops_links-2090783-46.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: comment_ops_links-2090783-46.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
amateescu’s picture

So.. this solves the problem for the edit link, but what if someone needs the same behavior for other links, they'd have to do the same thing all over again for each additional link?

Wim Leers’s picture

#52: Yes… but only for links that depend on more things (contexts) than just the user's role.

The only alternative is to not render *any* of the comment links while rendering the comments, but render *all* comment links in a #post_render_cache callback. Then comment links wouldn't be alterable (+ extensible) anymore.

I think the only solution is to completely revise how rendering works in D9: each thing should specify the contexts it depends on, so that the render system can autonomously decide which things can safely be render cached (because caching happens for a certain configurable set of contexts, like the current "cache by role" default) and which things should be rendered post-render caching.

Status: Needs review » Needs work

The last submitted patch, 46: comment_ops_links-2090783-46.patch, failed testing.

amateescu’s picture

#52: Yes… but only for links that depend on more things (contexts) than just the user's role.

I thought it was obvious that I was referring to this kind of links, sorry if it wasn't.

I think the alternative is to make those links render arrays. We're not helping anyone with one-off fixes/hacks like this.. If that's all we can do in core, imagine what contrib will do.

I think the only solution is to completely revise how rendering works in D9: each thing should specify the contexts it depends on, so that the render system can autonomously decide which things can safely be render cached (because caching happens for a certain configurable set of contexts, like the current "cache by role" default) and which things should be rendered post-render caching.

This is what I was trying to achieve with the simplified post_render_cache proposal, and it certainly doesn't feel like a D9 task, otherwise we'd need to step back with render caching in D8.

Wim Leers’s picture

I think the alternative is to make those links render arrays. We're not helping anyone with one-off fixes/hacks like this.. If that's all we can do in core, imagine what contrib will do.

I agree!
But how do you propose that would work? I don't know why link rendering was designed to work this way (theme_links() accepting #links that don't contain render array *and* the whole drupal_pre_render_links() thing), but I can only assume it was done for good reason?


Your next point I think is off-topic for this issue, but I'll answer anyway:

This is what I was trying to achieve with the simplified post_render_cache proposal, and it certainly doesn't feel like a D9 task, otherwise we'd need to step back with render caching in D8.

I think I may not have been explicit enough in what I said. I meant that every single element in a render array should indicate all of its dependencies in a well-defined, consistent, succinct way, so that the code responsible for the rendering itself can understand the content. It needs to understand the content if it needs to be able to autonomously decide which things can safely be render cached.
Doing that in the render array system that we're using today would mean a huge amount of additional data in an already complex data structure. I don't see how that can work at all in our current Render API. I don't know yet what would be the better alternative, I just know that our Render API is not designed to support that — at least not without very much getting in the way.

So, from my point of view, your proposal for #post_render_cache definitely did not do that. It did do that as much as it could within the constraints of the current Render API. But by no means did it result in a contexts/dependencies being defined in a well-defined, consistent or succinct way.
AFAICT it's render caching in D8 is necessary to compensate for a lot of other systems having gotten slower (due to more abstraction). However, the render system did not get changed in any significant way in D8. Hence the way #post_render_cache is piggy-backed on top of it is a necessary evil.

Wim Leers’s picture

In #46 I said:

The only flaw is that until #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors lands, any time the "edit comment" list item gets removed because the current user cannot access it, all list items after it will have the wrong odd|even class!

This is wrong. There's another flaw. Because the "edit" links presence or absence is determined after all the links are rendered, the logic to determine whether the "Log in or register to post comments" message should be displayed is also broken:

      if (empty($links)) {
        $comment_post_forbidden = array(
          '#theme' => 'comment_post_forbidden',
          '#commented_entity' => $commented_entity,
          '#field_name' => $entity->field_name->value,
        );
        $links['comment-forbidden']['title'] = drupal_render($comment_post_forbidden);
        $links['comment-forbidden']['html'] = TRUE;
      }

It's broken because $links is never empty anymore.

Wim Leers’s picture

I'm working on getting CommentViewBuilder::buildLinks() in its entirety to run as a #post_render_cache callback. That would solve both amateescu's concerns and the huge problem explained in #57.

EDIT: the downside of that is that it becomes impossible to alter the "delete", "edit", "reply" and "approve" links using alter hooks. You'd have to alter them by overriding \Drupal\Comment\Entity\Comment's annotation: controllers.view_builder = "Drupal\comment\CommentViewBuilder" to point to your own class, which could inherit most logic from CommentViewBuilder. I think this is acceptable? That's how you need to alter things in most systems besides Drupal: through subclassing.

dsnopek’s picture

EDIT: the downside of that is that it becomes impossible to alter the "delete", "edit", "reply" and "approve" links using alter hooks. You'd have to alter them by overriding \Drupal\Comment\Entity\Comment's annotation: controllers.view_builder = "Drupal\comment\CommentViewBuilder" to point to your own class, which could inherit most logic from CommentViewBuilder. I think this is acceptable? That's how you need to alter things in most systems besides Drupal: through subclassing.

But what if two modules want to add two distinct and unrelated things to the comment links? There can only be one sub-class registered as the CommentViewBuilder, right? I think that would be very limitting to contrib.

What if the comment system added it's own alter hook that would be called somewhere in this process? I know adding *new* hooks at this point is frowned on, so maybe an event or something?

Anyway, just throwing ideas out there. :-)

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +API change
FileSize
6.54 KB
10.68 KB

In this patch I've applied the idea in #58, which I've already checked with amateescu and he considers sound. To solve the lack of alterability, amateescu suggested to just add an alter hook invocation in the #post_render_cache callback. Which I did.

The code is now much simpler. And passes tests. All that's needed is test coverage for the newly added hook_comment_links_alter(), but before I do that, I want confirmation from amateescu and at least one other person that this solution is sound and acceptable.

Also: this introduces an API change! If this patch goes in, you will have to alter comment entity links using hook_comment_links_alter(), not using hook_entity_view_alter().


This effectively solves all "dynamic and/or crazy-access-rules-dependent links" for comments. But… it doesn't yet do that for nodes. And nodes effectively have the same problem: nodes will be render-cached by role (just like anything else), but that means "dynamic and/or crazy-access-rules-dependent links" will not work for nodes. So we should probably apply the same pattern to nodes in a follow-up.
Or come up with a better solution, which none of us have thought of yet.

The last submitted patch, 60: comment_ops_links-2090783-60.patch, failed testing.

Wim Leers’s picture

HEAD had changed: comment links have been capitalized ("approve" became "Approve"), which broke the patch. Simple reroll.

The last submitted patch, 62: comment_ops_links-2090783-62.patch, failed testing.

amateescu’s picture

  1. +++ b/core/modules/comment/comment.api.php
    @@ -197,5 +198,32 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    +function hook_comment_links_alter(array $links, CommentInterface $entity, $view_mode, $langcode, EntityInterface $commented_entity) {
    

    Shouldn't $links be received by reference?

  2. +++ b/core/modules/comment/comment.api.php
    @@ -197,5 +198,32 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    +  $container = \Drupal::getContainer();
    

    There's only one usage of the container so this can inlined below, where it's used.

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
    @@ -146,6 +150,42 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    +    drupal_alter('comment_links', $links, $entity, $context['view_mode'], $context['langcode'], $commented_entity);
    

    You should use \Drupal::moduleHandler()->alter() here.

    Also, you're passing too many "contexts" to the alter method and so they need to passed as an associative array instead.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
10.45 KB
7.26 KB
  1. You're right. That's just a docs error though. Fixed. Thanks!
  2. Idem.
  3. Ugh, great catch!

Also: added test coverage. Finally: \Drupal\comment\CommentInterface's documentation was wrong, fixed that too.

amateescu’s picture

I forgot to say in the last comment, but the new approach here looks great to me. I mentioned a couple doxygen problem to Wim in IRC, but other than that, this looks RTBC.

I'll let another person review and set it as such, based on Wim's request in #60.

Wim Leers’s picture

FileSize
10.46 KB
1.5 KB

Fix the two Doxygen problems amateescu pointed out in IRC.

Wim Leers’s picture

Wim Leers’s picture

Assigned: Wim Leers » catch

Assigning to catch for feedback — this issue is de facto RTBC, but still needs sign-off on the new hook/API change:

[…] this looks RTBC.

I'll let another person review and set it as such, based on Wim's request in #60.

The last submitted patch, 67: comment_ops_links-2090783-67.patch, failed testing.

The last submitted patch, 65: comment_ops_links-2090783-65.patch, failed testing.

Wim Leers’s picture

FileSize
10.47 KB
1.97 KB

Now it should pass all tests.

moshe weitzman’s picture

Title: Display comment op links (delete, edit, reply, approve + contrib) via JS to prevent render caching granularity being per-user » Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Adjust title and issue summary to match current approach. Hope I got it right.

Code looks clean and well documented. RTBC per #66

moshe weitzman’s picture

Issue summary: View changes
catch’s picture

Title: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user » Change notice: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Having a more targeted hook for the link altering is fine for this I think, iirc we used to have a dedicated link in Drupal 6 until the links were moved to the render array.

Rest of the patch looks good as well, so committed/pushed to 8.x, thanks!

Wim Leers’s picture

xjm’s picture

Issue tags: +beta blocker

Retroactively marking beta blocker as a blocker for #2151459: Enable node render caching.

xjm’s picture

Issue tags: +beta blocker
Wim Leers’s picture

Title: Change notice: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user » Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user
Assigned: catch » Wim Leers
Status: Active » Fixed
Issue tags: -sprint, -Needs change record

Change notice written: https://drupal.org/node/2152957

Wim Leers’s picture

Priority: Major » Critical

Retroactively marking critical: a blocker to a critical issue should itself be critical, and this blocked #2151459: Enable node render caching, which is critical.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jessebeach’s picture

Issue summary: View changes