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().
Related Issues
- #1830598: [meta] support render caching properly
- #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user
- FIXED IN D8 (eea18a48). D7 backport
#914382: Contextual links incompatible with render cache - #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash
- Node links need similar treatment
Comment | File | Size | Author |
---|---|---|---|
#72 | interdiff.txt | 1.97 KB | Wim Leers |
#72 | comment_ops_links-2090783-72.patch | 10.47 KB | Wim Leers |
Comments
Comment #1
catchComment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedagain, 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.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #4
catchI 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.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedcool, so this is just a title change we need?
Comment #6
catchHow's this?
Comment #7
catchWith 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).
Comment #8
Wim LeersRequest 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:drupalSettings.user.commentPermission
to indicate which permissions they have that are user-specific ("edit own comments")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.
Comment #9
Wim LeersComment #10
catchI'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.
Comment #11
Wim Leers+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 settingrender_cache = TRUE
in(Comment|Node).php
.Comment #12
amateescu CreditAttribution: amateescu commentedNope, 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.Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #14
dixon_@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.
Comment #15
dixon_#1266306: Add a shortcut to unapprove a comment for consistency and usability is another use case that's adding links to comments.
Comment #16
larowlanCorrect 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).
Comment #17
dixon_@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.
Comment #18
larowlanIgnore me, you are right
Comment #19
andypostRelated #1975962: Move comment_links() into CommentRenderController
Comment #20
Wim LeersAt 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
edit own comments
permission also depends on the current user. Every other permission is solely role-based.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.The patch
#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 introducedrupal_merge_attachments()
— which odd enough didn't exist yet…user_access()
calls for now) inside theCommentRenderController
doesn't feel right. I tried to create an additional custom access check, like the "create" check (for whichCommentAccessController::checkCreateAccess()
exists): "role can update"/CommentAccessController::checkRoleCanUpdateAccess()
. But that doesn't work; the waycheckCreateAccess()
works is by having a hardcoded check deep in the entity base classes… So I'm forced to useuser_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:
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
Next steps
EDIT: fixed three big confusing typos.
Comment #21
andypostSuppose API addition should live in separate issue, probably related #1762204: Introduce Assetic compatibility layer for core's internal handling of assets
This access check have more checks inside, see CommentAccessController::checkAccess()
This code should use \Drupal::currentUser()->hasPermission()
Comment #22
catchComment #23
Wim Leers#21: thanks, fixed those two flaws. Before starting to work on a separate issue with the API addition, I want approval of the approach.
Comment #24
Wim LeersForgot to upload the interdiff :/
Comment #25
catchThis 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.
Comment #26
Wim LeersOkay :)
It's because:
Please look at
drupal_pre_render_links()
. It allows us to separate multiple logical groups of links into children, thatdrupal_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. Butdrupal_merge_attachments()
is useful to have in general, I think. The only reason we haven't needed it before is because we usually calldrupal_render_collect_attached()
on render arrays with attachments, which then just collects and hence merges all attachments in a global…)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.Comment #27
Wim LeersP.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.Comment #28
larowlanThis 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.
Comment #29
Wim Leers#28: no,
.hidden
stuff doesn't get read by screen readers. Seesystem.module.css
, read the differences between.hidden
,.visually-hidden
and.invisible
:)Comment #30
Dave ReidTo go from using the entity access check to hard-coded checks against a role and user IDs seems like a big step backwards. Why?
Comment #31
Wim LeersOne 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:
data-
attribute) matches the current user ID (stored indrupalSettings.user.uid
), andSince 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.Comment #32
Wim Leers#30:
CommentAccessController::checkAccess('update')
runs this:Note that in the patch, we retain exactly that, with the exception of the first two comparisons:
These now happen in JavaScript. Why in JavaScript? Because if it happens in PHP, it breaks the render cache, because it's user-specific.
Comment #33
DamienMcKennaLets revew the patch, then bump it back to 'postponed'.
Comment #34
Dave ReidThis 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)?
Comment #36
Dave ReidIf 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.
Comment #37
Wim Leers#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? :)
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedIf we go with the "fast default" solution would it be possible to extend the system with more flexible access later?
Comment #39
Dave ReidIf the choice is performance vs crazy flexibility, then I pick crazy flexibility. It's what brought me to Drupal in the first place.
Comment #40
catchYou still get crazy flexibility, you just have to be crazily flexible in the right place.
Comment #41
andypostMore flexible solution could be done by moving permissions to field level #1903138: Move global comment permissions to comment-type level
Comment #42
Wim Leers#27: comment_ops_links-2090783-27.patch queued for re-testing.
Comment #43
Wim LeersNow 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 :)
Comment #44
amateescu CreditAttribution: amateescu commentedPosted my thoughts and proposal here #2118703-18: Introduce #post_render_cache callback to allow for personalization without breaking the render cache.
Comment #45
Wim LeersAdding "Performance" tag for tracking purposes.
Comment #45.0
Wim LeersUpdated issue summary.
Comment #46
Wim LeersI'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 withdrupal_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 updatedrupal_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 thanpreg_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.
Comment #47
Wim LeersComment #49
Wim Leers46: comment_ops_links-2090783-46.patch queued for re-testing.
Comment #51
Wim Leers46: comment_ops_links-2090783-46.patch queued for re-testing.
Comment #52
amateescu CreditAttribution: amateescu commentedSo.. 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?
Comment #53
Wim Leers#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.
Comment #55
amateescu CreditAttribution: amateescu commentedI 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.
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.
Comment #56
Wim LeersI 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 wholedrupal_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:
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.Comment #57
Wim LeersIn #46 I said:
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:
It's broken because
$links
is never empty anymore.Comment #58
Wim LeersI'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 fromCommentViewBuilder
. I think this is acceptable? That's how you need to alter things in most systems besides Drupal: through subclassing.Comment #59
dsnopekBut 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. :-)
Comment #60
Wim LeersIn 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 usinghook_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.
Comment #62
Wim LeersHEAD had changed: comment links have been capitalized ("approve" became "Approve"), which broke the patch. Simple reroll.
Comment #64
amateescu CreditAttribution: amateescu commentedShouldn't $links be received by reference?
There's only one usage of the container so this can inlined below, where it's used.
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.
Comment #65
Wim LeersAlso: added test coverage. Finally:
\Drupal\comment\CommentInterface
's documentation was wrong, fixed that too.Comment #66
amateescu CreditAttribution: amateescu commentedI 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.
Comment #67
Wim LeersFix the two Doxygen problems amateescu pointed out in IRC.
Comment #68
Wim LeersComment #69
Wim LeersAssigning to catch for feedback — this issue is de facto RTBC, but still needs sign-off on the new hook/API change:
Comment #72
Wim LeersNow it should pass all tests.
Comment #73
moshe weitzman CreditAttribution: moshe weitzman commentedAdjust title and issue summary to match current approach. Hope I got it right.
Code looks clean and well documented. RTBC per #66
Comment #74
moshe weitzman CreditAttribution: moshe weitzman commentedComment #75
catchHaving 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!
Comment #76
Wim LeersIssue + patch for node op links posted: #2151439: Run node op links ("Read more", "Add child page", "Printer-friendly version", "X views" + contrib) through #post_render_cache to prevent render caching granularity being per-user.
Change notice in progress.
Comment #77
xjmRetroactively marking beta blocker as a blocker for #2151459: Enable node render caching.
Comment #78
xjmComment #79
Wim LeersChange notice written: https://drupal.org/node/2152957
Comment #80
Wim LeersRetroactively marking critical: a blocker to a critical issue should itself be critical, and this blocked #2151459: Enable node render caching, which is critical.
Comment #82
jessebeach CreditAttribution: jessebeach commented