Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5609 closed defect (bug) (fixed)

Blog data stored in activitymeta and blogmeta must be invalidated

Reported by: boonebgorges Owned by: r-a-y
Milestone: 2.1 Priority: normal
Severity: normal Version: 2.0
Component: Activity Keywords: has-patch commit
Cc: trisha@…

Description

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

Attachments (5)

5609.01.patch (2.4 KB) - added by r-a-y 5 years ago.
5609.02.patch (8.7 KB) - added by r-a-y 5 years ago.
5609.03.patch (9.0 KB) - added by r-a-y 5 years ago.
5609.04.patch (8.7 KB) - added by r-a-y 5 years ago.
5609.05.patch (7.6 KB) - added by r-a-y 5 years ago.

Download all attachments as: .zip

Change History (21)

#1 @DJPaul
5 years ago

  • Keywords needs-patch added

#2 @trishasalas
5 years ago

  • Cc trisha@… added

#3 @r-a-y
5 years ago

  • Keywords has-patch added; needs-patch removed

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.

Last edited 5 years ago by r-a-y (previous) (diff)

@r-a-y
5 years ago

@r-a-y
5 years ago

#4 @r-a-y
5 years ago

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.

#5 @boonebgorges
5 years ago

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.

#6 @DJPaul
5 years ago

Is this going to be ready to commit very soon?

@r-a-y
5 years ago

#7 @r-a-y
5 years ago

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.

#8 @boonebgorges
5 years ago

  • Keywords commit added

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

#9 @DJPaul
5 years ago

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

#10 @r-a-y
5 years ago

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.

Last edited 5 years ago by r-a-y (previous) (diff)

@r-a-y
5 years ago

#11 @DJPaul
5 years ago

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
5 years ago

#12 @r-a-y
5 years ago

Jeez, I feel stupid :)

05.patch should fix this.

#13 @DJPaul
5 years ago

Cool, looks great.

#14 @r-a-y
5 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

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.

#15 @r-a-y
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Last edited 5 years ago by r-a-y (previous) (diff)

#16 @r-a-y
5 years ago

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

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.

Note: See TracTickets for help on using tickets.