Skip to:
Content

bbPress.org

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#2322 closed defect (fixed)

Moving Topic doesn't update _bbp_topic_count

Reported by: wpdennis Owned by: johnjamesjacoby
Milestone: 2.6 Priority: high
Severity: normal Version: 2.3
Component: Component - Forums Keywords: has-patch
Cc:

Description

If I move topics in the front-end it doesn't update the postmetas "_bbp_topic_count", "_bbp_reply_count", "_bbp_total_reply_count" and "_bbp_total_topic_count" neither of the target nor the old forum post.

bbp_move_topic_handler() calls bbp_update_topic_forum_id() and maybe bbp_update_topic_forum_id() could recalc the counts of the old and the new forum post?

Attachments (1)

2322.01.patch (562 bytes) - added by thebrandonallen 6 years ago.

Download all attachments as: .zip

Change History (17)

#1 @johnjamesjacoby
8 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 2.4

Hm. Thought we'd fixed this once before. Moving to 2.4 to look into this there.

#2 @aliso
7 years ago

  • Milestone 2.4 deleted
  • Resolution set to worksforme
  • Status changed from new to closed

I'm not able to reproduce this - all the meta values are updating properly for me when I move a topic from one forum to another. Closing for now.

#4 @wpdennis
7 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

It's not fixed. Not even in 2.4.1.

I've done some more digging. To reproduce it you need to activate object caching.

The reason seems to be that bbp_get_public_child_ids gets the topics to count from the object cache:

// Check for cache and set if needed
$child_ids = wp_cache_get( $cache_id, 'bbpress_posts' );

But during the topic moving the cache wasn't invalidated and will not contain the moved topic.

