Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#6669 closed defect (bug) (no action required)

Blank notifications when using the bp_notifications_get_notifications_for_user filter hook

Reported by: henrywright's profile henry.wright Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Toolbar & Notifications Keywords:
Cc:

Description

I've come across a strange problem with the bp_notifications_get_notifications_for_user filter hook which causes admin bar notifications to be blank. Ref: https://github.com/buddypress/BuddyPress/blob/master/src/bp-notifications/bp-notifications-functions.php#L282

Steps to reproduce the problem

Step 1: Filter active components so that 2 are added:

function step_1( $component_names, $active_components ) {
    $component_names[] = 'my_first_component';
    $component_names[] = 'my_second_component';
    return $component_names;
}
add_filter( 'step_1', 'crowdmentions_get_registered_components', 10, 2 );

Step 2: Now try to use the bp_notifications_get_notifications_for_user filter twice like this:

function step_2a( $component_action, $item_id, $secondary_item_id, $item_count, $format = 'string' ) {
    if ( $component_action === 'my_first_component' ) {
        // Do something making sure to return the appropriate type (array in this case).
    }
}
add_filter( 'bp_notifications_get_notifications_for_user', 'step_2a', 10, 5 );
function step_2b( $component_action, $item_id, $secondary_item_id, $item_count, $format = 'string' ) {
    if ( $component_action === 'my_second_component' ) {
        // Do something making sure to return the appropriate type (array in this case).
    }
}
add_filter( 'bp_notifications_get_notifications_for_user', 'step_2b', 10, 5 );

You should see blank notifications.

To the best of my knowledge, it is happening because either step_1a() or step1b() will always return NULL which means the value of $content back inside bp_notifications_get_notifications_for_user() will also be NULL at some point.

Change History (12)

#1 @r-a-y
8 years ago

  • Keywords reporter-feedback added

Make sure you return a value in your step_2a() and step2b() functions:

function step_2a( $component_action, $item_id, $secondary_item_id, $item_count, $format = 'string' ) {
    if ( $component_action === 'my_first_component' ) {
        // Do something making sure to return the appropriate type (array in this case).
    }
    return $component_action;
}
add_filter( 'bp_notifications_get_notifications_for_user', 'step_2a', 10, 5 );

function step_2b( $component_action, $item_id, $secondary_item_id, $item_count, $format = 'string' ) {
    if ( $component_action === 'my_second_component' ) {
        // Do something making sure to return the appropriate type (array in this case).
    }
    return $component_action;
}
add_filter( 'bp_notifications_get_notifications_for_user', 'step_2b', 10, 5 );

Does that fix your issue?

#2 @henry.wright
8 years ago

Thanks for the suggestion r-a-y but I don't think you really want to be returning the component action here?

#3 @henry.wright
8 years ago

  • Keywords reporter-feedback removed

#4 @r-a-y
8 years ago

  • Keywords reporter-feedback added

When working with filters, you need to return something when you are not working with your own conditional statement; that's why you are getting a null value for other notifications.

Your example should look something like this:

function step_2a( $component_action, $item_id, $secondary_item_id, $item_count, $format = 'string' ) {
    if ( $component_action === 'my_first_component' ) {
        // Do something making sure to return the appropriate type (array in this case).
        return array( CUSTOM_ARRAY );
    }

    // Make sure you return the default value so other notifications will still work
    return $component_action;
}
add_filter( 'bp_notifications_get_notifications_for_user', 'step_2a', 10, 5 );

Let me know if that works for you.

Edit: If you are trying to create and format notifications for your custom component without extending the BP_Component class, you can do something like this:

// Step 1: Register components for notification component use
add_action( 'bp_setup_globals', 'my_register_custom_notifications' );

function my_register_custom_notifications() {
	// Register component manually into buddypress() singleton
	buddypress()->my_first_component = new stdClass;
	// Add notification callback function
	buddypress()->my_first_component->notification_callback = 'my_first_component_notif_callback';

	// Repeat for other components
	buddypress()->my_second_component = new stdClass;
	buddypress()->my_second_component->notification_callback = 'my_second_component_notfi_callback';

	// Now register components into active components array
	buddypress()->active_components['my_first_component'] = 1;
	buddypress()->active_components['my_second_component'] = 1;
} );

// Step 2: Format the notifications for your component
//
function my_first_component_notif_callback( $action, $item_id, $secondary_item_id, $total_items, $format = 'string' ) {
	// Do what you need to here; should resemble other notification callbacks.
	// See bp_activity_format_notifications() for how the Activity component does it.
}

// Repeat for second component
function my_second_component_notif_callback( $action, $item_id, $secondary_item_id, $total_items, $format = 'string' ) {
}
Version 3, edited 8 years ago by r-a-y (previous) (next) (diff)

