Skip to:
Content

Opened 2 years ago

Closed 19 months ago

#6750 closed enhancement (fixed)

Extending Messages notification should be improved

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

Description

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 2 years ago.
6750.02.patch (5.5 KB) - added by r-a-y 21 months ago.

Download all attachments as: .zip

Change History (11)

@imath
2 years ago

#1 @boonebgorges
2 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
2 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.


21 months ago

#4 @imath
21 months ago

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

#5 @imath
21 months ago

The patch should handle the string format.

@r-a-y
21 months ago

#6 @r-a-y
21 months 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...:
https://buddypress.trac.wordpress.org/browser/tags/2.4.3/src/bp-messages/bp-messages-notifications.php#L212 :(

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

#7 @imath
19 months 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.


19 months ago

#9 @r-a-y
19 months 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.