Skip to:

Opened 9 years ago

Closed 9 years ago

#6515 closed enhancement (fixed)

Allow plugins to filter groups_format_notifications unmatched return values.

Reported by: dcavins's profile dcavins Owned by: dcavins's profile dcavins
Milestone: 2.4 Priority: lowest
Severity: trivial Version: 2.3.2
Component: Toolbar & Notifications Keywords: has-patch commit
Cc: david.cavins@…


I think we should include all of the known arguments when passing unformatted items out via the groups_format_notifications filter.

Attachments (2)

6515.01.patch (827 bytes) - added by dcavins 9 years ago.
6515.02.patch (1.2 KB) - added by dcavins 9 years ago.
Allow plugins to filter unmatched component_actions in group-related notification.

Download all attachments as: .zip

Change History (6)

9 years ago

#1 @dcavins
9 years ago

  • Component changed from API to Component - Notifications
  • Summary changed from Add $format arg to groups_format_notifications filter. to Allow plugins to filter groups_format_notifications unmatched return values.

OK, I'm not sure I understand what the action that I was modifying is meant to do. It appears at the end of a long switch for each of the various group-related component_actions but it doesn't seem as though a do_action allows a hooked function to return a value for the originating function.

Here's what I'm thinking should be possible: Group-related plugins add their own custom component_actions, but they are really group-related, so they attempt to save them like this:

'user_id'           => $user_id,
'item_id'           => $group_id,
'secondary_item_id' => $meaningful_id,
'component_name'    => 'groups',
'component_action'  => 'my_plugin_action'

As groups_format_notifications() is currently written, any unrecognized action will not match any of the cases in the long switch and then fire off the do_action( 'groups_format_notifications' ) before returning false.

If it makes any sense for plugins to use an existing component like 'groups' as a component_name then my second patch adds a default case to the long switch so that unmatched actions will do an apply_filters( 'bp_groups_' . $action . '_notification', null, $item_id, $secondary_item_id, $total_items, $format ), which, if a notification results, will be returned.

I'm not sure if we really want plugins that create notifications to declare a unique component_name when adding notifications (even though they aren't really a full-blown component), or if it makes sense to piggyback on existing components.

Thanks for your input, notification-savvy people.

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

9 years ago

Allow plugins to filter unmatched component_actions in group-related notification.

#2 @johnjamesjacoby
9 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 2.4

Both patches seem good to me.

I agree that the groups_format_notifications action at the end is not very helpful, especially considering every other condition returns something, which means it only executes if nothing else happened.

It looks like this was addressed in the Friends component in friends_format_notifications by using a $return value, but that still doesn't explain to me why it's an action and not a filter.

These Notifications callbacks are pretty monolithic. Does it make sense to also break these cases up into new functions? groups_format_notifications for example is 650 lines long.

#3 @dcavins
9 years ago

John, if we add a default case hook sort of like I've done in 6515.02.patch, then we could break the function up into similar component-y pieces--for instance, moving the membership request and invitation cases out to the requests/invites code and hooking into the new default case using the dynamic hooks:


Does that seem like a good approach?

Thanks for the feedback!

#4 @dcavins
9 years ago

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

In 9970:

Add filter in groups_format_notifications().

Allow plugins to filter unmatched component
actions in group-related notification. Changeset
adds a dynamically named filter based on the name
of the component action.

Fixes #6515.

Note: See TracTickets for help on using tickets.