Allow plugins to filter groups_format_notifications unmatched return values.

[dcavins]

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

[dcavins]

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.

[dcavins]

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

[johnjamesjacoby]

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.

[dcavins]

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:

bp_groups_new_membership_request_notification
bp_groups_membership_request_accepted_notification
bp_groups_membership_request_rejected_notification
bp_groups_group_invite_notification

Does that seem like a good approach?

Thanks for the feedback!

[dcavins]

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.