Skip to:
Content

BuddyPress.org

Opened 3 days ago

Last modified 15 hours ago

#8175 new defect (bug)

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

Reported by: boonebgorges Owned by:
Milestone: 6.0.0 Priority: normal
Severity: normal Version:
Component: Members Keywords: 2nd-opinion has-patch
Cc:

Description

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.

Attachments (2)

8175.diff (3.4 KB) - added by boonebgorges 3 days ago.
8175.2.diff (28.9 KB) - added by boonebgorges 15 hours ago.

Download all attachments as: .zip

Change History (6)

@boonebgorges
3 days ago

#1 @boonebgorges
3 days 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(). 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.

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

#4 @boonebgorges
15 hours 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.

@boonebgorges
15 hours ago

Note: See TracTickets for help on using tickets.