Trac/43421

[ethanclevenger91]

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props eclev91, flixos90, johnjamesjacoby, johnbillion.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[felixarntz]

I tend to be onboard with the idea to support a numeric array, but I question the value of supporting a mix of both.

[felixarntz]

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 );
	}

[ethanclevenger91]

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.

[ethanclevenger91]

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

[felixarntz]

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' ) );

[JJJ]

Since this reuses $value as a key on lines 165 & 166, it would make the code more resilient to do: is_int( $key ) && is_string( $value )

But, I think if we are considering doing argument validation, it may warrant a dedicated method – one where we can more intentionally conclude (and document) what mutations will take place to what kind of submitted keys & values.

(The obvious benefit of a dedicated method would be reuse elsewhere that an array of caps is passed in as an argument, ensuring the same results, and ability to write unit tests for it.)

Alternatively, wp_is_numeric_array() could be used before the new foreach, but that function checks that all array keys are numeric, which means we’d lose support for mixed key types (which was an example in the core ticket).

[JJJ]

What if $capabilities[$value] already exists, and what if it is false?

That could theoretically overwrite an intentionally blocked capability with an unintentionally allowed one.

[JJJ]

Unsetting the numeric array key does make sense to me in this scenario. 👍

[johnbillion]

Agree with Felix's comments. Let's get the docs for the $capabilities parameter of the add_role() global function and the WP_Roles::add_role() method updated, and a @since entry added for both.