WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 3 months ago

#49102 reviewing feature request

Multisite: removed_user_from_blog hook

Reported by: kevinu Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: minor Version: 5.3.2
Component: Users Keywords: needs-dev-note has-patch needs-refresh
Focuses: multisite Cc:

Description

I useful hook would be something like "removed_user_from_blog". As it is now there is not a way to hook in once a user has been removed from a multisite blog.

There could be many use cases for this such as a custom notice to the user that was removed, doing something with the user after they are removed, etc.

Attachments (2)

0001-Add-removed_user_from_blog-action-hook.patch (1.1 KB) - added by kevinu 11 months ago.
Patch file for changes to add this hook
49102.2.patch (2.6 KB) - added by kevinu 11 months ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
11 months ago

  • Component changed from General to Users

@kevinu
11 months ago

Patch file for changes to add this hook

#2 @kevinu
11 months ago

Hello! I have created a patch to make this change and attached it to this ticket. When a maintainer gets a chance I would love a review.

#3 @SergeyBiryukov
11 months ago

  • Milestone changed from Awaiting Review to 5.4

#4 follow-up: @SergeyBiryukov
11 months ago

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

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

My first thought was that it might be good idea to have a corresponding added_user_to_blog action, however that turned out to be more complicated than I expected.

Looks like there are some inconsistencies between these related functions:

That does not necessarily block introducing a removed_user_from_blog action here, just something to keep in mind.

At a glance, 0001-Add-removed_user_from_blog-action-hook.patch seems OK to proceed.

If we decide to address the discrepancies while we're at it, some options could be:

  1. Introduce added_new_user in add_new_user_to_blog(), for consistency with added_existing_user.
  2. Introduce added_user_to_blog in add_user_to_blog() after the user cache has been cleaned. That would be similar to create_term/created_term actions.
  3. Deprecate both add_user_to_blog and remove_user_from_blog actions and introduce new actions instead. Something like:
    • add_user_to_blog_before
    • add_user_to_blog_after
    • remove_user_from_blog_before
    • remove_user_from_blog_after

I'm leaning to options 1 and 2 at the moment.

Somewhat related: #15173

@kevinu
11 months ago

#5 in reply to: ↑ 4 ; follow-up: @kevinu
11 months ago

Hello @SergeyBiryukov, and thank you for the welcoming and your review!

That makes a lot of sense and I think it should definitely be kept somewhat consistent between these four functions. So I have uploaded 49102.2.patch which includes making the suggested changes 1 & 2.

While working on it I noticed three other discrepancies I wanted to bring up and and if you feel these should be addressed I would be happy to add it into this patch as well.

  1. The already present actions add_user_to_blog and remove_user_from_blog as well as my addition of removed_user_from_blog all use the terms "to_blog" or "from_blog".

Should the action added_existing_user in add_existing_user_to_blog() and added_new_user in add_new_user_to_blog() also share that same

  1. All the actions current and suggested provide the arguments of $user_id and $blog_id, (some providing $role as well) except for added_existing_user. This provides the arguments $user_id and $result.

I feel as though the name added_existing_user already implies a success. Should this be changed to be surrounded in an if-statement and provide $blog_id like the others?

  1. I noticed that in remove_user_from_blog() and it's action remove_user_from_blog the doc-blocks refer to $blog_id being an (int). But it actually comes through as a string.

We could either update the doc-blocks, or type cast it as (int) like we do to $user_id in that same function perhaps?
If this one needs to be tracked in a separate ticket I could do so.

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


11 months ago

#7 @davidbaumwald
10 months ago

  • Keywords needs-dev-note added

With the added actions here, this at least deserves a small call out on the Misc Dev Note, when the time comes.

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


10 months ago

#9 in reply to: ↑ 5 @pbiron
10 months ago

Replying to kevinu:

  1. The already present actions add_user_to_blog and remove_user_from_blog as well as my addition of removed_user_from_blog all use the terms "to_blog" or "from_blog".

Should the action added_existing_user in add_existing_user_to_blog() and added_new_user in add_new_user_to_blog() also share that same

I think it would be good to a new added_existing_user_to_blog action to add_existing_user_to_blog(), but not change the name (or deprecate) the current added_existing_user action.

I'm on the fence about the name of the new added_new_user action in add_new_user_to_blog(). On the one hand, using the same action name as the existing action in add_existing_user_to_blog() makes sense. But if the suggestion above about adding a new added_existing_user_to_blog action is agreed to, then the question becomes: should both actions be added to add_new_user_to_blog()?

  1. All the actions current and suggested provide the arguments of $user_id and $blog_id, (some providing $role as well) except for added_existing_user. This provides the arguments $user_id and $result.

I feel as though the name added_existing_user already implies a success. Should this be changed to be surrounded in an if-statement and provide $blog_id like the others?

For backwards-compat reasons, I don't think it would be good to change that (at least not as part of this ticket...maybe that question deserves a ticket of it's own?).

  1. I noticed that in remove_user_from_blog() and it's action remove_user_from_blog the doc-blocks refer to $blog_id being an (int). But it actually comes through as a string.

We could either update the doc-blocks, or type cast it as (int) like we do to $user_id in that same function perhaps?
If this one needs to be tracked in a separate ticket I could do so.

Really? It should be an int in all the places I find that function called...but I haven't done an exhaustive search. Again, that issue is probably worth another ticket.

A few other comments about 49102.2.patch:

  1. it uses spaces for indenting, and should use tabs
  2. the @since tags for the new actions should just be @since 5.4.0 instead of @since MU (5.4.0). Yes, the current actions have @since MU (3.0.0), but that's because 3.0.0 was also called "MU".
  3. the DocBlocks of the functions the new actions have been added to should get @since 5.4.0 tags that mention the new actions.

#10 @davidbaumwald
10 months ago

  • Milestone changed from 5.4 to Future Release

This ticket still needs a decision, and with 5.4 Beta 1 landing shortly, this is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#11 @Mista-Flo
3 months ago

  • Keywords has-patch needs-refresh added
Note: See TracTickets for help on using tickets.