The cache will only be invalidated in bbp_clean_post_cache(), but this is not added to bbp_edit_forum_pre_extras (and we shouldn't do that, it would be too expensive).

It is only added to bbp_new_forum_pre_extras and similar actions.

Maybe we could add a new action in the beginning of bbp_move_topic_handler() and add bbp_clean_post_cache() to it?

Like bbp_move_forum_pre_extras or something?

#5 @wpdennis
7 years ago

The idea with bbp_move_forum_pre_extras wasn't that good, since we need it for every forum.

As a hack, adding bbp_clean_post_cache( $ancestor ); in bbp_move_topic_handler() is working:

// Loop through ancestors and update them
if ( !empty( $old_forum_ancestors ) ) {
	foreach ( $old_forum_ancestors as $ancestor ) {
		if ( bbp_is_forum( $ancestor ) ) {
			bbp_clean_post_cache( $ancestor );
			bbp_update_forum( array(
				'forum_id' => $ancestor,
			) );
		}
	}
}

and:

// Loop through ancestors and update them
if ( !empty( $new_forum_ancestors ) ) {
	foreach ( $new_forum_ancestors as $ancestor ) {
		if ( bbp_is_forum( $ancestor ) ) {
			bbp_clean_post_cache( $ancestor );
			bbp_update_forum( array(
				'forum_id' => $ancestor,
			) );
		}
	}
}

#6 @netweb
7 years ago

  • Milestone set to Awaiting Review

#7 @johnjamesjacoby
7 years ago

What if we clean the post cache when updating a forum? We should be doing that anyways; is that enough, VS the brute force tree flush?

#8 @wpdennis
7 years ago

You mean in bbp_update_forum()? That would do the trick.

bbp_update_topic_walker() would call it, too. But in bbp_update_topic() it only gets triggered on new topics, is it?

Just a quick note aside:
On new topics and probably some other things it would get called twice. If it's expensive, maybe these actions should be revisited:

// Caches
add_action( 'bbp_new_forum_pre_extras',  'bbp_clean_post_cache' );
add_action( 'bbp_new_forum_post_extras', 'bbp_clean_post_cache' );
add_action( 'bbp_new_topic_pre_extras',  'bbp_clean_post_cache' );
add_action( 'bbp_new_topic_post_extras', 'bbp_clean_post_cache' );
add_action( 'bbp_new_reply_pre_extras',  'bbp_clean_post_cache' );
add_action( 'bbp_new_reply_post_extras', 'bbp_clean_post_cache' );

#9 follow-up: @johnjamesjacoby
7 years ago

Not very expensive, but it's not currently ideal. You're correct that we should revisit our cache (and cache busting) strategy to make sure we're not running into more situations like this one. This will become more and more important as we stop doing expensive recalculations and start incrementing based on existing values.

There is a lot we can learn from WordPress core and Pages in this regard.

#10 in reply to: ↑ 9 @wpdennis
7 years ago

Maybe we should split this ticket then.

We could use this for Ticket for the Hotfix you suggested.

It's so much work to fix the counts manually in a live system with object caching enabled, that maybe you could consider to add the cache cleaning in 2.4.2 as a temporary fix? I'm currently testing it and will report back soon.

For the more ideal solution a new ticket would be better, especially if it's targeting the whole caching strategy. But I'm sorry, I haven't enough insights to start this ticket myself.

#11 follow-up: @johnjamesjacoby
7 years ago

  • Milestone changed from Awaiting Review to 2.6

We should be busting the cache for every change made to any post type or their meta. It's somewhat expensive given the hierarchical nature of reply > topic > forum(s), but it's necessary to make sure we're getting the correct results. This will become even more important when we move to _bump_ functions rather than recalculations, as we'll want to make sure we're getting the correct counts from fresh post data to bump from.

How's your test run looking?

#12 in reply to: ↑ 11 @wpdennis
7 years ago

We should be busting the cache for every change made to any post type or their meta. It's somewhat expensive given the hierarchical nature of reply > topic > forum(s), but it's necessary to make sure we're getting the correct results.

Yes, editing existing posts doesn't need to recalculate counts but shouldn't be worth the trouble, to build extra work arounds.

Editing is a relativly rare scenario and avoiding caching issues should be more important. So, cleaning the chache for every change should be the safest way.

How's your test run looking?

The specific bug (moving multiple topics into a newly created forum) is gone. Even programmatically created posts seem to work better now (I just noticed another caching bug in this case a few days ago).

I've patched it in a live environment 2 days ago, too. No errors or other bad things happened so far. :-)

This is the function with the added bbp_clean_post_cache():

function bbp_update_forum( $args = '' ) {

	// Parse arguments against default values
	$r = bbp_parse_args( $args, array(
		'forum_id'           => 0,
		'post_parent'        => 0,
		'last_topic_id'      => 0,
		'last_reply_id'      => 0,
		'last_active_id'     => 0,
		'last_active_time'   => 0,
		'last_active_status' => bbp_get_public_status_id()
	), 'update_forum' );

	// Last topic and reply ID's
	bbp_update_forum_last_topic_id( $r['forum_id'], $r['last_topic_id'] );
	bbp_update_forum_last_reply_id( $r['forum_id'], $r['last_reply_id'] );
	
	bbp_clean_post_cache( $r['forum_id'] );

	// Active dance
	$r['last_active_id'] = bbp_update_forum_last_active_id( $r['forum_id'], $r['last_active_id'] );

	// If no active time was passed, get it from the last_active_id
	if ( empty( $r['last_active_time'] ) ) {
		$r['last_active_time'] = get_post_field( 'post_date', $r['last_active_id'] );
	}

	if ( bbp_get_public_status_id() === $r['last_active_status'] ) {
		bbp_update_forum_last_active_time( $r['forum_id'], $r['last_active_time'] );
	}

	// Counts
	bbp_update_forum_subforum_count    ( $r['forum_id'] );
	bbp_update_forum_reply_count       ( $r['forum_id'] );
	bbp_update_forum_topic_count       ( $r['forum_id'] );
	bbp_update_forum_topic_count_hidden( $r['forum_id'] );

	// Update the parent forum if one was passed
	if ( !empty( $r['post_parent'] ) && is_numeric( $r['post_parent'] ) ) {
		bbp_update_forum( array(
			'forum_id'    => $r['post_parent'],
			'post_parent' => get_post_field( 'post_parent', $r['post_parent'] )
		) );
	}
}

#13 @johnjamesjacoby
7 years ago

  • Owner set to johnjamesjacoby
  • Status changed from reopened to new

#14 @thebrandonallen
6 years ago

  • Keywords has-patch added; dev-feedback needs-patch removed

Ran into this issue while working on tests for #2801. Attached patch should be all that's needed.

#15 @johnjamesjacoby
6 years ago

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

In 5729:

Topics: In bbp_move_topic_handler(), clean both old and new forum caches before updating forum hierarchies.

Fixes issue where moving a topic would result in incorrect counts for both forums, due to out-of-date cache values.

Props thebrandonallen. Fixes #2322.

#16 @netweb
6 years ago

In 5778:

Topics: When moving a topic update the topics post parent in bbp_move_topic_handler(), includes unit test test_bbp_move_topic_handler()

Props netweb. See #2322

Note: See TracTickets for help on using tickets.