Skip to:

Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#6750 closed enhancement (fixed)

Extending Messages notification should be improved

Reported by: imath's profile imath Owned by: imath's profile imath
Milestone: 2.6 Priority: normal
Severity: normal Version: 1.0
Component: Messages Keywords: has-patch commit


If you try to add a new notification action (meaning !== 'new_message'), it's quite risky:

  • First you get a notice error because in this case the $text var is not defined (which is not so serious)
  • Most serious is you can't be sure to only filter your custom action because you need to filter bp_messages_single_new_message_notification which is not in the if ( 'new_message' === $action ) statement and which is not including the needed $action and $format variables.

Maybe we should improve this. See attached patch.

Attachments (2)

6750.patch (4.9 KB) - added by imath 8 years ago.
6750.02.patch (5.5 KB) - added by r-a-y 8 years ago.

Download all attachments as: .zip

Change History (11)

8 years ago

#1 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 2.5

Ugh, this was a pretty ugly one. Good catch, imath. Moving the new_message filter into the if ( 'new_message' === $action ) block, and introducing a new catch-all filter, both seem good to me.

It's a pity that these filters were originally introduced as dynamic. We could greatly simplify the documentation, and greatly decrease the number of filters, if each notification function had a single filter: bp_messages_format_notification (or whatever), which would accept an $action and $amount parameter in addition to whatever params they normally accept. This is not a criticism of 6750.patch - it just copies what the rest of BP is doing. Just registering my sadness for the record :)

#2 @boonebgorges
8 years ago

  • Owner set to imath
  • Status changed from new to assigned

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

8 years ago

#4 @imath
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.5 to 2.6

#5 @imath
8 years ago

The patch should handle the string format.

8 years ago

#6 @r-a-y
8 years ago

  • Keywords has-patch added; needs-patch removed

02.patch handles the string format and also adds another edge case for the 'bp_messages_single_new_message_notification' filter. If $format is an array, there are seven parameters; if $format is a string, there are six...: :(

Not sure how people are using this filter at the moment!

#7 @imath
8 years ago

  • Keywords commit added

02.patch looks good @r-a-y

We just need to make sure the @since is bumped to 2.6.0 for the dynamic filter 'bp_messages_' . $action . '_notification'

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

8 years ago

#9 @r-a-y
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 10706:

Messages: Introduce dynamic filter, "bp_messages_{$action}_notification".

This new filter helps plugin developers filter private message
notifications that are not the 'new_message' action.

For backward compatibility, we apply the existing
"bp_messages_{$amount}_new_message_notification" filter just before
adding our new dynamic filter.

Fixes #6750.

Props imath.

Note: See TracTickets for help on using tickets.