Skip to:
Content

bbPress.org

Opened 11 years ago

Closed 9 years ago

#2529 closed defect (bug) (fixed)

Single topic description counts not updated after permanently deleting a reply

Reported by: netweb's profile netweb Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 2.6 Priority: normal
Severity: normal Version: 2.5
Component: Component - Replies Keywords: has-patch commit
Cc:

Description

Repro Example (front end)

A Topic:

  • This topic contains 8 replies, has 1 voice, and was last updated by admin 3 days, 23 hours ago.

Mark the last reply as 'Trash' (Using the last reply checks latest topic freshness functionality)

  • This topic contains 7 replies (+ 1 hidden), has 1 voice, and was last updated by admin 4 days, 1 hour ago.

Permanently Delete that reply

  • This topic contains 7 replies (+ 1 hidden), has 1 voice, and was last updated by admin 4 days, 1 hour ago.

Expected the the sigle topic description to now be only 7 replies, to get the counts accurate again you need to run the repair tool "Count spammed & trashed replies in each topic"

  • This topic contains 7 replies, has 1 voice, and was last updated by admin 4 days, 1 hour ago.

The same behaviour when performing the above from the back end.

See bbp_single_topic_description, bbp_deleted_reply and bbp_update_reply_walker

Attachments (7)

2529.01.diff (907 bytes) - added by thebrandonallen 11 years ago.
2529.2.diff (1.1 KB) - added by thebrandonallen 10 years ago.
2529.3.diff (1.2 KB) - added by thebrandonallen 10 years ago.
Same as 2529.2.diff, targeted to run on bbp_deleted_reply only
2529.4.diff (839 bytes) - added by thebrandonallen 10 years ago.
Patch bbp_update_reply_walker
2529.5.diff (926 bytes) - added by thebrandonallen 10 years ago.
Same as 2529.4.diff, targeted on run on bbp_deleted_reply only
2529.06.patch (651 bytes) - added by thebrandonallen 10 years ago.
2529.7.diff (1.1 KB) - added by netweb 9 years ago.

Download all attachments as: .zip

Change History (23)

#1 @thebrandonallen
11 years ago

I wasn't able to reproduce the problem from the back end, but the attached patch fixes the issue when permanently deleting from the front end using the admin links.

#2 @thebrandonallen
11 years ago

  • Keywords has-patch added

whoops :)

#4 @johnjamesjacoby
11 years ago

  • Milestone changed from Awaiting Review to 2.6

Thanks for this. Curious why it's not already happening, though.

On the deleted_post action, bbp_deleted_reply calls bbp_update_reply_walker with similar default parameters as this, so it should already be happening.

I'll do some checking to confirm the call stack. Do you mind doing the same on your end?

#5 @johnjamesjacoby
11 years ago

Oh... because by the time deleted_post fires, the metadata has already been removed via delete_metadata_by_mid()

Duh.

We'll likely need an additional action on before_delete_post to store the necessary meta in the bbpress() instance so we can pass them into a special action to handle deletes.

#6 @thebrandonallen
10 years ago

Spent some more time with this. Came up with two basic solutions, each with a version targeted only for bbp_deleted_reply (so... basically 4 solutions). All are based on the fact that _bbp_forum/topic_id meta is essentially derived from the post_parent field of the reply, and the post_parent of the reply's topic. Since deleted_post is called directly before the post cache is cleaned, we can dip into the cache to help us get our topic_id/forum_id. With that in mind, the 4, below, patches fix the issue.

The first set works by patching the bbp_get_reply_topic/forum_id functions to pull from post cache. This also includes a more targeted version that only runs on bbp_deleted_reply.

The second set works by patching bbp_update_reply_walker to pull from the post cache. It too, includes a more targeted version that only runs on bbp_deleted_reply.

Personally I think 2529.2.diff is the best approach, because I could be a very helpful fallback. Going this route would also raise the question of whether or not to do this with any other bbp_get_*_topic/forum_id functions.

Last edited 10 years ago by thebrandonallen (previous) (diff)

@thebrandonallen
10 years ago

Same as 2529.2.diff, targeted to run on bbp_deleted_reply only

@thebrandonallen
10 years ago

Patch bbp_update_reply_walker

@thebrandonallen
10 years ago

Same as 2529.4.diff, targeted on run on bbp_deleted_reply only

This ticket was mentioned in IRC in #bbpress-dev by netweb. View the logs.


10 years ago

#8 @thebrandonallen
10 years ago

Came across this again while working on the tests for #2801. The new attached patch should be sufficient.

This ticket was mentioned in Slack in #bbpress by jjj. View the logs.


10 years ago

#10 @johnjamesjacoby
9 years ago

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

In 5859:

Replies: fallback on post_parent in bbp_update_reply_walker().

This changeset fixes a bug where topic reply counts were not updated when replies were permanently deleted, causing hidden reply counts to be wildly inaccurate.

Props thebrandonallen. Fixes #2529.

#11 @johnjamesjacoby
9 years ago

In 5860:

Topics: fallback on post_parent in bbp_update_topic_walker().

This changeset fixes a bug where forum topic counts were not updated when topics were permanently deleted, causing hidden topic counts to be wildly inaccurate.

Props thebrandonallen. Fixes #2529.

#12 @netweb
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

All WordPress 3.7 and 3.8 versions PHPUnit tests fail with this change

  • PHP Fatal error: Call to undefined function current_action() in /tmp/wordpress/src/wp-content/plugins/bbPress/src/includes/replies/functions.php on line 932

Function current_action() was added to WordPress in 3.9 in [wp27294]

Last edited 9 years ago by netweb (previous) (diff)

#13 @johnjamesjacoby
9 years ago

Harrumph. I suppose so. That's unfortunate.

I'm comfortable bumping the minimum WordPress version whenever the core team EOL's it, maybe around the 4.3 RC?

#14 follow-up: @thebrandonallen
9 years ago

We could just switch to current_filter for now.

@netweb
9 years ago

#15 in reply to: ↑ 14 @netweb
9 years ago

  • Keywords commit added

Replying to thebrandonallen:

We could just switch to current_filter() for now.

Works for me, tested 2529.7.diff per original repro steps and works as expected.

Those repro steps of mine are horrible, glad my writing has, hopefully improved ;)

#16 @johnjamesjacoby
9 years ago

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

In 5866:

Prefer current_filter() over current_action() to maintain compatibility with WordPress versions older than 3.9. Fixes #2529.

Note: See TracTickets for help on using tickets.