#5 @henry.wright
8 years ago

  • Keywords reporter-feedback removed

Is there a specific reason why you are using the bp_notifications_get_notifications_for_user filter?

I was looking at how bbPress does it in version 2.5.8. See here:

https://plugins.trac.wordpress.org/browser/bbpress/tags/2.5.8/includes/extend/buddypress/notifications.php#L41

I think the bbp_format_buddypress_notifications() function will suffer from the same problems I came across because that doesn't return a value in cases where 'bbp_new_reply' !== $action. But looking at trunk, that function now seems to have been rewritten to return a value (like you suggested above).

Also, thanks for the advice you gave in your my_register_custom_notifications() function above, that looks a neat way of doing it. Which approach of the two would you recommend?

Last edited 8 years ago by henry.wright (previous) (diff)

#6 @r-a-y
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed
  • Version 2.3.3 deleted

Which approach of the two would you recommend?

There is no wrong way of doing it.

The bbPress method also works for registering, but I prefer the suggestion I recommended above.

The reason why is I think the 'bp_notifications_get_notifications_for_user' filter is a little too general because there are no component checks.

If you use this filter, you can only do checks on the component action name. So there is the potential for conflicts to occur if other plugins use the same component action name to record their notifications and if the plugin is using this filter to format their notifications.

Update: See my additional thoughts here - comment:1:ticket:7020.

Going to close this as I do not think this is a bug, but feel free to continue replying if you have other questions.

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

#7 @henry.wright
8 years ago

Thanks r-a-y, really appreciate you taking the time to help me with this!

#8 @likebtn
8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Guys, this seems to be a bug.

More than one "bp_notifications_get_notifications_for_user" filter will never work.

In /wp-content/plugins/buddypress/bp-notifications/bp-notifications-template.php:

<?php
$description = apply_filters_ref_array( 'bp_notifications_get_notifications_for_user', array( $notification->component_action, $notification->item_id, $notification->secondary_item_id, 1 ) );

In /wp-includes/plugin.php:

<?php
do {
        foreach( (array) current($wp_filter[$tag]) as $the_ )
                if ( !is_null($the_['function']) )
                        $args[0] = call_user_func_array($the_['function'], array_slice($args, 0, (int) $the_['accepted_args']));

} while ( next($wp_filter[$tag]) !== false );

apply_filters_ref_array() function calls callback functions and return value of the first callback is passed as the first argument of the second. For example if "bbp_format_buddypress_notifications" function returns nothing our own "bp_notifications_get_notifications_for_user" callback function receives an empty first "action" parameter.

#9 @r-a-y
8 years ago

  • Resolution set to invalid
  • Status changed from reopened to closed

For example if "bbp_format_buddypress_notifications" function returns nothing our own "bp_notifications_get_notifications_for_user" callback function receives an empty first "action" parameter.

This is a bbPress issue that is scheduled for release in bbPress 2.6. See:
https://bbpress.trac.wordpress.org/ticket/2665
https://bbpress.trac.wordpress.org/ticket/2779

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

#10 @likebtn
8 years ago

OK. Here is the hack we are using to make our custom filter work:

<?php
function likebtn_notifications_get_notifications_for_user($action, $item_id, $secondary_item_id, $total_items, $format = 'string')
{
    global $wp_filter;

    $return = '';

    if (count($m) == 3) {
        // Do something here

        // We modify global wp_filter to call our bbPress wrapper function
        if (isset($wp_filter['bp_notifications_get_notifications_for_user'][10]['bbp_format_buddypress_notifications'])) {
            $wp_filter['bp_notifications_get_notifications_for_user'][10]['bbp_format_buddypress_notifications']['function'] = 'likebtn_bbp_format_buddypress_notifications';
        }

        return $return;
    }

    return $action;
}
// bbPres has a bug: 
// https://bbpress.org/forums/topic/return-value-in-bbp_format_buddypress_notifications/
// https://buddypress.trac.wordpress.org/ticket/6669
// Filter must be called before corresponding bbPress filter
add_filter('bp_notifications_get_notifications_for_user', 'likebtn_notifications_get_notifications_for_user', 5, 5);

// Wrapper for bbp_format_buddypress_notifications function as it is not returning $action
function likebtn_bbp_format_buddypress_notifications($action, $item_id, $secondary_item_id, $total_items, $format = 'string')
{
    $result = bbp_format_buddypress_notifications($action, $item_id, $secondary_item_id, $total_items, $format);
    if (!$result) {
        $result = $action;
    }
    return $result;
}

#11 @r-a-y
8 years ago

Related: #7020.

#12 @r-a-y
8 years ago

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.