Activity meta data not deleted for the comments of an activity when the activity is deleted (bloating the meta table with orphans)

[dtc7240]

Create an activity with a comment and a comment to the comment, making sure that some meta data exists for each (Askimet entries dutifully suffice).

If you delete the first comment, the meta data for it and its child are correctly removed from the database, as bp_activity_delete_comment recursively calls bp_activity_delete from the bottom comment up, one at a time.

If however, you delete the activity itself, the meta data for both comments remain in the wp_bp_activity_meta table (and any custom table using the bp_activity_deleted_activities hook), bloating the meta table(s) with orphans.

In this case, when bp_activity_delete calls BP_Activity_Activity::delete, the value of $activity_ids only ever contains the id of the activity itself, instead of adding the ids of the child comments it found when calling BP_Activity_Activity::delete_activity_item_comments( $activity_ids ). Thus, when it next calls BP_Activity_Activity::delete_activity_meta_entries( $activity_ids ), only the meta data for the activity itself is removed.

I suggest we change this existing function within class BP_Activity_Activity:

function delete_activity_item_comments( $activity_ids = array() ) {
	global $bp, $wpdb;

	$activity_ids = implode( ',', wp_parse_id_list( $activity_ids ) );

	return $wpdb->query( "DELETE FROM {$bp->activity->table_name} WHERE type = 'activity_comment' AND item_id IN ({$activity_ids})" );
}

to the following in order to add the comment ids to $activity_ids:

function delete_activity_item_comments( &$activity_ids = array() ) {
	global $bp, $wpdb;

	$activity_id_list = implode( ',', wp_parse_id_list( $activity_ids ) );

	$activity_ids = array_merge( $activity_ids, $wpdb->get_col( "SELECT id FROM {$bp->activity->table_name} WHERE type = 'activity_comment' AND item_id IN ({$activity_id_list})" ));

	return $wpdb->query( "DELETE FROM {$bp->activity->table_name} WHERE type = 'activity_comment' AND item_id IN ({$activity_id_list})" );
}

There may be a more efficient way to handle the DELETE in order to keep from running essentially the same query twice (that someone more experienced than me might implement).

I've tested this fix in my own development application, and it works great. Even for the custom meta table I've created to service both the activity and message tables with my own specific typing across both forms of communication ( utilizing the bp_activity_deleted_activities hook).

Thank you,
Scott Seitz

[johnjamesjacoby]

Hi Scott,

Thanks for this. I'm able to confirm what you're seeing. Your fix looks good at a cursory.

Moving to 1.9 and marking as early, to get this into trunk to soak.

[dtc7240]

Thanks John. I just moved to 1.8 today and had to remember to replace it manually. I appreciate you getting it in so quickly!

[r-a-y]

01.patch addresses the issue of deleting activity meta for activity comments.

I've chosen to move the deletion of activity comments and meta entries from the BP_Activity_Activity::delete() method to the 'bp_activity_deleted_activities' hook.

Patch also adds a unit test and passes when the patch is applied. It fails when the patch isn't applied.

The negative with this patch is if someone is directly calling BP_Activity_Activity::delete(), their activity comments and meta entries will not be deleted. A way to remedy this is to remove my bp_activity_delete_activity_comments_and_meta() function and move that code directly into the static delete() method. Either approach would fix this issue.

[boonebgorges]

The logic in the patch looks good to me, r-a-y.

For consistency's sake, I think it makes sense to do this in the BP_Activity_Activity class, rather than in a procedural function. I think putting it right into the delete() method is probably fine.

[r-a-y]

As I was writing up yesterday's Trac comment for 01.patch, I realized the error of my ways ;)

02.patch handles activity comment deletion directly in the delete() method.

Patch also adds phpDoc and adds a second parameter to the delete_activity_item_comments() method to clear the activity comment meta cache. Even though this method is no longer used, I felt that we should fix this anyway.

[boonebgorges]

Nice, I think this looks good, except for the use of the word 'cache' in delete_activity_item_comments() - it's not really the activity meta cache you're deleting (we do that elsewhere), but it's the activity meta itself.

[r-a-y]

In 7422:

Delete activity comment meta when deleting parent activity items.

Previously, when deleting parent activity items, BP would delete the
related activity comments, but not the activity comment metadata.

To solve this, activity comments are now queried and removed in the
main BP_Activity_Activity::delete() method and the activity comment
IDs are now properly passed to the delete_activity_meta_entries()
method.

Commit also includes a unit test for this issue.

Hat-tip boonebgorges for feedback.

Fixes #5097.