Moving Topic doesn't update _bbp_topic_count

[wpdennis]

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?

[johnjamesjacoby]

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

[aliso]

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.

[alexvorn2]

related #1981

[wpdennis]

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?

[wpdennis]

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,
			) );
		}
	}
}

[johnjamesjacoby]

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?

[wpdennis]

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' );

[johnjamesjacoby]

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.

[wpdennis]

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.

[johnjamesjacoby]

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?

[wpdennis]

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'] )
		) );
	}
}

[thebrandonallen]

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

[johnjamesjacoby]

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.

[netweb]

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