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

Files: 
CommentFileSizeAuthor
#64 unify_field_access-1994140-64.patch43.6 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,100 pass(es).
[ View ]
#64 unify_field_access-1994140-64-interdiff.txt2.21 KBBerdir
#60 unify_field_access-1994140-60.patch43.61 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,306 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#60 unify_field_access-1994140-60-interdiff.txt680 bytesBerdir
#59 unify_field_access-1994140-59.patch43.6 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 59,191 pass(es).
[ View ]
#59 inderdiff-58-59.txt554 bytessmiletrl
#58 1994140-unify_field_access-58.patch43.61 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,288 pass(es).
[ View ]
#58 1994140-unify_field_access-58-interdiff.txt2.43 KBBerdir
#54 1994140-unify_field_access-52.patch43.59 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,112 pass(es).
[ View ]
#54 1994140-unify_field_access-52-interdiff.txt1.99 KBBerdir
#52 1994140-unify_field_access-50.patch43.59 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#52 1994140-unify_field_access-52.patch1.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1994140-unify_field_access-52.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#50 1994140-unify_field_access-50.patch43.59 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,211 pass(es), 2 fail(s), and 736 exception(s).
[ View ]
#49 1994140-unify_field_access-49.patch43.58 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1994140-unify_field_access-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 1994140-unify_field_access-49-interdiff.txt1.64 KBBerdir
#48 entity_access-remove-langcode-param-2072945-26.patch46.63 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,790 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#48 entity_access-remove-langcode-param-2072945-26-interdiff.txt4.87 KBBerdir
#40 1994140-unify_field_access-37.patch42.84 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]
#40 1994140-unify_field_access-37-interdiff.txt2.14 KBBerdir
#37 1994140-unify_field_access-37.patch42.84 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,208 pass(es).
[ View ]
#37 1994140-unify_field_access-37-interdiff.txt1.94 KBBerdir
#34 1994140-unify_field_access-34.patch42.28 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,673 pass(es).
[ View ]
#34 1994140-unify_field_access-34-interdiff.txt11.99 KBBerdir
#31 1994140-unify_field_access.patch39.52 KBfgm
PASSED: [[SimpleTest]]: [MySQL] 58,916 pass(es).
[ View ]
#31 interdiff.txt6.69 KBfgm
#28 field_access_unify-1994140-28.patch40.52 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es).
[ View ]
#28 field_access_unify-1994140-28-interdiff.txt879 bytesBerdir
#26 field_access_unify-1994140-26.patch40.43 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,454 pass(es), 23 fail(s), and 0 exception(s).
[ View ]
#26 field_access_unify-1994140-26-interdiff.txt1.32 KBBerdir
#24 field_access_unify-1994140-24.patch39.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,702 pass(es), 1 fail(s), and 149 exception(s).
[ View ]
#24 field_access_unify-1994140-24-interdiff.txt3.01 KBBerdir
#22 field_access_unify-1994140-22.patch38.41 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 field_access_unify-1994140-19.patch29.2 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_access_unify-1994140-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 interdiff.txt11.54 KByched
#16 field_access_unify-1994140-16.patch15.44 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_access_unify-1994140-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 interdiff.txt2.9 KByched
#15 field_access_unify-1994140-15.patch13.67 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_access_unify-1994140-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 interdiff.txt2.21 KByched
#12 field_access_unify-1994140-11.patch12.16 KByched
FAILED: [[SimpleTest]]: [MySQL] 57,937 pass(es), 22 fail(s), and 41 exception(s).
[ View ]
#12 interdiff.txt1.77 KByched
#9 field_access_unify-1994140-9.patch12.15 KByched
FAILED: [[SimpleTest]]: [MySQL] 58,053 pass(es), 21 fail(s), and 42 exception(s).
[ View ]
#9 interdiff.txt672 bytesyched
#7 field_access_unify-1994140-7.patch12.1 KByched
FAILED: [[SimpleTest]]: [MySQL] 57,610 pass(es), 48 fail(s), and 903 exception(s).
[ View ]
#7 interdiff.txt951 bytesyched
#5 field_access_unify-1994140-5.patch12.1 KByched
FAILED: [[SimpleTest]]: [MySQL] 58,067 pass(es), 59 fail(s), and 2,844 exception(s).
[ View ]

