Opened 7 years ago
Last modified 6 days ago
#43421 reviewing enhancement
The $capabilities argument in the `add_role()` function requires an explicit grant
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 4.9.4 |
Component: | Role/Capability | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
Reproduce:
Add a role to WP using add_role()
, and pass it custom capabilities using the third argument. Like:
<?php $userCaps = [ 'read', 'my_cap', ]; add_role('my_new_role', 'My New Role', $userCaps);
Create a user of said role. Then:
<?php $user = new WP_User($id_of_created_user); return user_can($user, 'my_cap'); // returns false, expected return true
If, alternatively, I create my role this way:
<?php $userCaps = [ 'read', 'my_cap', ]; $role = add_role('my_new_role', 'My New Role'); foreach($userCaps as $cap) { $role->add_cap($cap); }
And repeat testing, it returns true as expected.
This has to do with how the caps are saved. The first way, when capabilities are retrieved in WP_User::has_cap()
, they look like this:
<?php array(4) { [0]=> "read", [1]=> "my_cap", ["my_new_role"]=> bool(true) ["exist"]=> bool(true) }
And when done via add_cap
, they look like this:
<?php array(4) { ["read"]=> bool(true) ["my_cap"]=> bool(true) ["my_new_role"]=> bool(true) ["exist"]=> bool(true) }
The subsequent empty
array check is true for the first, and false for the second.
Change History (12)
#2
@
7 years ago
The add_cap
method has the following signature: add_cap( $cap, $grant = true )
Notice the 2nd argument. To add a capability you need two things: a capability name, and a grant status (true
, false
).
When adding capabilities using add_role
, the third argument needs a way to pass both the capability name and the grant status. How does it do it? A key-value array.
As stated in the docblock:
* @param array $capabilities List of capabilities, e.g. array( 'edit_posts' => true, 'delete_posts' => false );
and the Codex examples show this, too: https://codex.wordpress.org/Function_Reference/add_role#Example
You have to pass both these things. When you pass $userCaps = [ 'read', 'my_cap', ];
you are effectively passing in the following array: [ 0 => 'read', 1 => 'my_cap' ]
, which in turn tries to perform two calls: add_cap( 0, 'read' )
and add_cap( 1, 'my_cap' )
, which is, indeed, incompatible with what you're trying to achieve.
This is not a bug, in my opinion.
#3
@
7 years ago
- Milestone Awaiting Review deleted
- Resolution set to invalid
- Status changed from new to closed
Thanks for the report, but yes this isn't a bug and the documentation for both functions is correct.
#4
@
7 years ago
I actually agree, but think it might be worth reviewing as an enhancement. If $capabilities
is passed a numerically-keyed array (or "not associative", or whatever), that it be assumed each cap should be assigned as "true".
Thoughts?
Semi-related, what's the use case for explicitly saying someone doesn't have a capability ("my_cap" => false
)?
#6
@
7 years ago
- Resolution invalid deleted
- Status changed from closed to reopened
Interesting though. add_role
doesn't come with any predefined capabilities that you would want to "disable". So theoretically this sugary syntax could be supported. Any thoughts?
#7
@
7 years ago
By "this sugary syntax", you mean passing an array of caps without associated booleans? Yeah, I think from a UX perspective it makes sense. Even though the nuts and bolts of how it's accomplished are different, when I add_cap
, there's a mechanism that assumes I mean true (the default second arg). A consistent UX would allow for the same assumption to be made when assigning caps at role creation.
This ticket was mentioned in PR #7351 on WordPress/wordpress-develop by @eclev91.
5 months ago
#9
- Keywords has-patch added; needs-patch removed
Updates WP_Role::add_role to assume capabilities should be granted for capabilities not passed associatively.
Trac ticket: https://core.trac.wordpress.org/ticket/43421
The way around this is to format your capabilities when using
add_role
like so:I really feel like it should support a generic array with true being assumed.