BP_XProfile_ProfileData::delete_data_for_user() doesn't clear cache

[dd32]

When calling BP_XProfile_ProfileData::delete_data_for_user() via xprofile_remove_data() the data is removed from the database, but the data isn't removed from the object cache.

This results in the delete being successful, but the UI continuing to display the data.

Attached patch is a very rough attempt at adding.

[imath]

Hi @dd32

Thanks a lot for the report and the patch. Let's try to fix it asap (6.4.0).

[dd32]

An alternative approach would be to call BP_XProfile_ProfileData::delete() for each individual user/field pair which processes the cache evictions via the actions there.

​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-xprofile/classes/class-bp-xprofile-profiledata.php#L273

There's also BP_XProfile_ProfileData:: delete_for_field() which also doesn't do the cache evictions
​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-xprofile/classes/class-bp-xprofile-profiledata.php#L633

[dcavins]

This is dd32's patch but with a test added to track that the change works.

[dcavins]

This patch goes a step further and avoids using the bulk delete statement, instead using xprofile_delete_field_data() so that the before and after actions are called.

[dcavins]

Thanks for your patch, @dd32! I've added a test showing that your patch works and also added a second patch that avoids the troublesome bulk delete (the cached objects aren't cleared because the before and after delete hooks aren't called, so what else isn't getting done in the bulk version?)

I'd be happy to hear your comments, @dd32, @imath @johnjamesjacoby on what you feel is the best way to handle this. For reference, with profile groups and fields, we use an incremented cache so it's easy to invalidate a large swath of cached items, but the profile field data for a user is cached without an incrementor.

Thanks!

[dd32]

Thanks for running with it @dcavins!

While I don't really like using a group incrementor for caches, I can understand that sometimes it might be the best method for invalidations like this.
I guess it all comes down to the expected operations, if it's anticipated that it's more common to delete the data for many users at once then a group incrementor is useful, if it's anticipated that it's usually just one user, then individual cache deletions might be more reasonable.

I'm not familiar enough with how BuddyPress is used, I would normally assume that bulk deletions aren't a thing, but I guess there's some use-cases that would prove me wrong :)

[imath]

Thanks for working on this @dcavins 💪.

If you go with version 2 of the patch, then BP_XProfile_ProfileData::delete_data_for_user() is not used anymore and we fire hooks that were not fired when a user was completely removed or marked as spam so far. So I believe I'd go with version 1 of the patch.

[r-a-y]

I like version 2 for the reason dcavins mentioned about the 'xprofile_data_before_delete' and 'xprofile_data_after_delete' hooks actually being called during profile data deletion.

In version 2, I think dcavins meant to replace BP_XProfile_ProfileData::delete_data_for_user() with xprofile_remove_data() though. So that's what I did in 03.diff. Also currently, BP_XProfile_ProfileData::delete_data_for_user() expects an integer of deleted rows if successful so I've added this to the xprofile_remove_data() function. This doesn't account for a false return value though. If we don't care about the return value, remove the count() statement.

[imath]

hi @r-a-y

Thanks a lot for your feedback, I'm fine with choosing version 2. That being said I'm a bit concerned about having a class method that we don't use anymore in our code. BP_XProfile_ProfileData::delete_data_for_user is only used into xprofile_remove_data(). So If you want to go with 8388.03.diff, then BP_XProfile_ProfileData::delete_data_for_user should probably be deprecated.

In ​8388.04.diff, I'm suggesting another alternative to avoid deprecating the method (in a minor release, I'm unsure it's a good idea) and to keep DB stuff into BP_XProfile_ProfileData::delete_data_for_user.

[dcavins]

In 12781:

Ensure cached data is cleared in BP_XProfile_ProfileData::delete_data_for_user().

Data was being deleted in bulk using a direct SQL
query, bypassing the xprofile_data_before_delete
and xprofile_data_after_delete actions, resulting in
stale data persisting in the xprofile cache. This
change uses xprofile_delete_field_data() to delete
the user's data so that the action hooks are called.

Props dd32, r-a-y, imath.

Fixes #8388.

[r-a-y]

In 8388.04.diff, I'm suggesting another alternative to avoid deprecating the method (in a minor release, I'm unsure it's a good idea) and to keep DB stuff into BP_XProfile_ProfileData::delete_data_for_user.

That works for me. Thanks all!

[imath]

In 12785:

Ensure cached data is cleared in BP_XProfile_ProfileData::delete_data_for_user().

Data was being deleted in bulk using a direct SQL query, bypassing the xprofile_data_before_delete and xprofile_data_after_delete actions, resulting in stale data persisting in the xprofile cache. This change uses xprofile_delete_field_data() to delete the user's data so that the action hooks are called.

Props dd32, r-a-y, dcavins.

See #8388 (branch 6.0).