Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
#1696660: Add an entity access API for single entity access got commited, but now we need to actually implement the API for all entity types.
- Implement an entity access controller for comments
- Convert access checks to use
$entity->access()
.
Comment | File | Size | Author |
---|---|---|---|
#44 | drupal-1862754-44.patch | 15.07 KB | Berdir |
#37 | drupal-1862754-37.patch | 15.13 KB | dawehner |
#37 | interdiff.txt | 2.44 KB | dawehner |
#31 | durpal-1862754-31.patch | 13.22 KB | dawehner |
#31 | interdiff.txt | 2.33 KB | dawehner |
Comments
Comment #1
dawehnerTo support menu access callbacks we seem to need still something like entity_access().
Comment #3
dawehnerOh damn, forgot to add the new file to the patch.
Comment #4
dawehnerYeah i'm totally not sure whether it's a good step to rename 'edit' to 'update' in this patch.
Comment #6
dawehnerAfter some discussion with fubhy let's not add another function but reuse comment_access.
Comment #8
BerdirThis isn't going to end well if $account is NULL :)
Wondering if we should make $account a required argument here, to avoid having to add hardcoded assumptions about $GLOBALS['user']...
Comment #9
dawehnerThis seems to be a good idea! I'm not sure which level should load the current user, but i guess the entity level is the only one that allows to provide a good DX.
Comment #10
dawehnerWe could either patch the entity class first, or implement the argument as optional and then start to optimize it.
Should we maybe alternative set the global user always to an entity object?
Comment #12
dawehner#10: drupal-1862754-10.patch queued for re-testing.
Comment #13
BerdirYes, but that's a different issue. with it's own set of dependencies. The global $user is set in session.inc, where we don't have access to the entity system yet. There's both an issue for doing just this and the symfony one that completely replaces the session handling.
Comment #14
BerdirWill need a re-roll and should also get an implementation for the other methods.
As happened with the other conversion, let's kill comment_access() and use entity_page_access and update other access callbacks too.
This isn't really an appropriate way to create a user object IMHO.
Let's just use $account->uid and not worry about this for now.
Comment #15
dawehnerThanks for the review! Rerolled, changed the translation access controller as well.
Added a new access $op for approving comments and removed comment_access().
Comment #16
dawehnerI'm sorry, that's the wrong patch for this issue.
Comment #18
BerdirThis should be $account->uid now :)
Lovely. Did we do that for user/terms too?
Comment #19
dawehnerGood point.
For user there is no TranslationController,
One question: What should happen if you call $entity->access('random_op_that_does_not_exists'), because currently it blows up totally.
Comment #21
dawehnerSo it fails because $account is NULL in most cases, so we somehow need to get the current user from the global.
Comment #22
BerdirOh right, missed that. Yes, but $account = $GLOBALS['user'] should be enough. That's what the other implementations do and we just check uid, which is always there (Although it will get complicated once we convert User to NG but that's not specific to this) and pass it to user_access().
Comment #23
dawehnerLets see whether this works.
Comment #25
dawehnerHopefully this is a better one.
Comment #27
BerdirMost tests seem to fail with: Argument 1 passed to entity_page_access() must implement interface Drupal\Core\Entity\EntityInterface, string given.
Looks like you mixed this up with timer changes.
Comment #28
BerdirChanged the menu definitions to %comment, no idea why we weren't doing that already as it is a nice simplifcation. Also fixed some issues in the access controller. Comment is a NG entity, so we need to use $entity->uid->value and so on.
Comment #29
dawehnerThanks for fixing the remaining issues!
These changes are looking pretty solid, but I'm not sure whether I'm "allowed" to set this to RTBC?
Just wondering whether we should use entity_uri() instead + append "approve"?
Comment #30
fagoI think that would be dangerous as the entity-uri can be altered without supporting approve.
Implements what? This does not exist. For now I think it's best to just have this method, but with an actually right comment ala "Defines access for the custom 'approve' operation." ?
For a proper solution, I think we either need to introduce a proper entity operations API or define a CommentAccessControllerInterface.
Setting to needs work for fixing the wrong comment. Else I think this is good to go.
Comment #31
dawehnerLet's use an interface for now.
Comment #32
fagoThanks, improvements look good. Setting to RTBC given bot gives us green light!
Comment #33
catchThis reverts #638070: Router loaders causing a lot of database hits for access checks which in turn is closely related to #607244: Decrease performance impact of contextual links.
The short version is that now you have to load a comment to check access, whereas in most of these cases you could just call user_access() and skip loading the comment altogether.
It's possible we could mitigate this if #1237636: Entity lazy multiple front loading happens but no-one's working on that and it wasn't a popular suggestion when I originally posted it.
Needs more discussion.
Comment #34
BerdirHm. Right, at the moment, this is a performance issue for comments. I think that it's not that much of a problem with my patch that enables static caching for comments, that should be ready.
I'll try to get some numbers of how this affects performance with and without the comments static cache issue.
Comment #35
catchIf we move contextual links access checks to an AJAX callback per #914382: Contextual links incompatible with render cache, you'll want to be able to check access on the comments in that separate request.
It'll still be an issue there if that access check involves loading the comments first. With #1237636: Entity lazy multiple front loading we'd instantiate the entity with the ID, but not actually load it from storage until a property is accesssed, which won't happen for the access checks.
Comment #36
BerdirI've been trying to profile this but couldn't really find those problematic access calls.
The reason seems to be that we do not display contextual links for comments. I'm not sure if we want to do that (edit.module ?) later bot for now, we seem to be ok. Couldn't find any noticable differences with/without this patch.
I did notice a few things but not related to this. In general, it seems to be very hard to profile a 300 comments page. The time it takes to render the page is *doubled* with profiling enabled, which limits the usefulness of the result a lot.
I think the main argument for consistently using $comment->access() in all cases is that it allows to overwrite comment access logic in a single place instead of having to alter all kinds of different things. And at least in the current state, it does IMHO not cost us too much.
What I did notice is that we should probably switch to $comment->access() checks in comment_links() as well, as far as possible. I think the performance decrease in that function shouldn't be high as $comment is already available. So setting back to needs work for that.
Comment #37
dawehnerThis indeed makes the code a bit easier to read.
Comment #38
BerdirThat looks great, yes.
Setting back to RTBC to get some feedback to #36 from @catch or someone else.
Comment #39
larowlanAny chance we can pause this until #731724: Convert comment settings into a field to make them work with CMI and non-node entities gets in (or otherwise)? Would hate another massive re-roll at this late point in time.
Comment #40
webchickWe can, but then let's mark this postponed.
Although that other issue is kinda controversial, so this one might be easier to get in.
Comment #41
BerdirThis is the last big missing entity access conversion that is missing. I'd love to get it in as it would unblock certain quite interesting issues and other things will be broken without this (client side edit of comments once access checks are converted to entity access, entity references of comments, ..).
Comment #42
Berdir#37: drupal-1862754-37.patch queued for re-testing.
Comment #44
BerdirRe-roll.
Comment #45
andypostSuppose better to wait for #1947892: Improve DX with EntityAccessControllerInterface
Any reason to implement another interface?
Comment #46
dawehnerThe point is that we add a new method, so an extended interface seems to make sense. If you want to implement another access logic for comments, you will be happy to have an interface, which explains you what you have to implement in core.
Comment #47
Wim LeersThis works well! (Tested for in-place editing of comments, i.e. in combination with #1966704: In-place editing for taxonomy terms and custom blocks.)
What is blocking RTBC? The waiting for #1947892: Improve DX with EntityAccessControllerInterface?
Comment #48
BerdirDoesn't matter which issue gets in first. I've no problem with re-rolling the DX issue.
This was initially postponed on the comments field issue, but I'm not seeing that much activity there and really would like to see this in as well. I don't think it would be too complicated to re-roll after this, @larowlan?
Comment #49
larowlanMuch of the work on the comment field issue has been taking place in #1907960: Helper issue for "Comment field"
We're stuck on some views changes that are causing some fails.
Let's not hold this patch up.
Comment #50
larowlanBased on #38, #40 and #49
Comment #51
Berdir#44: drupal-1862754-44.patch queued for re-testing.
Comment #52
andypost+1 to commit the awesome clean-up
Comment #53
webchickCatch had some concerns earlier, want to assign it to him to make sure that he's comfortable with this.
Comment #54
BerdirIf necessary, I can, at least for now, roll back the menu access callback changes. My main interest right now is to make $comment->access($op) work ASAP to be able to rely on the entity access API for the main content entities and therefore allowing to remove unnecessary hooks and node-specific code like e.g. in edit.module and file download access checks.
Comment #55
catchSo the access callback issue isn't a problem in core at the moment, it's going to be one if we ever attach contextual links etc. to comments again though. Really that's contextual module's problem rather than comments, so I've gone ahead and committed this.
If I regret it later this comment will be linked back to in git blame for extra facepalming.
Comment #56
webchickLOL. :) Sounds good. Thanks, catch!
Comment #57
Wim LeersROFL