Document that EntityAccessControlHandler::checkFieldAccess returns AccessResultAllowed by default

Function definition of EntityAccessControlHandler::checkFieldAccess was updated in https://www.drupal.org/project/drupal/issues/3395404.

Some additinal checks have been added for revision log field

This means that all fields are editable if the user has 'update' access to the entity.

Is this valid now? Update access is checked only for the revision log field in the new definition.

  protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
    if (!$items instanceof FieldItemListInterface || $operation !== 'view') {
      return AccessResult::allowed();
    }
    $entity = $items->getEntity();
    $isRevisionLogField = $this->entityType instanceof ContentEntityTypeInterface && $field_definition->getName() === $this->entityType->getRevisionMetadataKey('revision_log_message');
    if ($entity && $isRevisionLogField) {
      // The revision log should only be visible to those who can view the
      // revisions OR edit the entity.
      return $entity->access('view revision', $account, TRUE)
        ->orIf($entity->access('update', $account, TRUE));
    }
    return AccessResult::allowed();
  }

Is this valid now?

yes it is. Let me emphasize a part of the code you pasted above:

    if ($operation !== 'view') {
      return AccessResult::allowed();
    }

What needs to be documented here is you can not rely on fieldAccess alone to check whether a field is accessible by a user, it must be andIf'd with entity access.

Thanks @larowlan and @Ghost of Drupal Past

I have updated function documentation as per the inputs.

Can it be documented in the issue summary why

\Drupal\Core\Field\FieldItemList::access

does not need to be updated.

Updated the issue summary as suggested.

Left a small comment.

Hi,

I've added a minor change in the documentation following what I understood by @smustgrave's comment. Let me know if I'm wrong.

Thanks

Feedback has been addressed. I did rebase the MR as it was 3200+ commits back. Couldn't figure out why the pipeline looked old lol.

I was on the fence about committing this as I think the comment could be stronger in warning about the consequences. And even an example recommended usage would be helpful. But since this change does point out the issue it is better to have this in than wait. I trust that someone will open an issue if they think the comment should be improved.

Also, wrapped to 80- columns on commit.

Committed to 11.x and 11.2.x

Thanks!