Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 8 years ago

#7020 closed defect (bug) (fixed)

bp_notifications_get_notifications_for_user() bug

Reported by: rogercoathup's profile rogercoathup Owned by: r-a-y's profile r-a-y
Milestone: 2.6 Priority: normal
Severity: normal Version: 1.9
Component: Toolbar & Notifications Keywords: dev-feedback has-patch commit
Cc:

Description

We pass an array to apply_filters_ref_array in bp_notifications_get_notifications_for_user()

i.e.

<?php
$ref_array = array(
        $component_action_name,
        $component_action_items[0]->item_id,
        $component_action_items[0]->secondary_item_id,
        $action_item_count,
        $format
);

The first parameter should be modifiable content for filtering, not the action name.

With the current setup, if a hook returns content, the action name is overwritten in apply_filters_ref_array() and invalid in any subsequent hooks hooked on bp_notifications_get_notifications_for_user.

Attachments (1)

7020.01.patch (2.7 KB) - added by r-a-y 9 years ago.

Download all attachments as: .zip

Change History (8)

#1 @r-a-y
9 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 2.6
  • Version changed from 2.5.0 to 1.9

Related: #6669, #bbPress2665.

The 'bp_notifications_get_notifications_for_user' filter is quite weird indeed. You either have to return a string of the content or array( 'text' => 'CONTENT', 'link' => 'LINK' ) depending on the $format. I think this needs better documentation.

If a plugin does change the value of $component_action_name, then no other plugin can do checks against $component_action_name. comment:8:ticket:6669 goes into the problem as well.

We probably need to add some additional parameters so the action is passed again for better checks:

<?php
$ref_array = array(
        $component_action_name,
        $component_action_items[0]->item_id,
        $component_action_items[0]->secondary_item_id,
        $action_item_count,
        $format,
        $component_action_name, // Duplicate this since this could be changed by plugins.
        $component_name,        // Component name is a good parameter to do checks against as well.
);

The additional parameters should allow you to check the action name again as intended.


Personally, you shouldn't use this filter. For an alternative way to format your custom notifications, read the latter half of comment:4:ticket:6669.

Last edited 9 years ago by r-a-y (previous) (diff)

#2 @r-a-y
9 years ago

  • Component changed from API to Component - Notifications

#3 @r-a-y
9 years ago

  • Keywords has-patch added

01.patch modifies the 'bp_notifications_get_notifications_for_user' filter to do the following:

  • Introduces two new parameters - $component_action_name and $component_name as the 6th and 7th parameters to the filter.
  • Deprecates the first parameter in the filter. It is recommended to do notification action checks against the 6th parameter from now on.
  • Adds proper documentation for the filter.
Last edited 9 years ago by r-a-y (previous) (diff)

@r-a-y
9 years ago

#4 @rogercoathup
9 years ago

Thanks @r-a-y -- in this case, our component is derived from BP_Component, but I hadn't realised the class supported a notification callback to format notifications

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


8 years ago

#6 @DJPaul
8 years ago

  • Keywords commit added

Looks good, @r-a-y.

#7 @r-a-y
8 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 10725:

Notifications: Introduce two new parameters to the 'bp_notifications_get_notifications_for_user' filter.

This commit:

  • Adds $component_action_name and $component_name as the 6th and 7th parameters to the filter.
  • Updates the DocBlock for the filter.

In the DocBlock, we are deprecating the first parameter, which is the
component action name. It is recommended to do notification action name
checks against the 6th parameter from now on.

The reason why we are deprecating the first parameter is due to potential
plugin conflicts. If a plugin is doing a specific check on the notification
action and changes the return value for the filter, then no other plugin
can do checks for that same notification action. Also see #6669.

Fixes #7020.

Note: See TracTickets for help on using tickets.