Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

#5970 closed enhancement (fixed)

Remove $filter variables holding hook names.

Reported by: tw2113 Owned by:
Milestone: 2.2 Priority: normal
Severity: normal Version:
Component: Core Keywords: needs-patch good-first-bug
Cc: georgemamadashvili@…

Description

There are currently four files that create a $filter variable that holds the filter hook name and are passed in to apply_filters(). The four files are bp-activity-notifications, bp-groups-notifications, bp-friends-notifications, and bp-messages-notifications because of how these are done, it's notably more difficult to set up documentation for the hooks, as well as makes things a bit more complex than they need to be.

I think a more optimal solution is to make parts of the hook names variable, depending on context in the surrounding code, instead of the whole hook. Think wp_insert_{$post->post_type}

For the activity one, it's simple enough as there's only two to deal with, and it's bp_activity_multiple_at_mentions_notification vs bp_activity_single_at_mentions_notification. In place of the whole hook, I have a diff ready to be created that changes $filter to $amount, and the apply_filters() become bp_activity_' . $amount . '_at_mentions_notification or bp_activity_{$amount}_at_mentions_notification depending on your preference for single vs double quotes.

Messages will be the same situation, except the hook is bp_messages_{$amount}_new_message_notification

Friends will be a bit more, except there will be two spots to change out. bp_friends_{$amount}_friendship_{$action}_notification with $amount being the same as above, and $action being "accepted" or "request".

Lastly, the Group notifications actually does the variable needlessly, as the apply_filters calls are all done within their respective if/else blocks. It'd be better to just remove that usage completely.

Attachments (6)

bp-friends-notifications-5970.diff (2.3 KB) - added by tw2113 6 years ago.
bp-groups-notifications-5970.diff (27.6 KB) - added by Mamaduka 6 years ago.
bp-groups-notifications-5970.2.diff (28.4 KB) - added by Mamaduka 6 years ago.
bp-groups-notifications-5970-tests.diff (8.3 KB) - added by Mamaduka 6 years ago.
bp-messages-notifications-5970.diff (1.7 KB) - added by tw2113 6 years ago.
Filter name changes.
bp-messages-notifications-tests-5970.diff (4.2 KB) - added by tw2113 6 years ago.
Filter name changes unit tests.

Download all attachments as: .zip

Change History (22)

#1 @DJPaul
6 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 2.2

I think if this kind of change makes the documenting easier, then let's do it, as long as the overall filter names don't change, or the arguments passed to them.

We could also make sure the complete action names (without variables) are contained within the documentation added, please, or at least nearby in a separate code comment, so people can find them easily when doing a "find in files".

#2 @tw2113
6 years ago

Agreed with keeping a "complete" version of each nearby. I'll get to doing some patches for this soon.

#3 @tw2113
6 years ago

If we can get the bp-friends-notifications diff file committed soon, I'll be able continue finishing up the Friends component hooks docs ticket #5942

#4 @boonebgorges
6 years ago

Change looks fine. I'm going to add unit tests.

#5 @boonebgorges
6 years ago

In 9153:

Assemble the filters in friends_format_notifications() dynamically.

This makes them easier to document.

Props tw2113.
See #5970.

#6 @Mamaduka
6 years ago

  • Cc georgemamadashvili@… added

Assembles filters dynamically in groups_format_notifications().

Also include hook documentation for those filters.

#7 @boonebgorges
6 years ago

Thanks, Mamaduka! I will feel more comfortable committing these if we have unit tests first, along the lines of what I wrote here: https://buddypress.trac.wordpress.org/browser/trunk/tests/phpunit/testcases/friends/notifications.php?rev=9153 If you write them, it'll get done faster, wink wink :)

#8 @Mamaduka
6 years ago

boonebgorges,

Wasn't sure how to write unit tests for this this. Thanks for the reference will add patch for tests well.

#9 @boonebgorges
6 years ago

Excellent, thanks! You should be able to copy those other tests more or less directly. The important thing is just to verify that the filters being fired are exactly the same pre- and post-patch. Thanks!

#10 @Mamaduka
6 years ago

boonebgorges,

Attached updated patch with tests.

#11 @boonebgorges
6 years ago

In 9170:

Assemble group notification hooks dynamically.

This makes them easier to document using WP's filter documentation standards.

Props Mamaduka.
See #5970.

This ticket was mentioned in Slack in #buddypress by mamaduka. View the logs.


6 years ago

@tw2113
6 years ago

Filter name changes.

@tw2113
6 years ago

Filter name changes unit tests.

#13 @tw2113
6 years ago

Filter name changes + unit tests for them. The tests are passing, but if possible, I'd appreciate someone looking them over to see if there's anything notable needing changed.

Once these are in place, I'll be able to continue with #5945

#14 @boonebgorges
6 years ago

In 9208:

Assemble message notification filters dynamically.

This allows us to centralize the documentation.

Props tw2113.
See #5970.

This ticket was mentioned in Slack in #buddypress by tw2113. View the logs.


6 years ago

#16 @DJPaul
6 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.