Skip to:
Content

BuddyPress.org

Opened 3 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#8388 closed defect (bug) (fixed)

BP_XProfile_ProfileData::delete_data_for_user() doesn't clear cache

Reported by: dd32 Owned by: dcavins
Milestone: 6.4.0 Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: has-patch dev-feedback
Cc:

Description

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.

Attachments (5)

8388.diff (1.5 KB) - added by dd32 3 weeks ago.
8388.01-w-test.diff (2.0 KB) - added by dcavins 2 weeks ago.
This is dd32's patch but with a test added to track that the change works.
8388.02-w-test.diff (2.9 KB) - added by dcavins 2 weeks ago.
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.
8388.03.diff (2.5 KB) - added by r-a-y 2 weeks ago.
8388.04.diff (1.9 KB) - added by imath 2 weeks ago.

Download all attachments as: .zip

Change History (16)

@dd32
3 weeks ago

#1 @imath
3 weeks ago

  • Milestone changed from Awaiting Review to 6.4.0

Hi @dd32

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

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


3 weeks ago

#3 @dd32
3 weeks ago

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
2 weeks ago

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

@dcavins
2 weeks ago

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.

#4 @dcavins
2 weeks ago

  • Owner set to dcavins
  • Status changed from new to assigned
  • Summary changed from BP_XProfile_ProfileData::delete_data_for_user() doesn't clear cache to `

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!

#5 @dd32
2 weeks ago

  • Summary changed from ` to BP_XProfile_ProfileData::delete_data_for_user() doesn't clear cache

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 :)

#6 @imath
2 weeks ago

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.

#7 @r-a-y
2 weeks ago

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.

@r-a-y
2 weeks ago

@imath
2 weeks ago

#8 @imath
2 weeks ago

  • Keywords dev-feedback added

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.

#9 @dcavins
2 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#10 @r-a-y
2 weeks ago

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!

#11 @imath
2 weeks ago

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).

Note: See TracTickets for help on using tickets.