Make WordPress Core

Opened 3 years ago

Closed 18 months ago

#54572 closed enhancement (duplicate)

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

Reported by: maksimkuzmin's profile maksimkuzmin Owned by: davidbaumwald's profile davidbaumwald
Milestone: Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (1)

54572.diff (5.7 KB) - added by maksimkuzmin 3 years ago.
Provides the proposed functionality as well as tests.

Download all attachments as: .zip

Change History (56)

@maksimkuzmin
3 years ago

Provides the proposed functionality as well as tests.

#1 @sabernhardt
3 years ago

  • Component changed from General to Users
  • Keywords has-patch has-unit-tests added
  • Version trunk deleted

#2 @sabernhardt
3 years ago

  • Component changed from Users to Role/Capability

This ticket was mentioned in Slack in #core by maksimkuzmin. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by sabernhardt. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#6 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 6.0

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.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#7 @audrasjb
3 years ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

#8 @peterwilsoncc
2 years ago

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.

#9 @audrasjb
2 years ago

  • Owner audrasjb deleted

#10 @peterwilsoncc
2 years ago

  • Keywords needs-refresh added

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

#11 @NomNom99
2 years ago

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.

  1. Reload WordPress as admin and add a new user with the Teacher role.
  2. 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' );
  1. 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

This ticket was mentioned in PR #2585 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#12

  • Keywords needs-refresh removed

#13 @peterwilsoncc
2 years ago

  • Keywords needs-refresh added

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

#14 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

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.

#15 @maksimkuzmin
2 years ago

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?

This ticket was mentioned in Slack in #core by maksimkuzmin. View the logs.


2 years ago

#17 @costdev
2 years ago

  • Keywords changes-requested added; needs-refresh removed

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.

#19 @peterwilsoncc
2 years ago

  • Keywords changes-requested removed

I've updated the PR with the requested changes

#20 @mxbclang
2 years ago

  • Focuses performance removed

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

Last edited 2 years ago by mxbclang (previous) (diff)

#21 @davidbaumwald
22 months ago

  • Owner set to davidbaumwald
  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#23 @davidbaumwald
22 months ago

  • Keywords needs-dev-note added

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.

#24 in reply to: ↑ description @manfcarlo
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#25 follow-up: @davidbaumwald
22 months ago

@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?

#26 in reply to: ↑ 25 @manfcarlo
22 months ago

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.

#27 follow-ups: @davidbaumwald
21 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

@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.

#28 in reply to: ↑ 27 @manfcarlo
21 months ago

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?

#29 in reply to: ↑ 27 @manfcarlo
21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#31 follow-up: @knutsp
21 months ago

I then suggest update_role_name( $role ).

#32 in reply to: ↑ 31 @manfcarlo
21 months ago

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?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


21 months ago

#34 @davidbaumwald
21 months ago

  • Keywords dev-feedback added

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

This ticket was mentioned in Slack in #core by chaion07. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


21 months ago

#38 @chaion07
21 months ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


21 months ago

#40 @maksimkuzmin
21 months ago

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!

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

#42 @audrasjb
21 months ago

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.

#43 @peterwilsoncc
21 months ago

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'

#44 @desrosj
21 months ago

  • Keywords commit added

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

#45 @desrosj
21 months ago

  • Keywords needs-dev-note dev-feedback removed

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.

#46 @davidbaumwald
21 months ago

  • Keywords dev-feedback added

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.

#47 follow-up: @SergeyBiryukov
21 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[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.

#48 in reply to: ↑ 47 @knutsp
21 months ago

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 ).

#49 @davidbaumwald
21 months ago

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.

#50 @davidbaumwald
21 months ago

  • Keywords commit dev-reviewed removed

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.

#51 @hellofromTonya
21 months ago

  • Milestone changed from 6.1 to 6.2

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

#52 follow-up: @manfcarlo
18 months ago

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?

This ticket was mentioned in Slack in #core by costdev. View the logs.


18 months ago

#54 in reply to: ↑ 52 @SergeyBiryukov
18 months ago

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.

#55 @davidbaumwald
18 months ago

  • Milestone 6.2 deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.