Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added function to remove topics from digest when trashed #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hoegrammer
Copy link

Copy link
Owner

@boonebgorges boonebgorges left a comment

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
Copy link
Collaborator

r-a-y commented May 19, 2020

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