Currently the access callback key requires the procedural function in a .module file to work. We can convert to CallableResolver, however as @berdir points out: https://git.drupalcode.org/project/drupal/-/merge_requests/12795#note_56...
I'm wondering if we want to deprecate this completely.
There are no usages of this in core. The very widely used EntityField field plugin doesn't call the parent, so 90% of fields would actually not support it.
There are several handler subclasses for it, which makes much more sense than a stray callback.
I found one case of such an access callback in a views.inc file, and that's on a custom handler that could easily override the access() method instead: https://git.drupalcode.org/project/opigno_calendar/-/blob/3.x/opigno_cal...
Seems like a such a weird concept, likely a leftover of D7 hook_menu() where it passed this through to that.
N/A
Deprecate the code path
Review
N/A
N/A
You can no longer pass 'access callback' in definition.
N/A
N/A
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
nicxvan commentedComment #4
nicxvan commentedComment #5
nicxvan commentedComment #6
nicxvan commentedComment #7
lendudeYeah deprecating this makes sense. And that seems like a good spot to do it.
That said, how do we want to proceed when removing this? If this is actually used in the wild (however unlikely), when we remove this functionality, it will go from an access callback to returning TRUE. Since we are dealing with access here, this could expose something somebody wanted hidden. Again, very unlikely, but something to consider, maybe ask somebody from the security team if we would be comfortable with this.
Comment #8
nicxvan commentedWe could return FALSE in that branch once it's removed and set a message.
Comment #9
larowlanTriggering an error now, and then returning FALSE without calling the access callback in D12 seems like the safest approach.
+1
Comment #10
nicxvan commentedThanks! I'll add a comment with a todo and a follow up
Comment #11
nicxvan commentedI added a todo with a link to a postponed issue for the secure deprecation removal.
I think this is RTBC, but will leave it for a bit for other's to chime in.
Comment #12
dcam commentedI left a suggestion on the MR for attribute conversion.
I see in the IS there's a pending task to update the CR. It looks like it was written a month and a half ago. Maybe the need to update was forgotten. It still needs to be taken care of.
Comment #14
nicxvan commentedI converted the deprecation thanks for catching that!
I also rebased and did it manually to add the use statement.
I also updated the CR, it's pretty niche and straightforward. I'm on the fence on whether we want a CR at all for this anyway.
It's ready for review again.
Comment #15
dcam commentedHa ha, that's good. I was just working in GitLab and obviously forgot about the use statement in the suggestion.
It all looks good to me now.
Comment #16
nicxvan commentedNo worries, thanks for catching the annotations!
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #19
nicxvan commentedLooks correct to me!
@oily if you are able to rebase rather than merge in these situations it makes future changes easier and cleaner.
Thanks!
Comment #20
oily commented@nicxvan Ah got it! Thanks, will do. BTW I was going to paste the test-only output but it explodes as expected, i think.
Comment #21
nicxvan commentedComment #22
catchCommitted/pushed to 11.x, thanks!
Comment #26
quietone commentedPublished the CR.