Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 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 nodes
- Convert access checks to use
$entity->access()
.
Follow-ups:
Comment | File | Size | Author |
---|---|---|---|
#45 | 1862750-45.patch | 22.75 KB | fubhy |
#45 | 1862750-45.interdiff.txt | 4.42 KB | fubhy |
#39 | 1862750-39.interdiff.txt | 3.25 KB | fubhy |
#39 | 1862750-39.patch | 22.54 KB | fubhy |
#35 | node-access-controller-1862750-35.patch | 23.77 KB | Berdir |
Comments
Comment #1
Wim Leers#1824500: In-place editing for Fields needs this to get rid of its current hack.
Comment #2
fubhy CreditAttribution: fubhy commentedThanks for opening this. Writing a patch now.
Comment #3
fubhy CreditAttribution: fubhy commentedI started playing around with this a bit... I was a bit sloppy with writing in-line documentation for the code. I am posting this anyways to get some early reviews as I am a bit unsure about the approach. The create access part makes it kinda ugly if you ask me. The node_access() function is currently a mixture of single and "any" entity access (you don't have to pass a node object). Let's see if this even works...
Comment #5
fubhy CreditAttribution: fubhy commentedTrying to kill some of the notices.
Comment #7
BerdirI guess it makes sense to change createAccess() to not get an entity object at all? will require a separate way to acess it, but you never have one if you ask if someone can create one?
Comment #8
dawehnerMh, so it would make at least sense to get the bundle as you may want to have create access per bundle.
This small fix should fix a lot of the exceptions.
Comment #10
Wim LeersTagging as per #1.
Comment #11
fubhy CreditAttribution: fubhy commentedWill take over again from here.
Well... It may look weird when you look at it like that, yes... But then again, how can we put createAccess in-line with the rest if not like this? Also, who will make a judgement on what createAccess() needs and what not? So, bundle is pretty obvious then... But in some use-cases we also need the author UID? We can't cover every single possible attribute that might be relevant for checking for access like that. Hence why we went with a full entity object. I don't see any other way for providing us with a good, generic solution (without requiring custom entry points!) for solving this in a way that gives us enough flexibility.
Comment #12
fubhy CreditAttribution: fubhy commentedComment #13
dawehnerReviewed the code under the aspect of ignoring documentation problems.
what about cacheID or better entityCacheID?
Should we support a way to truncate this cache?
Let's try to avoid ?: in such relative often used functions.
Should we call $account->id() twice, but $this->cacheId ... not?
What about $this->setCached($status, ...) instead?
These two blocks of code really look similar, maybe we should try to unify them.
We certainly need explainations in code itself, as that's not a simple logic.
Comment #15
fubhy CreditAttribution: fubhy commentedYes, totally... I am still playing around with it because I am moving a lot of stuff, splitting it up, unifying it again, etc. ...
I plan to add @todos there all over the place anyways because this whole area of issues (anything that touches the global user basically) gets ugly due to these not-yet-fixed issues:
#1825332: Introduce an AccountInterface to represent the current user
#1549526: Change global $user into $session
#361471: Global $user object should be a complete entity
#1277682: Move responsibility for global $user out of session
Comment #16
BerdirReplaced those entity_create() calls with a user_load() as entity_create() blows up if global user is a loaded user entity.
In the other issues, we tried to avoid the User/stdClass problem, we might be able to do that here too by postponing the type hint on the internal helper methods until that is cleaned up.
Just want it to get green first, will look at the review later.
Comment #17
dawehnerFor sure some documentation is needed, so this is just some small feedback.
Shouldn't these calls then also be changed?
What are the plans to make entity controllers injectable? Seems to be a bit related to the discussion about making plugins injectable.
Maybe parenthesis would help the readability a bit.
Should bundle() be used here?
It's kind of odd, as this could cause problems when you have two node add forms on the same page, but hey, this has been the case before as well.
Comment #18
fubhy CreditAttribution: fubhy commentedThanks for reviving this issue @Berdir.
Yes, we still need to solve that problem. @tim.plunkett already improved the way we are building and retrieving controllers but ultimately we need to find a fully injectable solution. It's funny that you mention that today because I chatted about that with @fago yesterday on the subway :). However, this discussion is going to be slightly different than the one we are currently having in the #1863816: Allow plugins to have services injected because the requirements are different. So, in case of Entities, we build plugin definitions based on the annotations of the entity classes which also contain the definitions for the controllers used by that entity type. However, we probably don't want to inject those into the entity objects, instead we probably want to inject the entity manager into the entity objects (which is kinda weird because that basically means we are injecting the plugin manager into our plugin instances) which in return will manage the controllers. So either the entity manager is the factory for our controllers directly or we move that responsibility to another layer (some kind of proper controller factory) possibly even per controller type... Anyways, this is the wrong place for that discussion.
I am going to clean this patch up now and add the required documentation.
Comment #19
BerdirAs discussed with @davereid in IRC, once this is done, we should look at the different access controllers and try to come up with a sane default implementation for entities. Mostly, that will mean moving currently node-specific features into a base implementation I guess. For example, changing hook_node_access() to hook_entity_access() + hook_$entity_access(), like all other $entity hooks now are.
For simple cases, that could even mean a definition of the related permissions in the entity info (e.g. view_access => 'access content') so that cases that are as trivial as VocabularyAccessController don't need need a custom implementation for it.
Comment #20
fagohm, this mis-uses entity_create as factory - but it isn't. I think we should stop doing so and start introducing one.
For now, we could add a interim helper function doing it without entity_create() and add a todo to fix it there?
+1. It would be nice if we could have the access controller provide the permission also, such that it's self-contained - defines and checks for the permissions. We'd have to invoke the controllers from the hook somewhere I guess....
Comment #21
fubhy CreditAttribution: fubhy commentedStarting to go for a default EntityAccessController implementation. Invoking a hook there. 100% untested therefore probably dark red, but who knows?
Comment #22
Wim Leers#21: :D
Comment #24
fubhy CreditAttribution: fubhy commentedNext try. There were some minor but obvious hiccups...
Also... We need a better solution for the static caching... What about using a proper static cache bin? We need a way to invalidate cached access checks.
Comment #25
fubhy CreditAttribution: fubhy commentedActually uploading the patch is always a good idea.
Comment #27
BerdirYou nicely tricked yourself out ;) Type safe comparison but still return the old constant values doesn't match well...
Comment #29
amateescu CreditAttribution: amateescu commentedThis is crazy, why not use the UUID which is always available?
Comment #30
BerdirChanges:
- Re-rolled for NodeNG. Some parts currently look a bit ugly but that's temporary.
- Added resetCache() to the access controller interfaces and implemented it.
- Fixed the translation test failure, that was because the permissions were checked in the wrong order, FALSE wins over TRUE.
No useful interdiff due to the re-roll.
Comment #32
jthorson CreditAttribution: jthorson commentedFrom testbot apache error logs:
Comment #33
BerdirThanks, I thought it would be something simple as that, some access controllers didn't extend from EntityAccessController which makes sense given that it previously didn't do anything.
Comment #35
BerdirThis should be green.
Comment #36
BerdirI'm not sure if I like the current process, we have separate methods on the interface (which are now almost identical) only to redirect it to the same one again? Would it maybe not make sense to invert that somehow?
Why not make those entity constants? They were introduced in 7.x I think to make it easier to distinguish between TRUE/FALSE/NULL return values because implementations often incorrectly returned FALSE when they should have returned NULL.
Comment #37
fubhy CreditAttribution: fubhy commentedYes, I found that particularly tedious too with this patch and some other that I looked at. I will open a follow-up for refactoring the interface. I guess we should simply go with forwarding the ->access() invocations from Entity::access() and TranslatableInterface::access() etc. to EntityAccessController::access() and do the mapping there whenever necessary (if necessary).
Also a task for a follow-up. And yes, we should consider making that a re-usable, entity-generic constant somewhere. +1 on that because I can see how it improves DX. However, that's also a follow-up task.
RTBCing this patch here. Will open the follow-ups asap.
Comment #38
fubhy CreditAttribution: fubhy commentedPer discussion on IRC we put UUIDs into the mix here as suggested by @amateescu
Comment #39
fubhy CreditAttribution: fubhy commentedComment #40
fubhy CreditAttribution: fubhy commentedSorry for the spam.
Comment #41
amateescu CreditAttribution: amateescu commentedThank you! Looks much better now :)
Comment #42
fubhy CreditAttribution: fubhy commentedI set up two follow-up issues following a quick chat on IRC with @berdir:
#1947892: Improve DX with EntityAccessControllerInterface
#1947880: Replace node_access() by $entity->access()
Comment #43
Wim LeersThanks for pushing this forward :)
Few nitpicks, one WTF, and a few things that appear to be wrong (despite passing tests).
"not reliably"? I think it's simply "not at all"? :)
Link to follow-up issue? Also: shouldn't this @todo also exist elsewhere in the code?
Idem.
Copy/paste leftover.
Shouldn't this be the other way around? Currently if it's an EntityNG node, then it used $node->status, whereas it should be the other way around?
Why have "node" in the variable names?
WTF?
As before.
Comment #44
fubhy CreditAttribution: fubhy commentedYes, that @todo should be in about 500 places in core :(.
That's related to that :(
The Symfony Session issue & a few more that basically circle around the topic of global $user / $session, etc.
#1858196: [meta] Leverage Symfony Session components
#335411: Switch to Symfony2-based session handling
#1549526: Change global $user into $session
#1825332: Introduce an AccountInterface to represent the current user
etc. ... There are many issues that discuss that problem.
Probably copy&paste from NodeNG conversion :)
Comment #45
fubhy CreditAttribution: fubhy commentedComment #46
Wim LeersBack to RTBC. Thanks :)
Comment #47
BerdirI think this description was copied from the interface, where it probably states the same. But that's already in and we can improve that in the follow-up issue.
Comment #48
Dries CreditAttribution: Dries commentedThis looks good. Committed to 8.x. Thanks.
Comment #49.0
(not verified) CreditAttribution: commentedAdding follow-ups.