Make WordPress Core

Opened 2 years ago

Closed 21 months ago

#54164 closed enhancement (fixed)

Consistently fire user role hooks

Reported by: dd32's profile dd32 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.3
Component: Users Keywords: needs-patch
Focuses: Cc:

Description

There are three main user role hooks:

  • add_user_role
  • remove_user_role
  • set_user_role

There are some differences in how these hooks are fired:

  • add_user_role is always fired if $user->add_role() is called with a non-empty role. Whether the user had the role already or not.
  • remove_user_role is only fired if $user->remove_role() is called with a non-empty role AND the user had that role.
  • set_user_role (Which combines add & remove, but doesn't fire their hooks) is only fired if the user had different roles prior to being called. If the role matches what the user already had, it's not.

This leaves with some complicated rules if you want to watch for user role changes.

  • add_user_role firing doesn't mean a user has gained a role.
  • set_user_role may have removed a role.
  • set_user_role may fire without gaining a role.

Attached is a proposed patch that:

  • Doesn't fire add_user_role if the user already had the role.
  • Fires remove_user_role and add_user_role from within $user->set() when appropriate

This standardises the actions that one needs to hook to, being either add_user_role or remove_user_role.

Attachments (3)

54164.diff (2.3 KB) - added by dd32 2 years ago.
54164.2.diff (2.3 KB) - added by dd32 2 years ago.
edgecase, when the user didn't have a role $old_roles would be falsey rather than arrayey
54164.3.diff (2.5 KB) - added by dd32 2 years ago.
consistently set $this->roles to an array, as this is the only place that sets it to an undocumented false value.

Download all attachments as: .zip

Change History (8)

@dd32
2 years ago

@dd32
2 years ago

edgecase, when the user didn't have a role $old_roles would be falsey rather than arrayey

@dd32
2 years ago

consistently set $this->roles to an array, as this is the only place that sets it to an undocumented false value.

#1 @SergeyBiryukov
21 months ago

  • Milestone changed from Awaiting Review to 6.0

#2 follow-up: @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 52823:

Users: Bring some consistency to user role hooks.

This standardizes the actions that one needs to hook to for tracking user role changes:

  • add_user_role is only fired when the user has actually gained a new role.
  • remove_user_role is only fired when the role was actually removed.

Both actions are now fired in WP_User::set_role() as appropriate.

Props dd32, SergeyBiryukov.
Fixes #54164.

#3 in reply to: ↑ 2 @azouamauriac
21 months ago

Replying to SergeyBiryukov:

In 52823

Hi can we merge these two if controle https://prnt.sc/VLGhGiND77NH ?

#4 @peterwilsoncc
21 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as the Multisite tests started failing following this commit.

#5 @SergeyBiryukov
21 months ago

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

In 52824:

Tests: Restore the original user role in the (add|remove)_user_role hooks test.

This makes sure the test does not unintentionally affect other tests.

Follow-up to [52823].

Fixes #54164.

Note: See TracTickets for help on using tickets.