hook_permission is missing doc about 'warning' [#1037352]

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

Comment #1

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 24 January 2011 at 15:28

Status:

Active

» Closed (works as designed)

Not really:

       $perm_item += array(
          'description' => '', 
          'restrict access' => FALSE, 
          'warning' => !empty($perm_item['restrict access']) ? t('Warning: Give to trusted roles only; this permission has security implications.') : '',
        );

The code is generating a warning from the 'restrict access' field from hook_permission(), which is correctly documented in hook_permission() docs.

Log in or register to post comments

Comment #2

JimmyAx CreditAttribution: JimmyAx commented 24 January 2011 at 21:15

Status:

Closed (works as designed)

» Active

I'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.

Log in or register to post comments

Comment #3

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 24 January 2011 at 21:24

Title:

Documentation problem with hook_permission

» hook_permission is missing doc about 'warning'

Ah, OK. Yes that should be documented in hook_permission.

Log in or register to post comments

Comment #4

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 February 2011 at 23:12

Status:

Active

» Needs review

File	Size
1037352.patch	836 bytes

Here's a patch.

Log in or register to post comments

Comment #5

kim-day CreditAttribution: kim-day commented 14 March 2011 at 01:55

Status:

Needs review

» Reviewed & tested by the community

the patch works correctly - looks good

Log in or register to post comments

Comment #6

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 March 2011 at 22:58

Version:

7.x-dev

» 8.x-dev

Needs to go to d8 and then d7

Log in or register to post comments

Comment #7

Dries CreditAttribution: Dries commented 20 March 2011 at 00:33

Version:

8.x-dev

» 7.x-dev

Committed to 8.x. Thanks.

Log in or register to post comments

Comment #8

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 21 March 2011 at 18:27

Status:

Reviewed & tested by the community

» Fixed

Committed to 7.x. Thanks!

Log in or register to post comments

Comment #9

David_Rothstein CreditAttribution: David_Rothstein commented 24 March 2011 at 04:03

Version:	7.x-dev	» 8.x-dev
Status:	Fixed	» Needs review

File	Size
1037352-followup-9.patch	1.73 KB

I 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 :)