Blog data stored in activitymeta and blogmeta must be invalidated

[boonebgorges]

The new activity action schema in BP 2.0 requires that some blog-specific content be stored in blogmeta and activitymeta (to avoid switch_to_blog() during the loop). See ​https://buddypress.trac.wordpress.org/browser/tags/2.0/bp-blogs/bp-blogs-activity.php#L93 and ​https://buddypress.trac.wordpress.org/browser/tags/2.0/bp-blogs/bp-blogs-activity.php#L154

At the moment, we do a bunch of invalidation for the 'url' and 'blogname' values cached in blogmeta. See ​https://buddypress.trac.wordpress.org/browser/tags/2.0/bp-blogs/bp-blogs-functions.php#L237

We do not do any corresponding validation for the 'post_url' and 'post_title' activitymeta values. We should. This will involve identifying all the times when a post URL or title could change (save_post is a good place to start, but see also permalink changes, etc).

[r-a-y]

I think only the 'post_title' needs updating as the 'post_url' uses the post shortlink, which never changes.

When we delete a blog post, we should already be deleting its corresponding activity item, which should delete activity meta at the same time.

Here's a quick patch.

Update: I jumped the gun. We do need to cache the 'post_url' for blog comments. Patch is updated to reflect this.

[r-a-y]

So this ended up being more of a pain than I envisioned.

The main source of this pain was during an update of a blog post and having to update the 'post_title' activity meta for each connecting activity item tied to a blog post comment.

boonebgorges mentions this in the ticket description:

We do not do any corresponding validation for the 'post_url' and 'post_title' activitymeta values.

We weren't even saving the activity meta when the blog post / blog comment was being added :) 02.patch adds this as well as unit tests.

[boonebgorges]

Thanks for the patch, r-a-y. It's a shame that this is the way we have to invalidate (by pulling up all the comments belonging to a post) but I suppose there's just no better way to do it.

I would suggest that doing an either/or on new_blog_comment vs activity_comment is probably insufficient here. For one thing, I'm not sure that bp_disable_blogforum_comments() is actually a reliable indicator of whether to look in one place vs another for the comments, since the disable_blogforum_comments value can be changed. Second, this won't handle legacy content correctly. So I'd suggest that we bite the bullet and just do both queries when posts are updated.

[DJPaul]

Is this going to be ready to commit very soon?

[r-a-y]

So I'd suggest that we bite the bullet and just do both queries when posts are updated.

03.patch should handle both conditions now.

[boonebgorges]

r-a-y - This looks great. And thanks for the unit tests.

[DJPaul]

$comments = get_comments( array( 'post_id' => $post->ID ) );

It looks like you're only using the comment_ID from this data. I'd suggest that you also set fields=ids in this get_comments call for a better query.

Other than that, the patch looks OK (I'm still not fond of requesting things like the 99999 items from functions like this -- it's going to use up all the memory on a large site -- but I haven't a better suggestion at the moment.

[r-a-y]

Thanks for the feedback, DJPaul.

get_comments() doesn't support the fields=ids parameter though. However, there is another filter down the stack that we could filter so we only grab the IDs - comments_clauses.

04.patch uses this filter to narrow down the SELECT query to just the comment_ID.

Patch also removes the multiple unit tests and replaces it with just one test. This unit test handles both cases that boonebgorges mentions in comment:5, which I forgot to add a test for in the previous patch.

[DJPaul]

get_comments() doesn't support the fields=ids parameter thoug

It does. Look at ​https://core.trac.wordpress.org/browser/trunk/src/wp-includes/comment.php#L374

[r-a-y]

Jeez, I feel stupid :)

05.patch should fix this.

[DJPaul]

Cool, looks great.

[r-a-y]

In 8944:

Add activity meta when recording a blog post or blog comment.

This commit:

• Adds the post_title and post_url information as activity meta when recording a blog post or blog comment into the activity table.
• Updates the post_title and post_url activity meta whenever a blog post is updated.
• Adds a unit test.

This addresses issues with showing the correct blog post information in the
activity loop for blog post and blog comment activity items.

Props r-a-y, DJPaul, boonebgorges.

Fixes #5609.

[r-a-y]

It appears that using get_comments() with fields=ids is causing issues with unit tests and appears only to be supported on WP trunk:
​https://travis-ci.org/buddypress/BuddyPress/builds/32875192

Update: fields=ids is only supported in WP 4.0 - ​https://core.trac.wordpress.org/ticket/28434

Going to put in what I recommended in 04.patch.

[r-a-y]

In 8945:

Do not use get_comments() with fields=ids parameter.

fields=ids is only supported in WordPress 4.0:
​https://core.trac.wordpress.org/ticket/28434

This was causing unit test failures in older versions of WordPress.

To workaround this, this commit introduces bp_blogs_comments_clauses_select_by_id(),
which is used as a filter so we can select comments by the comment_ID
column only and is an alternative to the fields=ids approach.

Fixes #5609.