Problem/Motivation

EditEntityFieldAccessCheck has a somewhat confusing name, especially now that the module has been renamed.

Proposed resolution

  • Rename EditEntityFieldAccessCheck to QuickEditEntityFieldAccessCheck.

Remaining tasks

API changes

  • EditEntityFieldAccessCheck renamed to QuickEditEntityFieldAccessCheck.

Postponed until

#2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes

CommentFileSizeAuthor
#90 interdiff-85-90.txt4.9 KBtameeshb
#90 2267621-90.patch15.24 KBtameeshb
#89 interdiff.txt630 byteshimanshu-dixit
#89 2267621-89.patch24.74 KBhimanshu-dixit
#85 2267621-85.patch15.07 KBtameeshb
#85 interdiff-81-85.txt11.45 KBtameeshb
#82 interdiff-77-81.txt11.48 KBtameeshb
#82 2267621-81.patch20.81 KBtameeshb
#78 2267621-77-w-renames.patch20.58 KBmpdonadio
#77 2267621-77.patch25.02 KBtameeshb
#77 interdiff-76-77.txt392 bytestameeshb
#76 2267621-76.patch25.05 KBtameeshb
#76 interdiff-70-76.txt6.14 KBtameeshb
#70 2267621-70.patch21.26 KBtameeshb
#70 interdiff-63-70.txt2.22 KBtameeshb
#67 Rename_EditEntityFieldAccessCheck_now_that_the_module_is_named_Quick_Edit-2267621-63.patch5.58 KBLal_
#67 1.txt12.22 KBLal_
#60 2267621-60.patch25.02 KBtameeshb
#60 interdiff-57-60.txt982 bytestameeshb
#57 2267621-57.patch24.92 KBtameeshb
#57 interdiff-56-57.txt376 bytestameeshb
#56 2267621-56.patch24.95 KBtameeshb
#56 interdiff-47-56.txt18.71 KBtameeshb
#49 QuickEdit2267621-48.patch15.12 KBchiranjeeb2410
#47 QuickEdit2267621-47.patch11.79 KBtameeshb
#44 QuickEdit2267621-44.patch11.79 KBtameeshb
#36 QuickEditEntityFieldAccess-2267621-36_.patch11.75 KBtameeshb
#31 QuickEditEntityFieldAccess-2267621-31.patch10.92 KBtameeshb
#16 2267621-16.patch12.14 KBWim Leers
#11 2267621-11.patch8.45 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
Wim Leers’s picture

No, those are named that way intentionally. It was confusing when this was still called edit.module, now it isn't confusing anymore — because this is not at all Quick Edit-specific! These are access checks for precisely what the file names say: whether the user has access to edit a particular entity, and whether the user has access to edit a particular field in a particular entity.

If anything, this should be moved out of quickedit.module and into Entity API.

Thoughts? :)

xjm’s picture

Hm. But the former duplicates part of the functionality of \Drupal\Core\Entity\EntityAccessCheck, no?

I can see how the field access check could be useful outside quickedit.

Wim Leers’s picture

I just looked into deleting Quick Edit's EditEntityAccessCheck, turns out EntityAccessCheck is useless for Quick Edit :(

EntityAccessCheck requires you to specify the entity type in the route definition (_entity_access: 'node.update'), but Quick Edit needs a generic mechanism, so it can work with any entity type.

The same goes for Quick Edit's EditEntityFieldAccessCheck.

If we had better parameter upcasting, we could generalize it, but I don't think that's currently possible. I'd love input from the Entity API/routing system guys to hear what they think — maybe they do see a way to make EntityAccessCheck generic :)

jessebeach’s picture

Status: Postponed » Active

Unpostponed!

gnuget’s picture

Issue tags: -Novice

I just remove the "novice" tag because i think it's still not clear what is necessary to do here.

still We need the input from the Entity/Api /Routing guys?

Wim Leers’s picture

#1837388: Provide a ParamConverter than can upcast any entity. is RTBC and will remove EditEntityAccessCheck because it's simply not necessary anymore. That will only leave EditEntityFieldAccessCheck.

Wim Leers’s picture

Title: Rename EditEntityAccessCheck and EditEntityFieldAccessCheck now that the module is named Quick Edit » Rename EditEntityFieldAccessCheck now that the module is named Quick Edit
Issue summary: View changes
tim.plunkett’s picture

I'd be fine with renaming it to QuickEditEntityFieldAccessCheck but that doesn't seem to be the direction Wim wants to go in.

The other access check *was* generic and only existed due to limitations of the upcasting system that are now fixed.

EditEntityFieldAccessCheck looks pretty generic, but where would it live?

Can we get a new issue summary detailing the other proposal?

Wim Leers’s picture

Status: Active » Needs review
FileSize
8.45 KB

Let's go with QuickEditEntityFieldAccessCheck — at this point in the cycle doing the work necessary to allow for a generic "entity field" access check no longer makes sense.

So +1 to the original proposals of @xjm and @tim.plunkett from ~1.5 year ago.

Wim Leers’s picture

Wim Leers’s picture

Issue tags: +rc deadline

Status: Needs review » Needs work

The last submitted patch, 11: 2267621-11.patch, failed testing.

The last submitted patch, 11: 2267621-11.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
12.14 KB

Something very strange went wrong with that patch. No interdiff because of that.

Alan Evans’s picture

So, I've had a read through the patch and I can't fault it - I've tried hard, believe me. The only issue I've found came from checking the patched codebase for instances of "EditEntityFieldAccessCheck" and I happened on the "contains" comment in one file:

core/modules/quickedit/tests/modules/src/MockQuickEditEntityFieldAccessCheck.php
5: * Contains \Drupal\quickedit_test\MockEditEntityFieldAccessCheck.

Looks like it will need a re-roll also when that's done.

Wim Leers’s picture

#17: that could totally be fixed on commit, it's only a single tiny typo :)

If you have no further concerns, RTBC? :)

Alan Evans’s picture

Status: Needs review » Reviewed & tested by the community

Sure thing ... I'm still always a little unsure on whether it's legit for me to mark stuff RTBC, or whether that needs to be left to someone with more community clout than me. Anyhoo ... I'll take your suggestion as a go-ahead on this one.

Thanks for the pointer regarding fix-on-commit. I didn't realise that was a done thing - I guess I'm more used to merge/pull workflows where you'd always need a "feature branch" to be in its final state before merging. Glad that we have a little flexibility here and there :).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2267621-16.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

The fail makes no sense. Re-tested.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2267621-16.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Version: 8.2.x-dev » 8.3.x-dev
Issue tags: -rc deadline +Needs reroll, +Novice, +php-novice

In the past year, we've gained a lot of experience with these kinds of issues. We can do this.

  1. EditEntityFieldAccessCheck must be renamed to QuickEditEntityFieldAccessCheck
  2. We must add a EditEntityFieldAccessCheck class that is marked @deprecated and just extends QuickEditEntityFieldAccessCheck
  3. Tada, BC!
tameeshb’s picture

Assigned: Unassigned » tameeshb
tameeshb’s picture

@Wim Leers Do the changes mentioned in #25 are yet to be made or does this patch needs a reroll or both?
Also, if both need to be done, is it recommended to reroll before moving the code or the other way round?
Thanks :)

Wim Leers’s picture

@tameeshb: It's best to start from scratch :)

tameeshb’s picture

@Wim Leers, will it include also renaming all occurences of "MockEditEntityFieldAccessCheck()" and "EditEntityFieldAccessCheckInterface"?

tameeshb’s picture

@Wim Leers looking at the patch at #16 It looks like the answer to my previous question is yes.
Also I assume, I have to rename the filenames with the occurrences of "EditEntityFieldAccessCheck".

tameeshb’s picture

tameeshb’s picture

Status: Needs work » Needs review
tameeshb’s picture

@Wim Leers Reroll from scratch Successful!
Please review! :)

tameeshb’s picture

@Wim Leers #25

We must add a EditEntityFieldAccessCheck class that is marked @deprecated and just extends QuickEditEntityFieldAccessCheck

/**
 * @deprecated in Drupal 8.3.x-dev
 * "EditEntityFieldAccessCheck" was renamed as "QuickEditEntityFieldAccessCheck"
 */
class EditEntityFieldAccessCheck extends QuickEditEntityFieldAccessCheck {}

Where(which php file) should I put this in? Will putting it here core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php work?

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll
rename from core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
rename to core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheckInterface.php
rename from core/modules/quickedit/tests/modules/src/MockEditEntityFieldAccessCheck.php
rename to core/modules/quickedit/tests/modules/src/MockQuickEditEntityFieldAccessCheck.php

It all looks great, except for these two. You cannot just rename them, that breaks BC. You have to:

  1. @deprecate the current classes
  2. add the new classes
  3. let the new classes extend the current classes

In other words: this must do what I wrote in #25.

tameeshb’s picture

@Wim Leers I've undone the renaming of those two files and also, defined the older as extensions of the newer classes.
Also, for interface QuickEditEntityFieldAccessCheckInterface I have used "implements" to extend it.

class EditEntityFieldAccessCheckInterface implements QuickEditEntityFieldAccessCheckInterface {

}

Please review, Thanks :)

tameeshb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: QuickEditEntityFieldAccess-2267621-36_.patch, failed testing.

MaskyS’s picture

Status: Needs work » Needs review

Let's retest this, shall we?

tameeshb’s picture

Yes, sure!

MaskyS’s picture

Hmm, failed again...

tameeshb’s picture

@Kifah Meeran, i'll rewrite the patch

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tameeshb’s picture

Redone patch and attached! :)

Status: Needs review » Needs work

The last submitted patch, 44: QuickEdit2267621-44.patch, failed testing.

tameeshb’s picture

Cannot redeclare class Drupal\quickedit_test\MockEditEntityFieldAccessCheck

tameeshb’s picture

Status: Needs work » Needs review
FileSize
11.79 KB

Left a small error in the previous patch.
Reuploaded patch.

Wim Leers’s picture

Status: Needs review » Needs work
rename from core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
rename to core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php

Rather than repeating myself, please see what I wrote in #35.

chiranjeeb2410’s picture

Made remaining changes

tameeshb’s picture

Status: Needs work » Needs review
gnuget’s picture

Hi @Chiranjeeb2410.

Thanks for your help on this, can you please provide an interdiff (https://www.drupal.org/documentation/git/interdiff) ?

gnuget’s picture

Status: Needs review » Needs work
Wim Leers’s picture

diff --git a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
deleted file mode 100644
diff --git a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
deleted file mode 100644

Sigh. See #48.

gnuget’s picture

Ok, ok, what we need to do basically is: (from #25)

  • EditEntityFieldAccessCheck must be renamed to QuickEditEntityFieldAccessCheck
  • We must add a EditEntityFieldAccessCheck class that is marked @deprecated and just extends QuickEditEntityFieldAccessCheck
  • Tada, BC!

So first:

mv EditEntityFieldAccessCheckTest.php QuickEditEntityFieldAccessCheck.php

Change the name of the class from EditEntityFieldAccessCheckTest to QuickEditEntityFieldAccessCheck in this new class

After that, lets create again EditEntityFieldAccessCheckTest.php but now that class will be empty and only will extend QuickEditEntityFieldAccessCheck and add the @deprecated comment, something like:


namespace Drupal\Tests\quickedit\Unit\Access;

/**
 * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
 */
class EditEntityFieldAccessCheckTest extends QuickEditEntityFieldAccessCheck {
}

So any code which use that class will still be able to use all its methods keeping the code backward compatible.

Profit!

repeat the same with MockEditEntityFieldAccessCheck.php (from #35)

And please don't forget the interdiff (https://www.drupal.org/documentation/git/interdiff) it is (at least for me) super hard to review patches without it.

Thanks!

tameeshb’s picture

@gnuget I'm not sure if "mv" is the way to go, in accordance to #35

tameeshb’s picture

Status: Needs work » Needs review
FileSize
18.71 KB
24.95 KB

Patch redone, please review!

tameeshb’s picture

Missed a new line in previous patch, re-adding patch with interdiff.

The last submitted patch, 56: 2267621-56.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 57: 2267621-57.patch, failed testing.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
982 bytes
25.02 KB

Updated test file

tameeshb’s picture

Status: Needs review » Needs work

The last submitted patch, 60: 2267621-60.patch, failed testing.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
21.33 KB
5.25 KB

Extending QuickEditEntityFieldAccessCheckTest for EditEntityFieldAccessCheckTest gave the above error. Added the original code without extending for EditEntityFieldAccessCheckTest. Is there a resolution to the problem by extending it?

gnuget’s picture

Status: Needs review » Needs work
  1. @ -8,55 +8,13 @@
     use Drupal\Core\Entity\EntityInterface;
    
     /**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    +/**
      * Access check for editing entity fields.
      */
    -class EditEntityFieldAccessCheck implements AccessInterface, EditEntityFieldAccessCheckInterface {
    -
    +class EditEntityFieldAccessCheck extends QuickEditEntityFieldAccessCheck {
      /**
      * Checks Quick Edit access to the field.
    - *

    The "Checks Quick Edit access to the field" comment seems orphant now, can we remove it?

  2. diff --git a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface
    .php
    index c7f14e2..d8f3f28 100644
    --- a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
    +++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
    @@ -7,19 +7,10 @@
     /**
      * Access check for editing entity fields.
      */
    -interface EditEntityFieldAccessCheckInterface {
    +interface EditEntityFieldAccessCheckInterface extends QuickEditEntityFieldAccessCheckInterface {
    
      /**
      * Checks access to edit the requested field of the requested entity.
    - *
    - * @param \Drupal\Core\Entity\EntityInterface $entity
    - * The entity.
    - * @param string $field_name
    - * The field name.
    - *
    - * @return \Drupal\Core\Access\AccessResultInterface
    - * The access result.
      */
    - public function accessEditEntityField(EntityInterface $entity, $field_name);
    
     }

    In this file the line: use Drupal\Core\Entity\EntityInterface; is not used anymore.
    Also there is an orphan comment there.

  3. diff --git a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
    similarity index 87%
    copy from core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
    copy to core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
    index 7b38838..9998a2e 100644
    --- a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
    +++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
    @@ -8,9 +8,12 @@
     use Drupal\Core\Entity\EntityInterface;
    
     /**
    - * Access check for editing entity fields.
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
      */
    -class EditEntityFieldAccessCheck implements AccessInterface, EditEntityFieldAccessCheckInterface {
    +/**
    + * Access check for in-place editing entity fields.
    + */
    +class QuickEditEntityFieldAccessCheck implements AccessInterface, QuickEditEntityFieldAccessCheckInterface {
    
      /**
      * Checks Quick Edit access to the field.

    Why the new class is deprecated? did I understand something wrong? the old (now empty) class is the one which should be deprecated, not the new one.

  4. copy from core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
    copy to core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheckInterface.php
    index c7f14e2..2e0a2c2 100644
    --- a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
    +++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheckInterface.php
    @@ -5,9 +5,12 @@
     use Drupal\Core\Entity\EntityInterface;
    
     /**
    - * Access check for editing entity fields.
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
      */
    -interface EditEntityFieldAccessCheckInterface {
    +/**
    + * Access check for in-place editing entity fields.
    + */
    +interface QuickEditEntityFieldAccessCheckInterface {
    
       /**
        * Checks access to edit the requested field of the requested entity.

    The same as above, this one shound't be deprecated. the same for QuickEditEntityFieldAccessCheckInterface

Extending QuickEditEntityFieldAccessCheckTest for EditEntityFieldAccessCheckTest gave the above error. Added the original code without extending for EditEntityFieldAccessCheckTest. Is there a resolution to the problem by extending it?

Not sure but that might be related with this? #2853581: make sure we specify @group annotations for all test classes I think the @group tags must be present in the parents and in the class otherwise the testbot complains.

But checking the errors seems a random fail?

Thanks for your work on this.

gnuget’s picture

Also, the idea of extending the classes is keeping them backward compatible, so #63 is just not in the correct path. (I reviewed #60 instead)

Thanks.

mpdonadio’s picture

Extending QuickEditEntityFieldAccessCheckTest for EditEntityFieldAccessCheckTest gave the above error. Added the original code without extending for EditEntityFieldAccessCheckTest.

That's not what is happening in #60:

+++ b/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php
@@ -5,145 +5,18 @@
+class EditEntityFieldAccessCheckTest extends QuickEditEntityFieldAccessCheck {
 }

EditEntityFieldAccessCheckTest is extending a normal class, not QuickEditEntityFieldAccessCheckTest, so phpunit isn't happen. If you make that change, the test should pass. BTW, all of the use statements in EditEntityFieldAccessCheckTest in #60 are unused and can be deleted.

Did not look to see if all of @WimLeer's feedback was addressed in #60 or #62 or really do a proper review. Just looked at this error b/c someone asked in IRC.

Lal_’s picture

@gnuget done as per your instructions.

Status: Needs review » Needs work
joyceg’s picture

@AbhishekLal,
I think you have missed the intermediate discussions. Please follow up with the comments mentioned and update the patch and inter diff accordingly.

tameeshb’s picture

Made changes from #64

tameeshb’s picture

Status: Needs work » Needs review
tameeshb’s picture

@gnuget #63 is also extending the classes for all classes except the test file as it was having issues when extended in #60
In the interdiff-60-63, only the test file was changed from being extended to its original code, modified a bit.
class EditEntityFieldAccessCheckTest extends QuickEditEntityFieldAccessCheck was not working in #60 gave errors ( #62 )

tameeshb’s picture

@AbhishekLal Your patch only has the changes mentioned in #54, all the other changes are missing.
Also, I learnt, the tests wont run without the "@group"

* @group Access
 * @group quickedit
Lal_’s picture

@ joyceg I am really sorry, my internet was a bit slow so I could'nt see the update(that's why named the patch as 63 ) pardon me.

mpdonadio’s picture

#73, re @group. Tests will run locally w/o the @group when you run them individually with phpunit. When you upload a patch without the @group on a test, TestBot will come back with a CI error, and there will be a message about it in the Jenkins log (think it is in the full console output when you click through enough times).

tameeshb’s picture

@mpdonadio Hmm, thanks! :)
I see where I made the mistake in #60
Please Review

tameeshb’s picture

Missed a new line again :P

mpdonadio’s picture

OK, when reviewing a patch where you are refactor class names and essentially renaming/moving files around, it helps to use the recommended git configuration for Drupal, in particular the

[diff]
  renames = copies

part. This drops the patch from 25k to 21k, so there is less to review and less chance of missing something. Uploading this version just to review.

mpdonadio’s picture

Status: Needs review » Needs work

Getting there!

  1. +++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
    @@ -8,55 +8,11 @@
     /**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    +/**
      * Access check for editing entity fields.
      */
    -class EditEntityFieldAccessCheck implements AccessInterface, EditEntityFieldAccessCheckInterface {
    

    The standard message is "@deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0." Note that this patch is against 8.4.x, so that is what the message needs to reflect. This should also be all part of the same comment, with the @deprecated below the "Access ..." line w/ one blank in between. See https://www.drupal.org/node/1354 for more info and other examples in core. This applies to everywhere the @deprecated is used in this patch.

  2. +++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
    @@ -8,9 +8,9 @@
    - * Access check for editing entity fields.
    + * Access check for in-place editing entity fields.
    

    @WimLeers, is this an in-scope change?

  3. +++ b/core/modules/quickedit/tests/modules/src/MockQuickEditEntityFieldAccessCheck.php
    @@ -0,0 +1,23 @@
    +/**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    +/**
    + * Access check for in-place editing entity fields.
    + */
    +class MockQuickEditEntityFieldAccessCheck implements QuickEditEntityFieldAccessCheckInterface {
    

    Why is this @deprecated? This is the new version.

Also, all of the `use` statements in EditEntityFieldAccessCheck and `MockEditEntityFieldAccessCheck` are unused and should be removed.

PhpStorm is telling me that all of the EditEntity* classes are unused, so that should be we caught all of the places that need to be updated. Good work.

mpdonadio’s picture

+++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
@@ -2,24 +2,12 @@
diff --git a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php

diff --git a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
similarity index 91%

similarity index 91%
copy from core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php

copy from core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
copy to core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php

copy to core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
index 7b38838..5404b04 100644

index 7b38838..5404b04 100644
--- a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php

--- a/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
+++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php

+++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
+++ b/core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php
@@ -8,9 +8,9 @@

@@ -8,9 +8,9 @@
 use Drupal\Core\Entity\EntityInterface;
 
 /**
- * Access check for editing entity fields.
+ * Access check for in-place editing entity fields.
  */
-class EditEntityFieldAccessCheck implements AccessInterface, EditEntityFieldAccessCheckInterface {
+class QuickEditEntityFieldAccessCheck implements AccessInterface, QuickEditEntityFieldAccessCheckInterface {
 
   /**
    * Checks Quick Edit access to the field.

So, take a look at the patch in #77 and the one in #78 for the changes related to QuickEditEntityFieldAccessCheck.php. With the git config, it just says it copies the file and then shows the changes. Without it, you see the whole patch. With the config, you can see that two changes, the comment change and the class rename. Without it, you see everything else, and may have missed the fact that there was a comment change introduced.

Wim Leers’s picture

That's looking much better already :)

#79:

  1. +1
  2. I think that's okay, yes.
  3. Heh, made the same remark :)

Review:

  1. +++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
    @@ -8,55 +8,11 @@
     /**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    +/**
      * Access check for editing entity fields.
      */
    

    Let's remove the old description, let's only keep the @deprecated. That's also what we've done elsewhere in Drupal core in cases like these.

  2. +++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
    @@ -2,24 +2,12 @@
    +/**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
     /**
      * Access check for editing entity fields.
      */
    

    Same here.

  3. +++ b/core/modules/quickedit/tests/modules/src/MockEditEntityFieldAccessCheck.php
    @@ -3,18 +3,14 @@
     use Drupal\Core\Entity\EntityInterface;
    -use Drupal\quickedit\Access\EditEntityFieldAccessCheckInterface;
    +use Drupal\quickedit\Access\QuickEditEntityFieldAccessCheckInterface;
    

    Not only is it pointless to update this use statement: all of these are no longer in use. Let's remove them. Here and in many other places too.

  4. +++ b/core/modules/quickedit/tests/modules/src/MockEditEntityFieldAccessCheck.php
    @@ -3,18 +3,14 @@
     /**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    +/**
      * Access check for editing entity fields.
      */
    

    Same here.

  5. +++ b/core/modules/quickedit/tests/modules/src/MockQuickEditEntityFieldAccessCheck.php
    @@ -0,0 +1,23 @@
    +/**
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    

    This one is not deprecated…

  6. +++ b/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php
    @@ -2,148 +2,14 @@
    + * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.x
    + */
    

    We don't need to maintain BC for these tests. This test class can simply be removed.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
20.81 KB
11.48 KB

Changes from #79 applied after adding

[diff]
  renames = copies

to ~/.gitconfig
Please review!

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
@@ -8,55 +8,11 @@
 /**
+ * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
+ *
  * Access check for editing entity fields.
  */

+++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheckInterface.php
@@ -2,24 +2,11 @@
 /**
+ * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
+ *
  * Access check for editing entity fields.
  */

See my remark from #81 about this.

tameeshb’s picture

@Wim Leers, sorry I saw #81 later after uploading my patch... (so my patch is also named 81)
Currently working on it!

tameeshb’s picture

Status: Needs work » Needs review
FileSize
11.45 KB
15.07 KB

Applied changes from #81
I deleted: ( #81.6 )

deleted:    core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php

But now it says

rename from core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php
rename to core/modules/quickedit/tests/src/Unit/Access/QuickEditEntityFieldAccessCheckTest.php

Will that be fine?

gnuget’s picture

Will that be fine?

That is the normal git behavior if it detects to a file was removed and another almost with the same content was created it assume to you rename the file instead, which is fine.

gnuget’s picture

Status: Needs review » Needs work

This looks almost ready.

Just a small nit:

  1. namespace Drupal\quickedit\Access;
    
    use Drupal\Core\Access\AccessResult;
    use Drupal\Core\Routing\Access\AccessInterface;
    use Drupal\Core\Session\AccountInterface;
    use Drupal\Core\Entity\EntityInterface;
    
    /**
     * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
     */
    class EditEntityFieldAccessCheck extends QuickEditEntityFieldAccessCheck {
    
    }
    

    here we still need to get rid of all the use statements, They are not longer necessary.

Thanks!

Wim Leers’s picture

Will that be fine?

Yes!

Just one small thing left, then this is RTBC:

+++ b/core/modules/quickedit/src/Access/EditEntityFieldAccessCheck.php
@@ -8,55 +8,8 @@
 use Drupal\Core\Entity\EntityInterface;

Unused use statements here.

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
24.74 KB
630 bytes

Here is the new patch. Should this be RTBC?

tameeshb’s picture

You missed #80 & #78 in #89

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

Why is the interdiff so big in at #90? I was expecting to see just a few lines marked as deleted. o.O

I reviewed the whole patch one more time anyway and it looks ready!

Thanks tameeshb

MaskyS’s picture

+1 from me

xjm’s picture

Title: Rename EditEntityFieldAccessCheck now that the module is named Quick Edit » Deprecate and rename EditEntityFieldAccessCheck now that the module is named Quick Edit
Wim Leers’s picture

Perfect! :) Zero remarks. Thanks!

  • xjm committed d85cf06 on 8.4.x
    Issue #2267621 by tameeshb, Wim Leers, himanshu-dixit, mpdonadio, xjm,...
xjm’s picture

Thanks @Wim Leers for confirming!

I also reviewed with git diff --color-words to check over the changes, and double-checked that there are no additional instances of the old name lurking in any case or naming convention:

[ibnsina:maintainer | Fri 15:29:19] $ grep -ri "editentityfieldaccess" * | grep -vi "quickeditentityfieldaccess"
[ibnsina:maintainer | Fri 15:29:39] $ grep -ri "edit_entity_field_access" *
[ibnsina:maintainer | Fri 15:30:08] $ 

Committed to 8.4.x. I went to publish the change record but there is not one. So drafting one myself since it is small.

tameeshb’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! @xjm :)

xjm’s picture

mpdonadio’s picture

@tameeshb, in case you work on other issues, the committer is the one who sets the Fixed status. In this case, the issue wasn't complete until the Change Record was actually published.

Status: Fixed » Closed (fixed)

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

himanshu-dixit’s picture

Attributing the contribution to GSoC.