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

[boonebgorges]

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 ​https://core.trac.wordpress.org/ticket/12720, ​https://core.trac.wordpress.org/ticket/32796

In many places, BuddyPress performs data cleanup/deletion on user delete. This generally takes the form seen at ​https://buddypress.trac.wordpress.org/browser/tags/5.0.0/src/bp-xprofile/bp-xprofile-functions.php?marks=895,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.

[boonebgorges]

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(). ​https://core.trac.wordpress.org/browser/tags/5.3/src/wp-admin/includes/user.php?marks=422#L411

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.

[johnjamesjacoby]

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?

[imath]

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: ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-members/bp-members-cache.php#L59

[boonebgorges]

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.

[imath]

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 😊

[boonebgorges]

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

[boonebgorges]

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.