Add a function for updating the existing role instead of removing, then adding one

[maksimkuzmin]

While I was working on and developing multiple themes and plugins, I often needed to update the existing role after using add_role function.

Say, I already have a role picture_admin. I might want to alter its display name, or capabilities as an array, or both at once. What I'm forced to do now is this (not taking peculiar ways of editing wp_options table in account):

<?php

remove_role( 'picture_admin' );
add_role( 'picture_admin', $display_name, $capabilities );

This code has multiple issues. First, I'll have to perform double query to MySQL database, which affects performance and doesn't feel right to me. Second, I won't be able to alter just one field as easily with one function, I need to carry out the old variable for either display name or capabilities every time I call this statement.

Instead, I propose having a neat way of easily updating roles via the following statement:

<?php

update_role( $role_name, 'New Display Name', $caps );
// or
update_role( $role_name, null, $caps );
// or
update_role( $role_name, 'New Display Name' );

Which also creates a role if it doesn't exist already.

[maksimkuzmin]

Provides the proposed functionality as well as tests.

[SergeyBiryukov]

Hi there, welcome to WordPress Trac! Thanks for the ticket and the patch.

The idea makes sense to me at a glance, and it's great that unit tests are also included. Moving to 6.0 for review.

[peterwilsoncc]

Thanks for the patch, @maksimkuzmin

Based on a quick review, I have a few suggestions for improving your patch:

