Skip to content

Added function to remove topics from digest when trashed#194

Open
hoegrammer wants to merge 1 commit into
boonebgorges:masterfrom
hoegrammer:trashed-topics
Open

Added function to remove topics from digest when trashed#194
hoegrammer wants to merge 1 commit into
boonebgorges:masterfrom
hoegrammer:trashed-topics

Conversation

@hoegrammer

Copy link
Copy Markdown

@boonebgorges boonebgorges left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request and thanks for your patience while I reviewed it!

It looks like your hooks and queries are too broad. By hooking to wp_trash_post, you are reacting to every deleted WP post, regardless of whether it was created by bbPress. This could result in cases where a non-bbPress ID is passed as $post_id, and then the bp_activity_get() query fetches an item that is a "false match".

So I'd suggest a few changes:

  • Instead of wp_trash_post, use the bbPress hooks: bbp_delete_reply, bbp_trash_reply, bbp_delete_topic, bbp_trash_topic. This will ensure that you're reacting only to bbPress content. See https://bbpress.trac.wordpress.org/browser/tags/2.6.4/src/includes/replies/functions.php#L1883 to see how these actions target only bbPress content.

  • The bp_activity_get() query needs to be more targeted. secondary_id is used for lots of different things by different plugins, and we don't want to get false positives. You'll probably want to match against action, either bbp_new_topic or bbp_new_reply, depending on what's being deleted. This will ensure accuracy.

@r-a-y

r-a-y commented May 19, 2020

Copy link
Copy Markdown
Collaborator

As I mentioned in the forums, trashing a forum post should, in theory, remove the corresponding activity item.

But bbPress 2.6 broke this functionality if the “Allow topic and reply revision logging” option is disabled under “Settings > Forums” in the admin dashboard.

See https://github.com/bbpress/bbPress/blob/8a1729aa3c9d77609c8a5b2d191894fa93a09d93/src/includes/extend/buddypress/activity.php#L644 and the subsequent reply_delete() call later down the method.

So this PR shouldn't be needed.

The temporary solution for bbPress 2.6 is to re-enable revisions or to patch the lines to remove the revision checks. Related: https://bbpress.trac.wordpress.org/ticket/3328.

@hoegrammer

Copy link
Copy Markdown
Author

tested on a fresh install and yes you are right, this PR is not needed, sorry to waste your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants