Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
EditEntityFieldAccessCheck
has a somewhat confusing name, especially now that the module has been renamed.
Proposed resolution
- Rename
EditEntityFieldAccessCheck
toQuickEditEntityFieldAccessCheck
.
Remaining tasks
- Needs patch.
- Will conflict with #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes so it should wait on that.
API changes
EditEntityFieldAccessCheck
renamed toQuickEditEntityFieldAccessCheck
.
Postponed until
Comment | File | Size | Author |
---|---|---|---|
#90 | interdiff-85-90.txt | 4.9 KB | tameeshb |
#90 | 2267621-90.patch | 15.24 KB | tameeshb |
#89 | interdiff.txt | 630 bytes | himanshu-dixit |
#89 | 2267621-89.patch | 24.74 KB | himanshu-dixit |
#85 | 2267621-85.patch | 15.07 KB | tameeshb |
Comments
Comment #1
xjmComment #2
xjmComment #3
Wim LeersNo, 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? :)
Comment #4
xjmHm. 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.
Comment #5
Wim LeersI just looked into deleting Quick Edit's
EditEntityAccessCheck
, turns outEntityAccessCheck
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 :)Comment #6
jessebeach CreditAttribution: jessebeach commentedUnpostponed!
Comment #7
gnugetI 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?
Comment #8
Wim Leers#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 leaveEditEntityFieldAccessCheck
.Comment #9
Wim Leers#1837388: Provide a ParamConverter than can upcast any entity. landed. Updating as per #8.
Comment #10
tim.plunkettI'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?
Comment #11
Wim LeersLet'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.
Comment #12
Wim LeersComment #13
Wim LeersComment #16
Wim LeersSomething very strange went wrong with that patch. No interdiff because of that.
Comment #17
Alan Evans CreditAttribution: Alan Evans as a volunteer and at Acquia commentedSo, 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:
Looks like it will need a re-roll also when that's done.
Comment #18
Wim Leers#17: that could totally be fixed on commit, it's only a single tiny typo :)
If you have no further concerns, RTBC? :)
Comment #19
Alan Evans CreditAttribution: Alan Evans as a volunteer and at Acquia commentedSure 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 :).
Comment #21
Wim LeersThe fail makes no sense. Re-tested.
Comment #25
Wim LeersIn the past year, we've gained a lot of experience with these kinds of issues. We can do this.
EditEntityFieldAccessCheck
must be renamed toQuickEditEntityFieldAccessCheck
EditEntityFieldAccessCheck
class that is marked@deprecated
and just extendsQuickEditEntityFieldAccessCheck
Comment #26
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #27
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@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 :)
Comment #28
Wim Leers@tameeshb: It's best to start from scratch :)
Comment #29
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@Wim Leers, will it include also renaming all occurences of "MockEditEntityFieldAccessCheck()" and "EditEntityFieldAccessCheckInterface"?
Comment #30
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@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".
Comment #31
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedRedone from scratch
Comment #32
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #33
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@Wim Leers Reroll from scratch Successful!
Please review! :)
Comment #34
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@Wim Leers #25
Where(which php file) should I put this in? Will putting it here core/modules/quickedit/src/Access/QuickEditEntityFieldAccessCheck.php work?
Comment #35
Wim LeersIt all looks great, except for these two. You cannot just rename them, that breaks BC. You have to:
@deprecate
the current classesIn other words: this must do what I wrote in #25.
Comment #36
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@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.
Please review, Thanks :)
Comment #37
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #39
MaskyS CreditAttribution: MaskyS at Google Code-In commentedLet's retest this, shall we?
Comment #40
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedYes, sure!
Comment #41
MaskyS CreditAttribution: MaskyS at Google Code-In commentedHmm, failed again...
Comment #42
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@Kifah Meeran, i'll rewrite the patch
Comment #44
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedRedone patch and attached! :)
Comment #46
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedCannot redeclare class Drupal\quickedit_test\MockEditEntityFieldAccessCheck
Comment #47
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedLeft a small error in the previous patch.
Reuploaded patch.
Comment #48
Wim LeersRather than repeating myself, please see what I wrote in #35.
Comment #49
chiranjeeb2410 CreditAttribution: chiranjeeb2410 commentedMade remaining changes
Comment #50
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #51
gnugetHi @Chiranjeeb2410.
Thanks for your help on this, can you please provide an interdiff (https://www.drupal.org/documentation/git/interdiff) ?
Comment #52
gnugetComment #53
Wim LeersSigh. See #48.
Comment #54
gnugetOk, ok, what we need to do basically is: (from #25)
So first:
Change the name of the class from
EditEntityFieldAccessCheckTest
toQuickEditEntityFieldAccessCheck
in this new classAfter that, lets create again
EditEntityFieldAccessCheckTest.php
but now that class will be empty and only will extendQuickEditEntityFieldAccessCheck
and add the @deprecated comment, something like: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!
Comment #55
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@gnuget I'm not sure if "mv" is the way to go, in accordance to #35
Comment #56
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedPatch redone, please review!
Comment #57
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedMissed a new line in previous patch, re-adding patch with interdiff.
Comment #60
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedUpdated test file
Comment #61
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #63
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedExtending 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?
Comment #64
gnugetThe "Checks Quick Edit access to the field" comment seems orphant now, can we remove it?
In this file the line:
use Drupal\Core\Entity\EntityInterface;
is not used anymore.Also there is an orphan comment there.
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.
The same as above, this one shound't be deprecated. the same for
QuickEditEntityFieldAccessCheckInterface
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.
Comment #65
gnugetAlso, 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.
Comment #66
mpdonadioThat's not what is happening in #60:
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.
Comment #67
Lal_@gnuget done as per your instructions.
Comment #69
joyceg CreditAttribution: joyceg commented@AbhishekLal,
I think you have missed the intermediate discussions. Please follow up with the comments mentioned and update the patch and inter diff accordingly.
Comment #70
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedMade changes from #64
Comment #71
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #72
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@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 )Comment #73
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@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"
Comment #74
Lal_@ 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.
Comment #75
mpdonadio#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).
Comment #76
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@mpdonadio Hmm, thanks! :)
I see where I made the mistake in #60
Please Review
Comment #77
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedMissed a new line again :P
Comment #78
mpdonadioOK, 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
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.
Comment #79
mpdonadioGetting there!
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.
@WimLeers, is this an in-scope change?
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.
Comment #80
mpdonadioSo, 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.
Comment #81
Wim LeersThat's looking much better already :)
#79:
Review:
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.Same here.
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.Same here.
This one is not deprecated…
We don't need to maintain BC for these tests. This test class can simply be removed.
Comment #82
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedChanges from #79 applied after adding
to ~/.gitconfig
Please review!
Comment #83
Wim LeersSee my remark from #81 about this.
Comment #84
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@Wim Leers, sorry I saw #81 later after uploading my patch... (so my patch is also named 81)
Currently working on it!
Comment #85
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedApplied changes from #81
I deleted: ( #81.6 )
But now it says
Will that be fine?
Comment #86
gnugetThat 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.
Comment #87
gnugetThis looks almost ready.
Just a small nit:
here we still need to get rid of all the use statements, They are not longer necessary.
Thanks!
Comment #88
Wim LeersYes!
Just one small thing left, then this is RTBC:
Unused
use
statements here.Comment #89
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedHere is the new patch. Should this be RTBC?
Comment #90
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedYou missed #80 & #78 in #89
Comment #91
gnugetWhy 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
Comment #92
MaskyS CreditAttribution: MaskyS at Google Code-In commented+1 from me
Comment #93
xjmComment #94
Wim LeersPerfect! :) Zero remarks. Thanks!
Comment #96
xjmThanks @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: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.
Comment #97
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedThanks! @xjm :)
Comment #98
xjmAdded and published https://www.drupal.org/node/2855748.
Comment #99
mpdonadio@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.
Comment #101
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer and at Google Summer of Code commentedAttributing the contribution to GSoC.