Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

#4804 closed defect (bug) (fixed)

Activity meta cache not flushed after BP_Activity_Activity::delete_activity_meta_entries()

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 1.8 Priority: low
Severity: minor Version:
Component: Activity Keywords: has-patch 2nd-opinion commit
Cc:

Description

When deleting all of an activity's meta values with BP_Activity_Activity::delete_activity_meta_entries(), the corresponding cache keys are not cleared.

I've attached a suggested strategy for fixing it. It's pretty ugly: because WP's cache API only allows for one level of grouping (which we use for 'bp'), the activity id is stored in the cache key, eg 'bp_activity_meta_24_foo'. That means that, in order to find all of the cached values, we have to examine each cache key. Moreover, the WP Cache API doesn't have a method for getting all cache keys registered to a group, so the only way to get the keys is to examine the global object directly.

FWIW, the same issue is going to arise with other database methods that clear lots of metadata in one query.

If we don't like the strategy in the attached patch, another option is to refactor methods like delete_activity_meta_entries() so that they work like this:

$meta_keys = $wpdb->get_col( $wpdb->prepare( "SELECT meta_key FROM {$bp->activity->table_name_meta} WHERE activity_id = %d", $activity_id ) );
foreach ( $meta_keys as $meta_key ) {
    bp_activity_delete_meta( $activity_id, $meta_key );
}

In other words, go through and delete each item one-by-one. That way, the cache will be deleted properly each time (easy to do when we have the meta key). The disadvantage is that this requires more queries, but from a practical point of view, does this matter? The method is only called in BP when activity items are deleted, and the UI only allows for up to 20 items to be deleted at once. So we might be looking at a couple dozen queries, but only in the admin, and only occasionally.

It's probably not urgent to fix this, because there's not much harm in the data hanging around in the cache after the activity item has been deleted - what would you do with metadata for a deleted item, after all? But if people start using this method to do other kinds of funny business, the bug will become more problematic.

Attachments (1)

4804.patch (1.5 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (4)

@boonebgorges
8 years ago

#1 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to Future Release

#4765 was marked as a duplicate.

#2 @DJPaul
7 years ago

  • Keywords commit added
  • Milestone changed from Future Release to 1.8

We should commit this in 1.8, because both you and I seem to have run into this issue in the real world :)

#3 @boonebgorges
7 years ago

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

In 7051:

When deleting an activity item, clear its metadata cache

Because of the way that the cache groups are structured, there is no simple
way to figure out which cache keys should be dropped upon deletion. Our
solution is to scan through all the keys in the 'bp' group to determine the
relevant ones. It's not particularly elegant, but this kind of cache-busting
will happen rarely (namely, when an activity item is deleted) so the overhead
is negligible.

Fixes #4804

Note: See TracTickets for help on using tickets.