Comments

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.

<?php
function hook_field_access($op, \Drupal\field\FieldInterface $field, $entity_type, $entity, $account) {
?>

=>
<?php
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.

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.

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 ?

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

Status:Active» Needs review
StatusFileSize
new12.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,067 pass(es), 59 fail(s), and 2,844 exception(s).
[ View ]

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

Issue summary:View changes

detailed summary

Issue summary:View changes

todos

Issue summary:View changes

formatting

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new951 bytes
new12.1 KB
FAILED: [[SimpleTest]]: [MySQL] 57,610 pass(es), 48 fail(s), and 903 exception(s).
[ View ]

Bleh, wrong order of arguments

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new672 bytes
new12.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,053 pass(es), 21 fail(s), and 42 exception(s).
[ View ]

And missing "use" statement.

Status:Needs review» Needs work

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

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

StatusFileSize
new1.77 KB
new12.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,937 pass(es), 22 fail(s), and 41 exception(s).
[ View ]

Forgot updated patch in #11.

Status:Needs review» Needs work

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

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!

StatusFileSize
new2.21 KB
new13.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_access_unify-1994140-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new2.9 KB
new15.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_access_unify-1994140-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

StatusFileSize
new11.54 KB
new29.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_access_unify-1994140-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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?

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

Status:Needs work» Needs review
StatusFileSize
new38.41 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.01 KB
new39.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,702 pass(es), 1 fail(s), and 149 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
new40.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,454 pass(es), 23 fail(s), and 0 exception(s).
[ View ]

This should fix the test failures.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new879 bytes
new40.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es).
[ View ]

Fixed entity_test_entity_field_access_alter() for realz.

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

StatusFileSize
new6.69 KB
new39.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,916 pass(es).
[ View ]

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

Status:Needs work» Needs review

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

  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 ?

StatusFileSize
new11.99 KB
new42.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,673 pass(es).
[ View ]

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

Let's see what I messed up.

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.94 KB
new42.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,208 pass(es).
[ View ]

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.

Impressive work!

Some nitpicks:

<?php
+    // 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:
<?php
 
// 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?

<?php
+    $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?
<?php
$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

<?php
+    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?

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

Here, we may need do
<?php
$hook_implementations
= this->moduleHandler->getImplementations('entity_field_access');
?>

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

StatusFileSize
new2.14 KB
new42.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]

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

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.

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

Status:Needs review» Needs work

crosspost

Issue summary:View changes

bla

Updated the summary

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

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^

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.

Status:Needs work» Needs review
StatusFileSize
new4.87 KB
new46.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,790 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new1.64 KB
new43.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1994140-unify_field_access-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new43.59 KB
FAILED: [[SimpleTest]]: [MySQL] 59,211 pass(es), 2 fail(s), and 736 exception(s).
[ View ]

Another re-roll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1994140-unify_field_access-52.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new43.59 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

The $items rename was not complete.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
new43.59 KB
PASSED: [[SimpleTest]]: [MySQL] 59,112 pass(es).
[ View ]

That was weird.

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
new43.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,288 pass(es).
[ View ]

Fixed those things.

StatusFileSize
new554 bytes
new43.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,191 pass(es).
[ View ]

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

<?php
+
/**
+   * {@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.

StatusFileSize
new680 bytes
new43.61 KB
FAILED: [[SimpleTest]]: [MySQL] 59,306 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

<?php
+    return $entity->access('update') && $entity->get($field_name)->access('edit');
?>

<?php
+    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.

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

Priority:Major» Critical
Status:Needs work» Needs review
StatusFileSize
new2.21 KB
new43.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,100 pass(es).
[ View ]

Ok, back to edit as discussed.

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

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.

Issue tags:+sprint, +Entity Access

Some more tags for rocketship.

Title:Unify entity field access and Field API accessChange 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!

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

The change record looks pretty good:)

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

update title

Issue tags:-Needs change record

Oops, I could have crosspost with myself!

Status:Closed (fixed)» Fixed

Marking fixed.

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

Issue tags:-sprint

Remove sprint tag.

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

Issue summary:View changes

patch summary