• update_role() -- function's display name parameter can default to null to match the WP_Roles::update_role() method
• It would be good to split the test up a bit. The first time an assertion fails in a test PHP Unit stops running the test (it knows it's failed). In this case, if the code to update the display names breaks, it would be handy to know whether the capabilities update code is working too.
• The empty( $role ) could be replaced by something more precise (ask yourself what you wish to test for). At the moment it could pass with [ 54572 ] which no body wants. I noticed this comes from add_role so it's possible that WordPress is stuck with it.

The patch looks fairly close, thank you, it's a little tidying around the edges.

Also, feel free to use a pull request on GitHub if you will find that easier, you'll just need to post a link to this ticket in the description. Most committers use the modern tools as much as possible.

[peterwilsoncc]

Marking this as needing a refresh, per my comments above.

[NomNom99]

Test Report

Environment

WordPress: 6.0-alpha-52448-src
Browser: Chrome, version 100.0.4896.75 (Official Build) (x86_64)
Operating System: macOS Big Sur 11.6

Steps to test

1. Clone WordPress-develop using git@github.com:WordPress/wordpress-develop.git
2. Setup WordPress using ​this instruction
3. Pull the changes of this patch using the command npm run grunt patch:54572
4. Open the functions.php file of the active theme.
5. Copy-paste the following snippet in the functions.php file.

function ticket_54572() {
    add_role(
        'school_staff',
        'Teacher',
        get_role( 'contributor' )->capabilities
    );
}
add_action( 'init', 'ticket_54572' );

This will add a Teacher role with capabilities of a Contributor.

6. Reload WordPress as admin and add a new user with the Teacher role.
7. Replace the above copy-pasted with the following:

function ticket_54572() {
    update_role(
        'school_staff',
        'Principal',
        get_role( 'administrator' )->capabilities
    );
}
add_action( 'init', 'ticket_54572' );

8. Reload the WordPress site and observe that the name of the school_staff role is renamed from Teacher to Principal and the capabilities have changed from that of a Contributor to an Administrator.

Results

• The patch is working as intended

https://core.trac.wordpress.org/ticket/54572

[peterwilsoncc]

In the ​linked pull request, I've added a little tidy up to 54572.diff​ :

• split unit tests to different types of update
• added a test to ensure user_can() works as expected after a role update
• modified the docblocks
• matched the functions arguments to the methods arguments
• left the empty( $role ) check as is, it's used as an array key only so that should keep the values sensible

[costdev]

As 6.0 Beta 1 was released on Tuesday, I'm moving this ticket to the 6.1 milestone.

I have also sent some suggested changes to Peter to help move this forward so that it might be committed early in the 6.1 cycle.

[maksimkuzmin]

Thank you everyone for making this function even better!

@peterwilsoncc the updates look great to me.

Is there something needs to be done before adding to 6.1?

[costdev]

From what I can see, the PR just needs a confidence check on one of the assertions and the @since annotations added.

Removing needs-refresh as there are no conflicts.

Adding changes-requested to indicate the current state of the patch.

I've pushed a few changes:

• ​https://github.com/WordPress/wordpress-develop/pull/2585/commits/9e9349cf8a16ae02098bfe63d9ec4d6f9f2a3cda: Since tags
• ​https://github.com/WordPress/wordpress-develop/pull/2585/commits/804e988ee8e5e5a4ac5ee5dfc834dfd4efa8859c Fixed the broken test
• ​https://github.com/WordPress/wordpress-develop/pull/2585/commits/f13d2e5e1a0de6a9011416032ffd6b989e59dfcf renamed unhappy path test to follow the convention of using data providers with a data_ prefix replacing test_

[peterwilsoncc]

I've updated the PR with the requested changes

[mxbclang]

Removing the performance focus per discussion in the 3 Aug performance bug scrub.

[davidbaumwald]

In 54213:

Role/Capability: Add a new update_role function.

Until now, changing a user's role involved deleting a user's role then re-adding. This change creates a new update_role function and associated method in WP_Roles to consolidate this process.

This commit also introduces new unit tests around update_role and adds additional "unhappy path" tests for roles and capabilities in general.

Props maksimkuzmin, peterwilsoncc, NomNom99, costdev, SergeyBiryukov.
Fixes #54572.

Merged into core in https://core.trac.wordpress.org/changeset/54213.

[davidbaumwald]

While this probably doesn't need a full, separate Dev Note, this new function deserves at least a call-out on the Misc Dev Note.

[manfcarlo]

Replying to maksimkuzmin:

First, I'll have to perform double query to MySQL database, which affects performance and doesn't feel right to me. Second, I won't be able to alter just one field as easily with one function, I need to carry out the old variable for either display name or capabilities every time I call this statement.

The new method only reads and modifies public instance variables, so you could include and run it in your own plugin classes without the need to make it available in core. You can still save the extra database query by running the three unset statements rather than calling the remove_role method. I don't see any inconvenience or difficulty with carrying through the old variable.

It also has some errors with reading array keys that may not be set. Line 220 reads $roles[ $role ] without checking if it's set, and line 233 reads $role_objects[ $role ] without checking if it's set. It can't be assumed that one must be set if the other is set.

Incidentally, line 233 won't actually run because $capabilities was already set earlier in the method, but it may end up running if other parts of the method are re-written later.

Similarly, the call to the add_role method on line 242 appears to wrongly assume that if $roles[ $role ] was not set, that the role did not exist and is being newly added rather than updated.

Line 220 is the most serious problem and will introduce PHP "undefined offset" errors, which I think justifies re-opening the ticket, but I would personally argue the method is not needed at all for the reasons given in my first paragraph.

[davidbaumwald]

@manfcarlo Thanks for the feedback. I think we can do some more hardening as you suggest. Would you like to upload a patch for the isset checks?

[manfcarlo]

Replying to davidbaumwald:

@manfcarlo Thanks for the feedback. I think we can do some more hardening as you suggest. Would you like to upload a patch for the isset checks?

I think that would be sensible if the method had already been released. Since it's been committed but not released yet, I would rather suggest that the whole method be reverted and removed.

The method that was committed basically consists of a few public variables being unset and an existing method being called, preceded by a series of defensive validation checks that would not be needed if the plugin author knows what role they want to update and how.

Even if an update method is deemed justified, I think the validation checks are overkill and it should be easy enough for the plugin author to supply the correct arguments. Besides, supplying null as the display name looks very odd and does not clearly communicate an intent to leave it unchanged.

For something that is so easily achieved without an addition to core, the method and particularly the defensive validation just add disproportionate overhead for core contributors. The method was missing for more than a decade and no one said anything, and we now know that any feature that does not directly deal with blocks is destined to be a legacy feature. I am surprised the suggestion made it this far with so little objection.

[davidbaumwald]

@manfcarlo After thinking about this, I think the merged code is OK for now.

Justification

I think adding this will be great for new developers, even if long-time developers implemented their own update_role functionality. I think this is one instance where core can lead the way and provide a needed method to reduce the burden on developers. As you mention, this is mainly an alias with the unset work already being done.

Validation Checks

A lot of the validation checks are just the way core does things. Countless functions in core accept a variety of input parameters then validate and convert them to a single type for processing inside the function. Also, with the changes in PHP 8 type checking is all the more important.

As it stands now, this will ship with 6.1. If you would like to see specific improvements, please file a follow-up ticket. If they are bugs, they can even be addressed in 6.1.1.

[manfcarlo]

Replying to davidbaumwald:

A lot of the validation checks are just the way core does things. Countless functions in core accept a variety of input parameters then validate and convert them to a single type for processing inside the function. Also, with the changes in PHP 8 type checking is all the more important.

The type checking (e.g. is_string) is fine. It is the explicit allowing of null to indicate the intention to leave unchanged that introduces the PHP "undefined offset" errors. I also think it is not very expressive and offers little benefit in this particular case.

<?php
update_role( $role_name, null, $caps );

Rather than talking vaguely about "countless functions in core," can you point to a specific precedent that allows the same pattern?

If not, what about keeping the method but requiring all three parameters to be supplied as the correct types?

[manfcarlo]

I feel a little bit silly that I didn't notice earlier, but there already exist ​add_cap and ​remove_cap methods for updating the capabilities of a role.

It seems that using the aforementioned methods is the officially supported way of updating the capabilities of a role, not the workaround of removing and re-adding the role that was described in the ticket description. It also probably explains why an update method was missing for so long with no request for it to be added.

Replying to davidbaumwald:

I think adding this will be great for new developers

Confusion about why there are two ways to do the same thing, and which one to use, is not great for new developers. There might be a good reason it was implemented the way it was.

If adding or removing multiple capabilities, using the older methods causes multiple database queries to achieve what is possible with just one. However, it is mentioned in the ​remove_cap documentation that the method should only be called once, so the performance cost is negligible. I think it's also highly questionable why a role that is already assigned to users should be re-defined from scratch beyond the incremental tweaks that the older methods are suitable for.

I previously did not want to re-open the ticket a third time, for the sake of being co-operative, but I think it is justified given that the existence of the older methods was previously undiscussed. Sorry for the hassle and I would have brought it up earlier if I had noticed.

My new suggestion is to designate the update_role method only for updating the display name. The docblock for the update_role method could indicate that the add_cap and remove_cap methods are to be used for updating the capabilities of a role.

Trac ticket: https://core.trac.wordpress.org/ticket/54572

[knutsp]

I then suggest update_role_name( $role ).

[manfcarlo]

Replying to knutsp:

I then suggest update_role_name( $role ).

I did consider something like update_role_name or rename_role, but using update_role gives an opportunity to use the docblock to direct people to the other methods if they are looking for a way to update the capabilities of a role.

Also, I don't know whether there's enough time to include the change in 6.1, so in the event that update_role makes it in, the third parameter could just be deprecated rather than the whole function.

Advice?

[davidbaumwald]

I'd like to get @SergeyBiryukov's thoughts on any improvements we can make here and if any are feasible before 6.1 RC2.

[chaion07]

Thanks @maksimkuzmin for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are happy to sit this one out a bit more considering the following:

1. @davidbaumwald is awaiting a feedback from @SergeyBiryukov for a better naming.
2. @peterwilsoncc had some interest on this as well.
3. Stating that there are no objections yet.

Cheers!

Props to @desrosj @davidbaumwald for the discussion.

[maksimkuzmin]

Thank you @chaion07! I and some other interested parties are looking forward to seeing this improvement in production at last.

As the old saying teaches, measure seven times, cut once. I hope that all the objections and propositions will be tackled comprehensively and successfully.

My gratitude to all who devoted their time to this ticket!

[audrasjb]

As per today's bugscrub:
Adding high priority. Let's see if it can be reviewed in time for RC3. If not, this ticket should be punted to 6.1.1.

[peterwilsoncc]

Discussing this with @davidbaumwald, we think it's worth reverting [54213] and coming back to this in another major release.

No patch as it's an SVN command for a reverse merge (revert): svn merge -c -54213 '^/trunk'

[desrosj]

Marking commit as this is ready for a committer to revert.

[desrosj]

Also removing needs-dev-note since this will no longer be included in 6.1, and removing dev-feedback so that it can be re-added after revert for a review prior to backport.

[davidbaumwald]

Missed the ticket # in the revert, but here it is.

In 54673:

Role/Capability: Revert the newly added update_role function for 6.1.

Based on feedback, this enhancement isn't quite ready. Reverting [54213] for now to continue the work in the next cycle.

Follow-up to [54213].

Props manfcarlo, peterwilsoncc.

Also tagging for a secondary review to revert from the 6.1 branch.

[SergeyBiryukov]

[54673] looks good to apply to the 6.1 branch.

I'm not sure yet what the right solution would be here, but I think comment:29 does bring up a good point that existing methods can be used for this.

Also, looking at the ticket description:

Say, I already have a role picture_admin. I might want to alter its display name, or capabilities as an array, or both at once. What I'm forced to do now is this (not taking peculiar ways of editing wp_options table in account):

<?php

remove_role( 'picture_admin' );
add_role( 'picture_admin', $display_name, $capabilities );

I might be missing something, but wouldn't add_role() work for updating the role without removing it first? Based on a brief reading of WP_Roles::add_role(), it seems like it should just overwrite the existing role definition.

[knutsp]

Replying to SergeyBiryukov:

I might be missing something, but wouldn't add_role() work for updating the role without removing it first? Based on a brief reading of WP_Roles::add_role(), it seems like it should just overwrite the existing role definition.

Fine. Just wanting to change the name of an existing role should have an inutitive name and be simple, like update_role_name( $role ).

[davidbaumwald]

In 54683:

Role/Capability: Revert the newly added update_role function for 6.1.

Based on feedback, this enhancement isn't quite ready. Reverting [54213] for now to continue the work in the next cycle.

Follow-up to [54213].

Props manfcarlo, peterwilsoncc, SergeyBiryukov.
Reviewed by SergeyBiryukov.
Reverts [54213] in the 6.1 branch.
See #54572.

[davidbaumwald]

Cleaning up the keywords after the revert in the 6.1 branch.

Thanks everyone who's contributed to this so far. We'll take some more time in the next cycle to refine this.

[hellofromTonya]

With the work being reverted/removed from 6.1, moving this ticket to the next major release cycle.

[manfcarlo]

If there's agreement that the new function should be limited to renaming the role, what about closing this ticket as a duplicate of #40320 and iterating on the patch that was added over there?

[SergeyBiryukov]

Replying to manfcarlo:

If there's agreement that the new function should be limited to renaming the role, what about closing this ticket as a duplicate of #40320 and iterating on the patch that was added over there?

Indeed, that makes sense to me.

[davidbaumwald]

@manfcarlo Closing this ticket as a dupe of #40320. Thanks to everyone who contributed here, and please continue the work on that ticket.