This issue is part of #1949932: [META] Unify entity fields and field API.
EntityNG fields implement access control via Drupal\Core\Entity\Field\Type\Field::access() which invokes hook_entity_field_access(). Meanwhile, field.module has field_access() which invokes hook_field_access(). These hooks have different signatures. We need to unify all this into a common API.
Motivations for the API break
In place editing currently only checks "configurable fields" field_access(), and bypasses access checks on "entity fields".
WidgetBase & FormatterBase also have a field_access() check, with a @todo on this issue.
Patch summary
- Field access is queried by calling EntityAccessController::fieldAccess(), which reproduces the behavior of current HEAD's for "EntityNG" field access: invoke hook_entity_field_access() & hook_entity_field_access_alter(). hook_field_access(), used by the "old Field API", is removed in favor of the above.
- This EntityAccessController::fieldAccess() method *optionally* receives a FieldItemList object, allowing the various hooks to implement optional logic based on specific values of the field. There are however cases for checking field access for "a field generally", not in the context of a specific $entity (ex: Views UI), so this parameter needs to be optional. D7's field_access() worked that way already.
- As a shorthand, fetching access grants in the context of a specific entity can be done by calling the access() method on the FieldItemList object : $node->body->access($op);
- Implementors of hook_entity_field_access_alter() need to be able to reason on a FieldDefinitionInterface object (which unifies base fields and configurable fields). Thus the patch adds an initial implementation of a FieldDefinition class for base fields, that is currently still missing in HEAD.
API break
- D7's field_access() is deprecated in favor of EntityNG's Field::access() / EntityAccessController::getFieldAccess()
- D7's hook_field_access() is removed in favor of EntityNG's hook_entity_field_access()
Remaining tasks
- agree on the approach :-)
- convert remaining field_access() calls in Edit, Views, WidgetBase / FormatterBase
Comment | File | Size | Author |
---|---|---|---|
#64 | unify_field_access-1994140-64.patch | 43.6 KB | Berdir |
#64 | unify_field_access-1994140-64-interdiff.txt | 2.21 KB | Berdir |
#60 | unify_field_access-1994140-60.patch | 43.61 KB | Berdir |
#60 | unify_field_access-1994140-60-interdiff.txt | 680 bytes | Berdir |
#59 | unify_field_access-1994140-59.patch | 43.6 KB | smiletrl |
Comments
Comment #1
fagoShortly discussed this with yched via mail. Discussed options have been:
- unfiy this upon the new FieldDefinitionInterface
- keep both for now, and make configurable fields call both hooks
Then, thinking a bit more about it I realized that it should not be really necessary to go via the FieldDefinitionInterface as $field for a configurable field holds all the context necessary - if given.
=>
The only problematic case is when no $entity is available. For this case, we could easily create a field object nevertheless, but the hook would miss $entity_type then. We could make it use an empty entity as entity-create access checks do right now?
Also, both hooks currently do not clarify what a not existing $entity means. Entity.module in d7 documents that case to be "have access to ALL entities", but I think Views used to use or uses a different approach of "have access to ANY entity". We need to become clear on that, but I suppose that's a separate issue.
Comment #2
yched CreditAttribution: yched commentedUpdating tags, and bumping priority.
In place editing currently only checks "configurable fields" field_access(), and bypasses access checks on "entity fields".
WidgetBase & FormatterBase also have a field_access() check, with a @todo on this issue.
Comment #3
yched CreditAttribution: yched commentedYes, there are cases where you need to check access on a field without having an entity - hence, without having a Drupal\Core\Entity\Field\Field on which to call access().
Examples in core right now:
- views (Drupal\field\Plugin\views\field\Field::access() - the "views 'field handler' plugin for Field API fields", overrides HandlerBase::access())
- autocomplete callbacks
In fact, field_access() initially didn't have an $entity argument. It was added later as an optional param (which I tried to resist :-p), to allow hook_field_access() implementations to do more fine grained logic on a per-entity context, *without* relying on its presence.
So:
- we can't merge hook_field_access() with hook_entity_field_access() if the latter assumes there is always a FieldItemList $field object to pass as context.
- having the entry point of the API as an $object::access() method when we don't always have that $object (FieldItemList), nor can make one up (we have no 'bundle' available in views either so we can't create an $entity), seems difficult too.
And I'm not really in favor of starting to generate "mock" $field objects without an $entity parent, seems like an awful can of worms. If you're a value, you're a value of an entity, values don't exist independently right now and it's much better that way IMO.
Also, I don't really see the ability to let field type subclasses override the access() logic as a really important feature ?
This is why I'm looking for another object to put this access() method:
- the FieldDefinition object...
- maybe the EntityManager ?
Comment #4
yched CreditAttribution: yched commentedIf we went with FieldDefinitionInterface::access(), we'd still want to keep the same implementation for both base and configurable fields though (invoke hook_entity_field_access() to collect array of grants, then hook_entity_field_access_alter()...)
That would mean either:
- putting that in a base class for both FieldDefinition class (= the definition object for base fields) and in field.module's Field config entity (= the definition object for configurable fields) to extend. But Drupal\field\Plugin\Core\Entity\Field can't really extend anything else then ConfigEntityBase, so, no go.
- duplicating that code, bleh.
So FieldDefinitionInterface doesn't look too good. Besides, FieldDefinitionInterface is there to abstract differences between base fields and configurable fields, so not much point in putting stuff that's the same for both in there.
So, EntityManager::fieldAccess($op, FieldDefinitionInterface $field_definition, AccountInterface $account = NULL, Field $field = NULL) ?
Comment #5
yched CreditAttribution: yched commentedCould look something like the attached patch.
Puts the fieldAccess() method in EntityAccessController rather than directly in the EntityManager, though.
Comment #5.0
yched CreditAttribution: yched commenteddetailed summary
Comment #5.1
yched CreditAttribution: yched commentedtodos
Comment #5.2
yched CreditAttribution: yched commentedformatting
Comment #7
yched CreditAttribution: yched commentedBleh, wrong order of arguments
Comment #9
yched CreditAttribution: yched commentedAnd missing "use" statement.
Comment #11
yched CreditAttribution: yched commentedHook params that can be NULL need to be marked as such.
Then, bleh, of course this can't work while Core\Entity\Field\Field::getFieldDefinition() returns NULL :-/...
So I'm afraid this is blocked on #2047229: Make use of classes for entity field and data definitions...
Comment #12
yched CreditAttribution: yched commentedForgot updated patch in #11.
Comment #14
fagoWe need to be able to easily customize field access for entity fields, e.g. user mail is not editable by everyone who may edit the user account... Of course we could use the hook for that, but having it in the field class would be a way better fit. So having the easy way to specify default access I think we should keep.
However, I like how the patch moves out the hook-logic of the field classes. Doing that + just keeping the default access in the field classes sounds good to me. Of course, then we have to move the default access method to an interface though.
The changes to the hook itself make sense to me as well!
Comment #15
yched CreditAttribution: yched commentedCool :-)
True, $field->defaultAccess() is now called from external code (EntityManager) rather then from the $field itself.
Attached patch moves it to FieldInterface.
Patch still requires we get #2047229: Make use of classes for entity field and data definitions in first, though.
Comment #16
yched CreditAttribution: yched commented+ @deprecated field_access() would move to field.deprecated.inc
Comment #17
andypostRelated #2062501: Remove calls to deprecated global $user in field module
Please use Drupal::currentUser() or service injection see #2062151: Create a current user service to ensure that current account is always available
Comment #18
yched CreditAttribution: yched commented@andypost: this code is just moved around, and the EntityAccessController class currently has other calls to the global $user, so I'll just leave it that way in this issue.
Comment #19
yched CreditAttribution: yched commented- Adds fieldAccess() to EntityAccessControllerInterface
- removes hacks in FormatterBase & WidgetBase (the @todos were pointing to this issue)
- converted existing calls to field_access()
Now all we need is #2047229: Make use of classes for entity field and data definitions :-)
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedInstead of waiting on #2047229: Make use of classes for entity field and data definitions, is it possible to add the parts from there that this patch needs into this patch?
Comment #21
effulgentsia CreditAttribution: effulgentsia commented@yched: Would simply adding the patch from #2047229-17: Make use of classes for entity field and data definitions into here be sufficient, or is there something else from that issue you need in here?
Comment #22
BerdirRe-roll and added FieldDefinition. Expecting this to explode completely, haven't looked at the merge at all yet nor FieldDefinition, actually.
Comment #24
BerdirFixed a merge mistake and added some missing methods that were previously defined on the parent class.
Comment #26
BerdirThis should fix the test failures.
Comment #28
BerdirFixed entity_test_entity_field_access_alter() for realz.
Comment #29
fagoGiven the field config is still call field this could be confusing. Should we clarify $field to be the actual field items?
Same here. Also, I'm wondering how this could play with #1869574: Support single valued Entity fields, i.e. we won't necessarily have $items then... But that's probably more stuff for the other issue.
There are some left-over references to that class, some of those were invalid from the initial patch.
Is that constant part of the API? Then it should be moved to the interface. Not necessarily stuff for this issue though.
Overridden does not really work for interface docs.
While this is referenced from new definitions, I don't think it's picked up already. Not sure it makes sense to add it already then?
Comment #30
yched CreditAttribution: yched commentedSide note: opened #2097691: Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface for #29.4
Comment #31
fgmRerolled as per #29. Not touching constant as per #30.
Comment #32
fgmRerolled as per #29. Not touching constant as per #30.
Comment #33
yched CreditAttribution: yched commentedLooks like those are never called in the current patch - can we punt on that and only add it later if we find it's needed ?
same (+ patch adds isFieldQueryable() & isFieldConfigurable() to FieldDefinitionInterface, but it doesn't really seem needed for now ?)
Not sure where those are needed either ?
Looks weird, why don't we put the field_name in the $definition that gets passed to the constructor ?
Comment #34
BerdirRemoved the old style definition stuff, item_definition, queryable/computed and added some unit tests.
Let's see what I messed up.
Comment #35
fagoAs discussed, let's move on with this asap and re-visit the field definition class details over at #2047229: Make use of classes for entity field and data definitions.
Comment mismatch.
Another mismatch.
Not sure why does do not got setters, but we can take care of it in the other issue also. Then, they should become todos here also though.
Comment #36
fago>Not sure why does do not got setters, ..
hm, not sure what I started here: configurable fields without FieldConfig entities? ;-) Maybe configurable could be a valid exception of being not writable, required should be though.
Comment #37
BerdirThanks, yes, added a setter for required, configurable is always FALSE, so not required, at least not until we introduce a different notion of configurable and then we can change it.
Fixed the docblocks.
Comment #38
smiletrl CreditAttribution: smiletrl commentedImpressive work!
Some nitpicks:
I think we could do something like this:
If default field access is denied, then no need to implement the access hook anymore, which may improve performance a bit?
This part looks a bit verbose to me:) Modules returned from
->getImplementations
have already been ensured to implement this hook, i.e., 'entity_field_access'.module_invoke
will call\Drupal::moduleHandler()->invoke()
to check the existence of hook implementation for each $module again. Why don't we simply call these implemented functions directly?AS for
This looks pretty clean to me, but drupal_alter() has been marked as deprecated. Maybe we could remove the sign of '@deprecated', or we have to rewrite
\Drupal::moduleHandler()->alter
to relace drupal_alter() here?Comment #39
smiletrl CreditAttribution: smiletrl commentedHere, we may need do
this->moduleHandler
will be set when this access controller is instantiated in entityManager.Comment #40
Berdir- I think the default handling is fine, there might be use cases for altering a FALSE there, who knows.
- Switched to use the injected module handler. I think continue to use module_invoke() there is the correct approach, module implementations are cached, we want to avoid fatal errors if someone removes a function I think.
Comment #41
fagoLeft over.
Left-over.
Besides that patch looks good, but could need an issue summary.
Comment #42
yched CreditAttribution: yched commentedl love the simplifications that were done after #34 !
#40 patch is RTBC as far as I'm concerned, but I wrote part of it so we'll need someone to push the button :-)
Comment #43
yched CreditAttribution: yched commentedcrosspost
Comment #43.0
yched CreditAttribution: yched commentedbla
Comment #44
yched CreditAttribution: yched commentedUpdated the summary
Comment #45
andypostseems fields should always have default_value initialized
Comment #46
smiletrl CreditAttribution: smiletrl commentedRe#40 -- @berdir Yeah, right, thanks for clearing that. Current way is the appropriate one. Especially for the second one,
ModuleHandler::invoke() need to load functions from inc files, which means calling functions directly may lead to fator error.
For doc of hook_entity_field_access
I guess, it should say 'invoked from \Drupal\Core\Entity\EntityAccessController::fieldAccess()'...
Since #2051923: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList is in, this patch needs a reroll^
Comment #47
smiletrl CreditAttribution: smiletrl commentedRe #45, @andypost, it's ok to leave default_value un-initialized:) If this setting is not available,
getFieldSetting
will take care of it and return NULL.Comment #48
BerdirRe-rolled after that issue went in. Did a few renames around $field/$items, also updates the reference, so please review the whole thing again.
Comment #49
BerdirThat was the wrong patch. This is better, also removed the isFieldQueryable() from Field and FieldInstace.
Comment #50
BerdirAnother re-roll.
Comment #52
BerdirThe $items rename was not complete.
Comment #54
BerdirThat was weird.
Comment #55
yched CreditAttribution: yched commentedStill, looks good to me, but we still need someone other than me pushing the RTBC button :-)
Comment #56
fagoberdir, I think you uploaded the wrong patch?
update: arg, I should not review using pages loaded a while ago.
Comment #57
fagoSo reviewed the recent patch:
As this map to the access() method of FieldInterface, the operations should be compatible. Thus, instead of 'edit' it should go with 'update' over there.
Probably it would be better to refer to the entityaccesscontroller method instead, as that docs are written specifically for field access.
Missing trailing point.
Exceeds 80 chars.
Comment #58
BerdirFixed those things.
Comment #59
smiletrl CreditAttribution: smiletrl commentedOne minor change:)
Two more questions:
1).
This seems right, but the truth is we've used "edit" everywhere in this patch, e.g,
'#access' => $items->access('edit')
. I guess 'update' is not suitable in this context?2).
Both entrance(setter) and exit(getter) for FieldType doesn't involve with the prefix "field_item". In other words, this prefix is agnostic to outside world of the definition class. Any particular reason to add/remove this prefix inside the class?
Also, since this is called "getFieldType/setFieldType" of FieldDefinition, I guess one of the implications for this context is this is about field_item type, not Entity type, so prefix "field_item" seems unnecessary to me:) If we are in the context of typed
data, then this prefix seems reasonable.
Comment #60
Berdir1. Thanks, nice find. Changed to update. Are there any other changes that we need to document? We'll also need to make sure to document all the related changes here properly. There might be an existing change notice about the field access stuff that we should update then.
2. Yes, we are setting the type, which is the typed data type. This is a preparation for this class to also be used as a typed data definition and will allow us to remove it again when we switch to instantiate field items through the field type plugin manager.
Comment #61
smiletrl CreditAttribution: smiletrl commented'Update' these too?
Comment #63
yched CreditAttribution: yched commented- 'edit' / 'update' : Hmm, field_access('edit') was used on widgets and worked for both entity creation and entity update forms. Using 'update' instead for those two contexts seems a bit off / misleading ?
- get/setFieldType(): I think @fago was considering that TypedDataDefinitionInterface would have a separate getDataType() method, and that getFieldType() would stay about the @FieldType plugin_id - which makes much sense IMO. Different facets of the same object, different ids, different methods.
Comment #64
BerdirOk, back to edit as discussed.
Also, this is blocking multiple criticals, so this should be critical too.
Comment #65
fagoYep, let's figure out 'edit' vs 'update' in a follow-up and move on with this asap.
Comment #66
BerdirSome more tags for rocketship.
Comment #67
alexpottNice work - I'm liking how configurable fields and non configurable fields are being treated the same.
Committed 691a576 and pushed to 8.x. Thanks!
Comment #68
yched CreditAttribution: yched commentedCreated https://drupal.org/node/2101719
Comment #69
smiletrl CreditAttribution: smiletrl commentedThe change record looks pretty good:)
Comment #70
smiletrl CreditAttribution: smiletrl commentedupdate title
Comment #71
smiletrl CreditAttribution: smiletrl commentedOops, I could have crosspost with myself!
Comment #72
jibranMarking fixed.
Comment #73
Wim LeersYay! Loving this clean-up in edit.module :)
Comment #74
BerdirRemove sprint tag.
Comment #75.0
(not verified) CreditAttribution: commentedpatch summary