Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Looking at user_admin_permissions() it is revealed that hook_permission() supports a warning
key for a description of why the user should be careful with the permission. This key is not documented in the hook_permission() documentation.
Comment | File | Size | Author |
---|---|---|---|
#12 | 1037352-followup-12.patch | 1.39 KB | David_Rothstein |
#9 | 1037352-followup-9.patch | 1.73 KB | David_Rothstein |
#4 | 1037352.patch | 836 bytes | jhodgdon |
Comments
Comment #1
jhodgdonNot really:
The code is generating a warning from the 'restrict access' field from hook_permission(), which is correctly documented in hook_permission() docs.
Comment #2
JimmyAx CreditAttribution: JimmyAx commentedI'm not sure you understand the issue here. This isn't about the automatic generation of the warning, but rather that you are allowed to set your own warning in hook_permission(), therefore replacing the default warning. This custom warning is then passed to the permission admin interface. This isn't documented at all, not even a single word indicating that you could do so, yet it's fully supported in the API.
Comment #3
jhodgdonAh, OK. Yes that should be documented in hook_permission.
Comment #4
jhodgdonHere's a patch.
Comment #5
kim-day CreditAttribution: kim-day commentedthe patch works correctly - looks good
Comment #6
jhodgdonNeeds to go to d8 and then d7
Comment #7
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #8
webchickCommitted to 7.x. Thanks!
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedI guess it was a mistake not to document this parameter at all... but I think we really really want to discourage people from using it.
The idea is that this is supposed to be a clear, standard warning that is constant across all permissions, not a place for people to dump things that actually belong in the permission description. I can think of very few (if any) situations where it would be a good idea to set this.
Here's one approach to deal with this via the documentation (attached).
I think it might actually be preferable to just take away the ability to set this altogether (you can still change the text globally for all permissions via the theme system, e.g. using a preprocess function, which is a legitimate thing to do). But since this is an issue in the documentation queue, I'm starting with a documentation patch :)
Comment #10
jhodgdonI'm on board with this idea, and the patch wording seems fine to me.
Should go into 8.x and 7.x.
Comment #11
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to D7. Probably needs work as both patches will have to be merged.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedThe original patch was already committed to D7, so #9 should apply fine there as well.
However, first we have to fix the conflicts that were introduced in D8...
Comment #13
Dries CreditAttribution: Dries commentedSorry about that. Committed #12 to my local 8.x tree, and will commit the changes shortly. Moving back to 7.x. Thanks David.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedPoor testbot. #9 is the one that's RTBC for Drupal 7.
Comment #16
webchickCommitted #9 to 7.x. Thanks!