Make QuickEditEntityFieldAccessCheck::access() use the $account that's passed in

#3 will apply once #2287071: Add cacheability metadata to access checks has landed. Postponing on #2287071: Add cacheability metadata to access checks.

Rerolled.

Oops, I lost the changes to the interface in my reroll.

Thanks @nod_ for notifying me of that in IRC.

all good now.

In the patch above I stupidly copied the changes from #3, without taking into account the additional parameters that this specified. Brought those back, that hugely diminishes the number of failures.

And to pass the current user to the access check, some other things need to be updated as well.

#2267621: Deprecate and rename EditEntityFieldAccessCheck now that the module is named Quick Edit is in, this needs a reroll.

Here is the patch, that i rerolled.

#24 Please provide an interdiff. See https://www.drupal.org/documentation/git/interdiff.

Attaching interdiff between 15 and 24.

#2267621: Deprecate and rename EditEntityFieldAccessCheck now that the module is named Quick Edit was only committed to 8.4.x. This issue was still on 8.3.x. The patch in #24 hence does not apply to 8.4.x.

Sorry for not noticing that sooner.

Upgraded #24 against 8.4.x.

+++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
@@ -22,28 +22,26 @@ class QuickEditEntityFieldAccessCheck implements AccessInterface, QuickEditEntit
+

Nit: Adding this newline is not necessary.

RTBC, because that silly nit can be fixed upon commit. Or not be fixed at all.

Thanks!

1. +++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
   @@ -22,28 +22,26 @@ class QuickEditEntityFieldAccessCheck implements AccessInterface, QuickEditEntit
   -  /**
   -   * {@inheritdoc}
   -   */
   

   Why is this removed? We need some documentation - I think it shouldn't be removed.

2. +++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
   @@ -22,28 +22,26 @@ class QuickEditEntityFieldAccessCheck implements AccessInterface, QuickEditEntit
   +  public function accessEditEntityField(EntityInterface $entity, $field_name, AccountInterface $account) {
   

   Adding a new parameter with no default is a BC break - are we sure this is ok?

3. +++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheckInterface.php
   @@ -16,10 +17,12 @@
   +  public function accessEditEntityField(EntityInterface $entity, $field_name, AccountInterface $account);
   

   Shouldn't we implement this in a BC compatible manor and fallback to the current user and default to NULL. We can only add a required argument to an interface like this in D9.

Two things:

1. This is only ever called by Quick Edit. So this is only breaking BC for the likely zero contrib/custom modules that are completely swapping the default implementation.
2. This should never have had an interface. This dates back to the days that we were adding an interface for every single thing.

Hence I'm asking: do we need to be this strict for something like this? Something that A) should have been marked @internal but isn't, and B) is extremely unlikely to be used by anything else?

Don't get me wrong: I agree with everything you said in principle. But in practice, it makes no difference.

Briefly discussed with @alexpott in IRC. We agreed on this:

The same rules must apply to everything, to Keep It Simple, Stupid. Exceptions are really problematic and if they exist need documenting.

So, @aries, you'll need to make sure to not require the new parameter to be specified. It'll need to be optional.

Sorry for the delay, let me know if I missed something.

@wim-leers: I removed the unnecessary extra line.

The default value was missing in the MetadataGenerator's constructor.

- In the unit-tests, do we need to keep the test cases against the old signatures along with the new ones?
- Shall I add a @TODO to remove the default value in 8.5.x?

Method documentation update - $current_user is optional.

Method MockQuickEditEntityFieldAccessCheck::accessEditEntityField() 's signature amended to match with the interface.

Could you provide interdiffs in the future? See https://www.drupal.org/documentation/git/interdiff.

+++ b/core/modules/quickedit/src/MetadataGenerator.php
@@ -67,7 +78,7 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
+    $access = $this->accessChecker->accessEditEntityField($entity, $field_name, $this->currentUser);

$this->currentUser might be NULL.

If that's the case, we need to retrieve the current user service from the container. Otherwise we're still breaking BC.

I've added this

if ($this->currentUser == NULL) {
  $this->currentUser = Drupal::currentUser();
}

before

  // Early-return if user does not have access.
  $access = $this->accessChecker->accessEditEntityField($entity, $field_name, $this->currentUser);
  if (!$access) {
    return ['access' => FALSE];
  }

Please provide an interdiff: https://www.drupal.org/documentation/git/interdiff

Reg #43:

$this->currentUser might be NULL.

If that's the case, we need to retrieve the current user service from the container. Otherwise we're still breaking BC.

@current_user injected in the service as an argument in services.yml file. If it is empty eventually it's going to be empty in container as well. Am I missing something here?
@current_user in service.yml == Drupal::currentUser() right?

