Skip to:

Opened 4 years ago

Closed 4 years ago

#8175 closed defect (bug) (fixed)

Calling `wp_delete_user()` on multisite should not trigger data deletion

Reported by: boonebgorges's profile boonebgorges Owned by: boonebgorges's profile boonebgorges
Milestone: 6.0.0 Priority: normal
Severity: normal Version:
Component: Members Keywords: 2nd-opinion has-patch dev-feedback


On non-multisite, wp_delete_user() does what it says: it deletes the user from the database (and reassigns post authorship). On multisite, the correct way to remove a user from the system altogether is to call wpmu_delete_user().

When calling wp_delete_user() on *multisite*, the function does the following:

  1. Reassigns post authorship
  2. Calls remove_user_from_blog()

As such, the Multisite semantics of wp_delete_user() are, arguably, incorrect. See,

In many places, BuddyPress performs data cleanup/deletion on user delete. This generally takes the form seen at,896#L883: hooking the same function to the wpmu_delete_user and delete_user hooks.

This behavior feels quite incorrect in the case of Multisite wp_delete_user(). Developers who use wp_delete_user() to remove a user from a Multisite subsite would not, presumably, expect the user's BuddyPress data to be deleted. BP data should only be deleted when the user account is removed from the system altogether.

Attachments (3)

8175.diff (3.4 KB) - added by boonebgorges 4 years ago.
8175.2.diff (28.9 KB) - added by boonebgorges 4 years ago.
8175.3.diff (29.1 KB) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (10)

4 years ago

#1 @boonebgorges
4 years ago

8175.diff shows how we might address this problem in the case of XProfile. Other components would work similarly. I've chosen to create a new callback for the 'delete_user' hook, where we do an is_multisite() check. In this way, we directly parallel the is_multisite() check in wp_delete_user().

Two questions for the group:

  1. Do others agree that this is a bug that BP should address? (The semantics in WP are a mess, but we can only control what we can control.)
  2. Do others agree that 8175.diff is a decent approach? I can think of a couple other strategies but they're not as clear as this one.

#2 @johnjamesjacoby
4 years ago

I see what you mean.

I wish we had our own functions for deleting users, but at the same time I know that would just be a third confusing alternative. Eventually they’d all need to boil down into using the WP core function anyway, so I think your patch is on the right track.

How do you feel about swapping that is_multisite() call for a custom function with a filter hook in it? bp_should_delete_user_data or something better named… and we can use its internals for these environment nuances? Maybe someone would want to restore the old way, or maybe we discover some other new relative problem. It might be nice to have a Core function/hook to contain that?

#3 @imath
4 years ago

  • Milestone changed from Awaiting Review to 6.0.0

Thanks for your work on this @boonebgorges

The strategy looks good to me. I'm just wondering if the same stratgy should be used to delete the cache there:

#4 @boonebgorges
4 years ago

  • Keywords has-patch added

Thanks to both of you for your feedback.

@johnjamesjacoby A wrapper function seems reasonable to me. The obvious situation where it might be valuable is in multi-network, when you may well want wp_delete_user() to remove (that network's) user data. I chose the function name and signature bp_remove_user_data_on_delete_user_hook( $data_type, $user_id ), so that plugins can filter on a per-component or per-user basis if they'd like. See the function definition in 8175.2.diff bp-members-functions.php

@imath Thanks for reviewing. IMO cache invalidation is the opposite of data deletion, in the sense that we want to err on the side of NOT deleting data, while we should err on the side of TOO MUCH invalidation. Put another way, it seems reasonable to me that wp_delete_user() on multisite would trigger a cache flush, even if it doesn't delete data, just out of caution.

Apologies for the very large 8175.2.diff. Most of it is tests, and the pattern is the same across the components. I'd mainly like a last sanity check on the general approach, as well as any feedback on function names.

4 years ago

4 years ago

#5 @imath
4 years ago

  • Keywords dev-feedback added

Hi @boonebgorges

8175.3.diff is a refreshed version of your patch + I've added $this->setExpectedDeprecated( 'wpmu_new_blog' ) for 2 tests as they were failing otherwise.

Do you think we can commit this soon? I'd like to package 6.0.0 before the end of March 2020 😊

#6 @boonebgorges
4 years ago

Thanks for the ping. I'll review and commit in the upcoming days.

#7 @boonebgorges
4 years ago

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

In 12605:

Don't delete user data on delete_user hook on Multisite.

The new function bp_remove_user_data_on_delete_user_hook(), which defaults
to false on Multisite and true otherwise, helps BuddyPress to differentiate
between the use of wp_delete_user() to delete a user account from an
installation (typical on non-Multisite) and its use to remove a user from
a site in a Multisite instance.

Props imath.

Fixes #8175.

Note: See TracTickets for help on using tickets.