Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trac/43421 #7351

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from
+32 −3
Open

Trac/43421 #7351

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e67221b
Assume capabilities passed to add_role should be granted
ethanclevenger91 Sep 13, 2024
016fe1a
Update spacing
ethanclevenger91 Sep 13, 2024
3a4adec
phpcs fixes
ethanclevenger91 Sep 13, 2024
925c285
Test a mixed array of explicit and implicit capabilities.
ethanclevenger91 Sep 17, 2024
2566498
Merge branch 'WordPress:trunk' into trac/43421
ethanclevenger91 Sep 17, 2024
83a3538
cs cleanup
ethanclevenger91 Sep 17, 2024
7d5d1f7
Merge branch 'trac/43421' of github.com:ethanclevenger91/wordpress-de…
ethanclevenger91 Sep 17, 2024
ccf5d0d
Auto-cs fix.
ethanclevenger91 Sep 17, 2024
e764924
Remove support for mixed format capabilities.
ethanclevenger91 Sep 17, 2024
c284c8b
Excess newlines.
ethanclevenger91 Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/wp-includes/class-wp-roles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,13 @@ public function add_role( $role, $display_name, $capabilities = array() ) {
return;
}

foreach ( $capabilities as $key => $value ) {
if ( ! is_bool( $value ) ) {
$capabilities[ $value ] = true;
unset( $capabilities[ $key ] );
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than checking every value individually, I think it would make more sense to check the entire $capabilities array for the shape that is provides. While there's arguably a reason to either specify a $cap => $grant map or a list of caps, it makes no sense to me to allow a mix of the two - that would just be unintuitive.

So I think we could simplify this by using wp_is_numeric_array(). For example:

Suggested change
foreach ( $capabilities as $key => $value ) {
if ( ! is_bool( $value ) ) {
$capabilities[ $value ] = true;
unset( $capabilities[ $key ] );
}
}
if ( wp_is_numeric_array( $capabilities ) ) {
$capabilities = array_fill_keys( $capabilities, true );
}

Copy link
Author

@ethanclevenger91 ethanclevenger91 Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it were up to me, we would deprecate supporting a map entirely, as the absence of a capability implies it is not granted. Without explicitly deprecating that usage, I think it makes sense to support both, even in a single call. Is there a performance implication here? I won't disagree that it's kind of gross.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with @johnbillion we're going to move forward w/o support for a mixed format array


$this->roles[ $role ] = array(
'name' => $display_name,
'capabilities' => $capabilities,
Expand Down
23 changes: 23 additions & 0 deletions tests/phpunit/tests/user/capabilities.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,6 +991,29 @@ public function test_add_role() {
$this->assertFalse( $wp_roles->is_role( $role_name ) );
}

/**
* Test add_role with implied capabilities grant successfully grants capabilities.
*/
public function test_add_role_with_single_level_capabilities() {
$role_name = 'janitor';
add_role(
$role_name,
'Janitor',
array(
'level_1',
'sweep_floors' => false,
)
);
$this->flush_roles();

// Assign a user to that role.
$id = self::factory()->user->create( array( 'role' => $role_name ) );
$user = new WP_User( $id );

$this->assertTrue( $user->has_cap( 'level_1' ) );
$this->assertFalse( $user->has_cap( 'sweep_floors' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, I'm not sure why one would want to provide capabilities in that format. Either you should use a list of capabilities to grant, or if for some reason you want to include capabilities to set them to false, then it makes sense to provide a map of $cap => $grant.

In other words, IMO the test should become something like:

Suggested change
add_role(
$role_name,
'Janitor',
array(
'level_1',
'sweep_floors' => false,
)
);
$this->flush_roles();
// Assign a user to that role.
$id = self::factory()->user->create( array( 'role' => $role_name ) );
$user = new WP_User( $id );
$this->assertTrue( $user->has_cap( 'level_1' ) );
$this->assertFalse( $user->has_cap( 'sweep_floors' ) );
add_role(
$role_name,
'Janitor',
array(
'level_1',
'sweep_floors',
)
);
$this->flush_roles();
// Assign a user to that role.
$id = self::factory()->user->create( array( 'role' => $role_name ) );
$user = new WP_User( $id );
$this->assertTrue( $user->has_cap( 'level_1' ) );
$this->assertTrue( $user->has_cap( 'sweep_floors' ) );

}

/**
* Change the capabilities associated with a role and make sure the change
* is reflected in has_cap().
Expand Down
Loading