Documentation problem with user_role_change_permissions [#923850]

The docs state:

> where the key holds the permission name and the value is an integer or boolean that determines whether to grant or revoke the permission:

However, you can pass a value that is a string rather than an integer, and in fact in form values array of checkboxes, the value of each selected checkbox will be equal to the key, ie

foo => foo, // this is selected
bar => 0, // this is not.

For example, this is what user_admin_permissions_submit() does this.

This needs to be stated explicitly, otherwise module authors may be thinking they have to run through their form values to convert the strings to 1s.

Comment	File	Size	Author
#6	923850-user_role_change_permissions.patch	1.9 KB	mr.baileys
#6
#4	923850-4.drupal.user_role_change_permissions.patch	1.89 KB	joachim
#4
#3	923850-user_role_change_permissions.patch	1.88 KB	mr.baileys
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 29 September 2010 at 14:04

Frankly, I'd be more in favor of changing the implementation of user_role_change_permissions to match the docs.

Using the current implementation, any value that evaluates to true will grant the permission when run through user_role_change_permissions:

array(
  'permission foo' => 'NOOOO',
  'permission bar' => -1,
  'permission naz' => array('lol look at me I'm an arrai!'),
);

Comment #2

joachim CreditAttribution: joachim commented 29 September 2010 at 14:10

I see your point... but the point of this API function is that you can feed it the values FormAPI gets back for a set of checkboxes.

If we change the function as you describe, a bunch of core forms need to do the extra work of massaging their arrays.

Comment #3

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 30 September 2010 at 07:13

Status:

Active

» Needs review

File	Size
923850-user_role_change_permissions.patch	1.88 KB

I see your point too. I dislike the fact that API functions are concerned about forms stuff, but the docs do mention "…the submitted values may be directly passed on in a form submit handler.".

Patch attached which documents the $permissions parameter more in detail.

Comment #4

joachim CreditAttribution: joachim commented 30 September 2010 at 08:02

File	Size
923850-4.drupal.user_role_change_permissions.patch	1.89 KB

Rerolled for whitespace and formatting.

I also meant to change the use of user_role_change_permissions to have a () after it, but 'this function' given that's what it is seems to read better to me.

Comment #5

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 October 2010 at 21:47

Status:

Needs review

» Needs work

Looks pretty close.

+ *   determines whether to grant or revoke that permission. Any value that
+ *   evaluates to true will cause the permission to granted. Values that
+ *   evaluate to false will cause the permission to be revoked.

to granted -> to be granted. Also I would like these two sentences to use parallel structure -- both saying "any value" or both saying "values", since that other change needs to be made too. Also, in Drupal we use TRUE/FALSE, not true/false, I think?

I'm also not sure why you want to include bad code in the examples:

+ *       'administer nodes' => 0,                // Revoke 'administer nodes'
+ *       'administer blocks' => false,           // Revoke 'administer blocks'
+ *       'access user profiles' => 1,            // Grant 'access user profiles'
+ *       'access comments' => 'access comments', // Grant 'access comments'
+ *       'access content' => -1,                 // Grant 'access content'

I think we should only demonstrate 0 and 1, TRUE and FALSE, and we should always use FALSE instead of false. Giving examples of poor code like -1 and 'access comments' is not really illuminating. PHP programmers know what "evaluates to true" means.

Comment #6

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 19 October 2010 at 05:32

Status:

Needs work

» Needs review

File	Size
923850-user_role_change_permissions.patch	1.9 KB

Thanks for the review jhodgdon,

I've taken care of the issues you mentioned, although I did leave 'access comments' => 'access comments' in there: since user_role_change_permissions() can be used in a form submit handler, this is actually a valid use and should not be considered a bad example (core uses it in this manner).