Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
I tend to be onboard with the idea to support a numeric array, but I question the value of supporting a mix of both.
src/wp-includes/class-wp-roles.php
Outdated
| foreach ( $capabilities as $key => $value ) { | ||
| if ( ! is_bool( $value ) ) { | ||
| $capabilities[ $value ] = true; | ||
| unset( $capabilities[ $key ] ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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:
| 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 ); | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
After discussion with @johnbillion we're going to move forward w/o support for a mixed format array
johnbillion
left a comment
johnbillion
left a comment
There was a problem hiding this comment.
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.
4 participants
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.