WP_User::add_role() runs even if the added role is already set

[bhujagendra]

When adding a role using WP_User::add_role() the function is run even though the role is already set for the given user. Hence, the also action add_user_role is fired when it should not.

This behavior is different from WP_User::remove_role() that exits the function if the role is not currently set, and hence the action remove_user_role does not fire - as one would expect.

An easy fix would be to add in_array( $role, $this->roles, true ) to the test in class-wp-user.php on Line 540 resulting in the following:

if ( empty( $role ) || in_array( $role, $this->roles, true ) ) {

Not sure if there are any backward-compatibility issues with this solution, as the action would not be called anymore in situations where previously it was called. A solution could be to run the action nonetheless, which would, however, kinda obsolete the fix. Another solution could be to introduce a new action, e.g. add_user_role_requested:

if ( empty( $role ) ) {
	return;
}

/**
 * Fires immediately after the user has been given a new role.
 *
 * @since x.x.x
 *
 * @param int    $user_id The user ID.
 * @param string $role    The new role.
 */
do_action( 'add_user_role_requested', $this->ID, $role );

if ( in_array( $role, $this->roles, true ) ) {
    return;
}

This filter uses the same syntax, than the existing add_user_role. I personally would prefer to pass the user object ($this) rather than the user ID. Maybe the user object could be passed as a third argument to both actions.

[bhujagendra]

Happy to create a PR if appreciated. Would be good to know which way to go.

[desrosj]

Hey @bhujagendra,

Welcome to Trac! Thanks for this ticket.

This seems sensible to me! I'd like to allow for more feedback just in case there's something I'm missing.

The only backwards compatibility issue I can think of would be if a plugin or theme depends on add_user_role firing every time. But that seems super edge case to me. This is a reasonable correction, though, and we could make sure to mention it in developer notes before a release.

I'm not totally sold on the need for a new filter, but I can see some potential use cases. That said, I think pre_add_user_role would better fit the naming convention found elsewhere in Core. An equivalent on in remove_role() should also be introduced if it's decided to add one.

Changing the Version to 2.0, as that's when WP_User->add_role() was introduced, and it's been mostly unchanged since. In Trac, Version is the first version of WordPress affected by an issue.

• Fixing a "bug" where add_user_role would be fired, even if the user already has that role
• Introduces two new filters (pre_add_user_role and pre_remove_user_role) to prevent or change the role being added or removed respectively.

Trac ticket: 53199

• Fixing a "bug" where add_user_role would be fired, even if the user already has that role
• Introduces two new filters (pre_add_user_role and pre_remove_user_role) to prevent or change the role being added or removed respectively.

Trac ticket: 53199

[bhujagendra]

Hi @desrosj

Thanks for looking into this and the info regarding the version on trac.

Yes, I too see it as an edge-case where someone would rely on `add_user_role? firing every time. And I agree, introducing a new hook that will probably never be used. I just wanted to mention it and an idea for a solution, as it might help others to think of a unwanted side-effect.

However, introducing two new filters instead, to filter the role, that might make more sense. By this, a plugin could prevent a given user been given a certain role, or a certain role to be removed. The filters could then look something along the lines of the following:

/**
 * Filters the role-to-be-added  to a user. Return an empty string to prevent the role being added.
 *
 * @since x.x.x
 *
 * @param string  $role          The new role.
 * @param WP_User $user          The user object
 * @param string  $original_role The new role (as originally requested).
 */
$role = apply_filters( 'pre_add_user_role', $role, $this );

and

/**
 * Filters the role-to-be-removed from a user. Return an empty string to prevent the role being added.
 *
 * @since x.x.x
 *
 * @param string  $role          The role to be removed.
 * @param WP_User $user          The user object
 * @param string  $original_role The role to be removed (as originally requested).
 */
$role = apply_filters( 'pre_remove_user_role', $role, $this, $role );

So the whole thing would look something like this: ​https://github.com/WordPress/wordpress-develop/pull/1254

Open for comments. Happy to make amendments.

Peace,
Bhujagendra.