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

CommentFileSizeAuthor
#64 unify_field_access-1994140-64.patch43.6 KBBerdir
#64 unify_field_access-1994140-64-interdiff.txt2.21 KBBerdir
#60 unify_field_access-1994140-60.patch43.61 KBBerdir
#60 unify_field_access-1994140-60-interdiff.txt680 bytesBerdir
#59 unify_field_access-1994140-59.patch43.6 KBsmiletrl
#59 inderdiff-58-59.txt554 bytessmiletrl
#58 1994140-unify_field_access-58.patch43.61 KBBerdir
#58 1994140-unify_field_access-58-interdiff.txt2.43 KBBerdir
#54 1994140-unify_field_access-52.patch43.59 KBBerdir
#54 1994140-unify_field_access-52-interdiff.txt1.99 KBBerdir
#52 1994140-unify_field_access-50.patch43.59 KBBerdir
#52 1994140-unify_field_access-52.patch1.99 KBBerdir
#50 1994140-unify_field_access-50.patch43.59 KBBerdir
#49 1994140-unify_field_access-49.patch43.58 KBBerdir
#49 1994140-unify_field_access-49-interdiff.txt1.64 KBBerdir
#48 entity_access-remove-langcode-param-2072945-26.patch46.63 KBBerdir
#48 entity_access-remove-langcode-param-2072945-26-interdiff.txt4.87 KBBerdir
#40 1994140-unify_field_access-37.patch42.84 KBBerdir
#40 1994140-unify_field_access-37-interdiff.txt2.14 KBBerdir
#37 1994140-unify_field_access-37.patch42.84 KBBerdir
#37 1994140-unify_field_access-37-interdiff.txt1.94 KBBerdir
#34 1994140-unify_field_access-34.patch42.28 KBBerdir
#34 1994140-unify_field_access-34-interdiff.txt11.99 KBBerdir
#31 1994140-unify_field_access.patch39.52 KBfgm
#31 interdiff.txt6.69 KBfgm
#28 field_access_unify-1994140-28.patch40.52 KBBerdir
#28 field_access_unify-1994140-28-interdiff.txt879 bytesBerdir
#26 field_access_unify-1994140-26.patch40.43 KBBerdir
#26 field_access_unify-1994140-26-interdiff.txt1.32 KBBerdir
#24 field_access_unify-1994140-24.patch39.99 KBBerdir
#24 field_access_unify-1994140-24-interdiff.txt3.01 KBBerdir
#22 field_access_unify-1994140-22.patch38.41 KBBerdir
#19 field_access_unify-1994140-19.patch29.2 KByched
#19 interdiff.txt11.54 KByched
#16 field_access_unify-1994140-16.patch15.44 KByched
#16 interdiff.txt2.9 KByched
#15 field_access_unify-1994140-15.patch13.67 KByched
#15 interdiff.txt2.21 KByched
#12 field_access_unify-1994140-11.patch12.16 KByched
#12 interdiff.txt1.77 KByched
#9 field_access_unify-1994140-9.patch12.15 KByched
#9 interdiff.txt672 bytesyched
#7 field_access_unify-1994140-7.patch12.1 KByched
#7 interdiff.txt951 bytesyched
#5 field_access_unify-1994140-5.patch12.1 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Shortly 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.

function hook_field_access($op, \Drupal\field\FieldInterface $field, $entity_type, $entity, $account) {

=>

function hook_entity_field_access($operation, $field, \Drupal\Core\Session\AccountInterface $account) {
$instance = $field->getInstance();
$entity = $field->getParent();

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.

yched’s picture

Priority: Normal » Major
Issue tags: +API change, +Field API

Updating 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.

yched’s picture

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?

Yes, 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 ?

yched’s picture

If 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) ?

yched’s picture

Status: Active » Needs review
FileSize
12.1 KB

Could look something like the attached patch.
Puts the fieldAccess() method in EntityAccessController rather than directly in the EntityManager, though.

yched’s picture

Issue summary: View changes

detailed summary

yched’s picture

Issue summary: View changes

todos

yched’s picture

Issue summary: View changes

formatting

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-5.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
951 bytes
12.1 KB

Bleh, wrong order of arguments

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-7.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
672 bytes
12.15 KB

And missing "use" statement.

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-9.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Hook 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...

yched’s picture

Forgot updated patch in #11.

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-11.patch, failed testing.

fago’s picture

Also, I don't really see the ability to let field type subclasses override the access() logic as a really important feature ?

We 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!

yched’s picture

Cool :-)