1. +++ b/core/modules/quickedit/src/MetadataGenerator.php
   @@ -66,8 +78,12 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
   +    if ($this->currentUser == NULL) {
   

   Should be ===.

2. +++ b/core/modules/quickedit/src/MetadataGenerator.php
   @@ -66,8 +78,12 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
   +      $this->currentUser = Drupal::currentUser();
   

   Should have a leading backslash.

But those are trivial fixes that can be made upon commit.

Thanks all!

Here is an updated Patch which fixed the comments mentioned in #47.

@Dinesh18: Thanks, but please also upload an interdiff then. Otherwise I have to manually compare every single change. See https://www.drupal.org/documentation/git/interdiff.

Thanks @Wim Leers
I will provide the interdiff.

Here is the interdiffs and the patch

ignore comment #51. patch name written is wrong.
Here is an updated one.

Thanks, @Dinesh18!

+++ b/core/modules/quickedit/src/MetadataGenerator.php
@@ -2,11 +2,13 @@
+use Drupal;

We normally use \Drupal instead of adding a use statement for classes in the global namespace

Oops, I missed that. Thanks. #44 introduced that. This is why we want interdiffs!

Here is an updated patch and interdiff which implements #54

Sorry for not being more clear: we don't need that line! Can you just remove it altogether? Thanks :)

No issues. I am new to Drupal 8. By the way #54 states clearly that we should not use \Drupal instead we should use it as a global namespace. Hope I am correct :)
Here is an updated patch with interdiff

Perfect, thank you!

Updating issue credit, giving @alexpott credit for picking up the BC break and @vijaycs85 for picking up the question in #46

+++ b/core/modules/editor/tests/src/Kernel/QuickEditIntegrationTest.php
index 3d7d934378..d4830b4d39 100644
--- a/core/modules/quickedit/quickedit.services.yml

+++ b/core/modules/quickedit/src/MetadataGenerator.php
@@ -66,8 +77,12 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
+    if ($this->currentUser === NULL) {
+      $this->currentUser = \Drupal::currentUser();
+    }

I agree with @vijaycs85 in #46- in what circumstances would this be NULL?

#63:

1. +++ b/core/modules/quickedit/src/MetadataGenerator.php
   @@ -43,11 +51,14 @@ class MetadataGenerator implements MetadataGeneratorInterface {
   +   * @param \Drupal\Core\Session\AccountInterface $current_user
   +   *   (optional) The current user.
   
   +++ b/core/modules/quickedit/tests/modules/src/MockQuickEditEntityFieldAccessCheck.php
   @@ -13,7 +14,7 @@ class MockQuickEditEntityFieldAccessCheck implements QuickEditEntityFieldAccessC
   -  public function accessEditEntityField(EntityInterface $entity, $field_name) {
   +  public function accessEditEntityField(EntityInterface $entity, $field_name, AccountInterface $account = NULL) {
   

   Because this is an optional new parameter. And therefore subclasses may not yet have.

2. +++ b/core/modules/quickedit/src/MetadataGenerator.php
   @@ -66,8 +77,12 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
   +    if ($this->currentUser === NULL) {
   +      $this->currentUser = \Drupal::currentUser();
   +    }
   +
        // Early-return if user does not have access.
   -    $access = $this->accessChecker->accessEditEntityField($entity, $field_name);
   +    $access = $this->accessChecker->accessEditEntityField($entity, $field_name, $this->currentUser);
   

   OTOH, this is just ensuring there is a currentUser service to pass to ::accessEditEntityField() which itself already allows NULL to be passed for similar BC reasons.

   So I agree that this if-test is pointless and can be removed.

As suggested by Wim and larowlan, removing if test.

Thanks Wim :)

Thank you, @snehi! :)

+++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
@@ -22,26 +22,26 @@ class QuickEditEntityFieldAccessCheck implements AccessInterface, QuickEditEntit
-   *   The currently logged in account.
+   *   (optional) The user for which to check access.
...
-  public function access(EntityInterface $entity, $field_name, $langcode, AccountInterface $account) {
+  public function access(EntityInterface $entity, $field_name, $langcode, AccountInterface $account = NULL) {

I don't understand why the argument here is becoming optional? This seems to have occurred in #35 as a result of mine & @Wim Leers comments in #32, #33, #34 - but I was referring to the new param on accessEditEntityField() not the existing param on access().

@Wim Leers what do you think?

Indeed, #35 introduced that, without an interdiff, and I failed to spot it. My bad :( You're right of course. Fixed.

Re-reviewed the entire patch now. One question for a core committer:

+++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheckInterface.php
@@ -16,10 +17,12 @@
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   (optional) The user for which to check access.
...
-  public function accessEditEntityField(EntityInterface $entity, $field_name);
+  public function accessEditEntityField(EntityInterface $entity, $field_name, AccountInterface $account = NULL);

This needs to become non-optional in the future. How do we communicate that?

Re #73 the best way would be to do something like this:

public function accessEditEntityField(EntityInterface $entity, $field_name, AccountInterface $account = NULL) {
  if (!$account) {
     @trigger_error('The $account parameter will be required in Drupal 9 etc...', E_USER_DEPRECATED);
  }
}

And add a legacy test for this. Doing this means that when we come to clean up the @trigger_errors in 9.x.x we'll be reminded to do this. This is only a good idea if we always pass the $account in in HEAD.

#74… except that this is an interface. So alternative implementations will actually not be made aware.

We should deprecate \Drupal\quickedit\Access\QuickEditEntityFieldAccessCheckInterface. There is no valid reason to ever override the default implementation, hence there is no valid reason for that interface to exist. You should be able to implement arbitrary access logic via the Entity Access API and/or Field Access API.

Once that interface is gone, #74 would work.

In other words: do what #74 says, but also deprecate that interface.

Agreed let's deprecate the interface (and also mark it @internal since we're not replacing it?).

By now we have much more experience with evolving existing APIs while retaining BC. When #74, #75 and #76 were written that was still kind of treading new ground, not anymore today! So let's get this done :)

Haven't really looked at this but ran into it through another issue and rerolled around #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib

ah, that's an easy fix.

Tried to fix custom command failure errors in #84, thanks!

Yeah, those errors are my fault. Tried a new way of merging patch failures and I messed it up.

Due to Quickedit being moved out of Drupal Core and into a Contrib Module, moving this issue to the Contrib Module queue.