Of course, then we have to move the default access method to an interface though.

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.

yched’s picture

+ @deprecated field_access() would move to field.deprecated.inc

andypost’s picture

Related #2062501: Remove calls to deprecated global $user in field module

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
@@ -244,4 +246,45 @@ protected function prepareUser(AccountInterface $account = NULL) {
+    global $user;
...
+      $account = $user;

Please use Drupal::currentUser() or service injection see #2062151: Create a current user service to ensure that current account is always available

yched’s picture

@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.

yched’s picture

- 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 :-)

effulgentsia’s picture

Instead 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?

effulgentsia’s picture

@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?

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.41 KB

Re-roll and added FieldDefinition. Expecting this to explode completely, haven't looked at the merge at all yet nor FieldDefinition, actually.

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.01 KB
39.99 KB

Fixed a merge mistake and added some missing methods that were previously defined on the parent class.

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
40.43 KB

This should fix the test failures.

Status: Needs review » Needs work

The last submitted patch, field_access_unify-1994140-26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
879 bytes
40.52 KB

Fixed entity_test_entity_field_access_alter() for realz.

fago’s picture

Status: Needs review » Needs work
+++ b/core/includes/entity.api.php
@@ -710,17 +710,20 @@ function hook_entity_operation_alter(array &$operations, \Drupal\Core\Entity\Ent
+ * @param \Drupal\Core\Entity\Field\FieldDefinitionInterface $field_definition
+ *   The field definition
  * @param \Drupal\Core\Session\AccountInterface $account
  *   The user account to check.
+ * @param \Drupal\Core\Entity\Field\FieldInterface $field
+ *   (optional) The entity field object on which the operation is to be

Given the field config is still call field this could be confusing. Should we clarify $field to be the actual field items?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -70,4 +72,22 @@ public function resetCache();
+   * @param \Drupal\Core\Entity\Field\FieldInterface $field
+   *   (optional) The field values for which to check access, or NULL if access
+   *    is checked for the field definition, without any specific value

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.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
@@ -0,0 +1,257 @@
+use Drupal\Core\TypedData\DataDefinition;

There are some left-over references to that class, some of those were invalid from the initial patch.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
@@ -0,0 +1,257 @@
+  public function getFieldCardinality() {
+    // @todo: Support reading out a possible cardinality constraint?
+    return $this->isList() ? FIELD_CARDINALITY_UNLIMITED : 1;

Is that constant part of the API? Then it should be moved to the interface. Not necessarily stuff for this issue though.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldInterface.php
@@ -60,6 +61,18 @@ public function getLangcode();
+   * See \Drupal\Core\TypedData\AccessibleInterface::access() for the parameter
+   * doucmentation. This method can be overriden by field sub classes to provide

Overridden does not really work for interface docs.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Field.php
@@ -0,0 +1,27 @@
+ * @DataType(
+ *   id = "entity_field",
+ *   label = @Translation("Entity field"),

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?

yched’s picture

fgm’s picture

Rerolled as per #29. Not touching constant as per #30.

fgm’s picture

Status: Needs work » Needs review

Rerolled as per #29. Not touching constant as per #30.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,257 @@
    +  public function setFieldType($type) {
    +  public function setTranslatable($translatable) {
    +  public function setPropertyConstraints($name, array $constraints) {
    

    Looks 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 ?

  2. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,257 @@
    +  public function setQueryable($queryable) {
    

    same (+ patch adds isFieldQueryable() & isFieldConfigurable() to FieldDefinitionInterface, but it doesn't really seem needed for now ?)

  3. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,257 @@
    +  public function getItemDefinition() {
    ...
    +  public function setItemDefinition(FieldDefinition $item_definition = NULL) {
    ...
    +    $new->setItemDefinition(new FieldDefinition($definition));
    

    Not sure where those are needed either ?

  4. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,257 @@
    +    $new->setFieldName($field_name);
    

    Looks weird, why don't we put the field_name in the $definition that gets passed to the constructor ?

Berdir’s picture

Removed the old style definition stuff, item_definition, queryable/computed and added some unit tests.

Let's see what I messed up.

fago’s picture

Status: Needs review » Needs work

As 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.

+++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
@@ -0,0 +1,131 @@
+   * Tests field label methods.
+   */
+  public function testFieldDescription() {

Comment mismatch.

+++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
@@ -0,0 +1,131 @@
+   * Tests field settings methods.
+   */
+  public function testFieldDefaultValue() {

Another mismatch.

+++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
@@ -0,0 +1,131 @@
+  /**
+   * Tests required.
+   */
+  public function testFieldRequired() {
+    $definition = new FieldDefinition();
+    $this->assertFalse($definition->isFieldRequired());
+  }
+
+  /**
+   * Tests configurable.
+   */
+  public function testFieldConfigurable() {
+    $definition = new FieldDefinition();
+    $this->assertFalse($definition->isFieldConfigurable());

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.

fago’s picture

>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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
42.84 KB

Thanks, 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.

smiletrl’s picture

Impressive work!

Some nitpicks:

+    // Get the default access restriction that lives within this field.
+    $default = $field ? $field->defaultAccess($operation, $account) : TRUE;
+
+    // Invoke hook and collect grants/denies for field access from other
+    // modules. Our default access flag is masked under the ':default' key.
+    $grants = array(':default' => $default);

I think we could do something like this:

  // Get the default access restriction that lives within this field.
  $default = $field ? $field->defaultAccess($operation, $account) : TRUE;
 
  if (!default) {
    return FALSE;
  }

  // Invoke hook and collect grants/denies for field access from other
  // modules.
  $grants = array();

If default field access is denied, then no need to implement the access hook anymore, which may improve performance a bit?

+    $hook_implementations = \Drupal::moduleHandler()->getImplementations('entity_field_access');
+    foreach ($hook_implementations as $module) {
+      $grants = array_merge($grants, array($module => module_invoke($module, 'entity_field_access', $operation, $field_definition, $account, $field)));
+    }

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?

$hook_implementations = \Drupal::moduleHandler()->getImplementations('entity_field_access');
foreach ($hook_implementations as $module) {
  $function = $module . '_entity_field_access';
  $grants = array_merge($grants, array($module => $function($operation, $field_definition, $account, $field)));
}

AS for

+    drupal_alter('entity_field_access', $grants, $context);

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?

smiletrl’s picture

+    $hook_implementations = \Drupal::moduleHandler()->getImplementations('entity_field_access');
+    foreach ($hook_implementations as $module) {

Here, we may need do

$hook_implementations = this->moduleHandler->getImplementations('entity_field_access');

this->moduleHandler will be set when this access controller is instantiated in entityManager.

Berdir’s picture

- 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.

fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
@@ -718,4 +718,17 @@ public function __wakeup() {
+  /**
+   * {@inheritdoc}
+   */
+  public function isFieldQueryable() {
+    return TRUE;

Left over.

+++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php
@@ -610,6 +610,20 @@ public function allowBundleRename() {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function isFieldQueryable() {
+    return TRUE;

Left-over.

Besides that patch looks good, but could need an issue summary.

yched’s picture

Status: Needs work » Needs review

l 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 :-)

yched’s picture

Status: Needs review » Needs work

crosspost

yched’s picture

Issue summary: View changes

bla

yched’s picture

Updated the summary

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
@@ -0,0 +1,234 @@
+  public function getFieldDefaultValue(EntityInterface $entity) {
+    return $this->getFieldSetting('default_value');

seems fields should always have default_value initialized

smiletrl’s picture

Re#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

This hook is invoked from \Drupal\Core\Entity\Field\FieldItemListInterface::access() to let modules grant or deny operations on fields.

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^

smiletrl’s picture

Re #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.

Berdir’s picture

Re-rolled after that issue went in. Did a few renames around $field/$items, also updates the reference, so please review the whole thing again.

Berdir’s picture

That was the wrong patch. This is better, also removed the isFieldQueryable() from Field and FieldInstace.

Berdir’s picture

Another re-roll.

Status: Needs review » Needs work

The last submitted patch, 1994140-unify_field_access-50.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
43.59 KB

The $items rename was not complete.

Status: Needs review » Needs work

The last submitted patch, 1994140-unify_field_access-52.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
43.59 KB

That was weird.

yched’s picture

Still, looks good to me, but we still need someone other than me pushing the RTBC button :-)

fago’s picture

Status: Needs review » Needs work

berdir, I think you uploaded the wrong patch?
update: arg, I should not review using pages loaded a while ago.

fago’s picture

So reviewed the recent patch:

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.php
@@ -70,4 +72,22 @@ public function resetCache();
+   *   The operation access should be checked for.
+   *   Usually one of "view" or "edit".

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.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldItemListInterface.php
@@ -60,6 +61,17 @@ public function getLangcode();
+   * See \Drupal\Core\TypedData\AccessibleInterface::access() for the parameter
+   * doucmentation.
+   *
+   * @return bool
+   *   TRUE if access to this field is allowed per default, FALSE otherwise.
+   */

Probably it would be better to refer to the entityaccesscontroller method instead, as that docs are written specifically for field access.

+++ b/core/modules/system/entity.api.php
@@ -705,23 +705,25 @@ function hook_entity_operation_alter(array &$operations, \Drupal\Core\Entity\Ent
+ * @param \Drupal\Core\Entity\Field\FieldDefinitionInterface $field_definition

Missing trailing point.

+++ b/core/modules/system/entity.api.php
@@ -705,23 +705,25 @@ function hook_entity_operation_alter(array &$operations, \Drupal\Core\Entity\Ent
+ *   (optional) The entity field object on which the operation is to be performed.

Exceeds 80 chars.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
43.61 KB

Fixed those things.

smiletrl’s picture

One minor change:)

Two more questions:
1).

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.

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).

+
+  /**
+   * {@inheritdoc}
+   */
+  public function getFieldType() {
+    // Cut of the leading field_item: prefix from 'field_item:FIELD_TYPE'.
+    $parts = explode(':', $this->definition['type']);
+    return $parts[1];
+  }
+
+  /**
+   * Sets the field type.
+   *
+   * @param string $type
+   *   The field type to set.
+   *
+   * @return \Drupal\Core\Entity\Field\FieldDefinition
+   *   The object itself for chaining.
+   */
+  public function setFieldType($type) {
+    $this->definition['type'] = 'field_item:' . $type;
+    return $this;
+  }
+

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.

Berdir’s picture

1. 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.

smiletrl’s picture

+    return $entity->access('update') && $entity->get($field_name)->access('edit');
+    if ($field['type'] != 'entity_reference' || !$access_controller->fieldAccess('edit', $instance)) {

'Update' these too?

Status: Needs review » Needs work

The last submitted patch, unify_field_access-1994140-60.patch, failed testing.

yched’s picture

- '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.

Berdir’s picture

Priority: Major » Critical
Status: Needs work » Needs review
FileSize
2.21 KB
43.6 KB

Ok, back to edit as discussed.

Also, this is blocking multiple criticals, so this should be critical too.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Yep, let's figure out 'edit' vs 'update' in a follow-up and move on with this asap.

Berdir’s picture

Issue tags: +sprint, +Entity Access

Some more tags for rocketship.

alexpott’s picture

Title: Unify entity field access and Field API access » Change notice: Unify entity field access and Field API access
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Approved API change

Nice work - I'm liking how configurable fields and non configurable fields are being treated the same.

Committed 691a576 and pushed to 8.x. Thanks!

yched’s picture

smiletrl’s picture

Status: Active » Closed (fixed)
Issue tags: -Needs change record

The change record looks pretty good:)

smiletrl’s picture

Title: Change notice: Unify entity field access and Field API access » Unify entity field access and Field API access
Issue tags: +Needs change record

update title

smiletrl’s picture

Issue tags: -Needs change record

Oops, I could have crosspost with myself!

jibran’s picture

Status: Closed (fixed) » Fixed

Marking fixed.

Wim Leers’s picture

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php
@@ -71,8 +71,7 @@ public function access(Route $route, Request $request) {
-    $entity_type = $entity->entityType();
-    return $entity->access('update') && ($field = $this->fieldInfo->getField($entity_type, $field_name)) && field_access('edit', $field, $entity_type, $entity);
+    return $entity->access('update') && $entity->get($field_name)->access('edit');

Yay! Loving this clean-up in edit.module :)

Berdir’s picture

Issue tags: -sprint

Remove sprint tag.

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

Anonymous’s picture

Issue summary: View changes

patch summary