Skip to:
Content

BuddyPress.org

Opened 8 months ago

Last modified 6 months ago

#8081 new defect (bug)

BuddyPress breaks disabling notification for new users

Reported by: zodiac1978 Owned by:
Milestone: Awaiting Contributions Priority: normal
Severity: normal Version: 1.2.2
Component: Members Keywords: close
Cc:

Description

I've written a blog post about the problem (in German) here:
https://torstenlandsiedel.de/2018/09/13/benachrichtigungsmails-fuer-admins-abstellen/

Please see these tickets for more details:
https://core.trac.wordpress.org/ticket/36009#comment:14
https://buddypress.trac.wordpress.org/ticket/7597#comment:7

Summary:
Sine WordPress 4.6 wp_send_new_user_notifications has a new parameter which sets up who should get the notification. Default is both (user and admin). You can remove this function via remove_action and re-add a new function which has the default changed to user instead to disable the admin notification.

The problem is that BuddyPress didn't know this and makes its own thing in bp-members-functions.php:

<?php
if ( apply_filters( 'bp_core_send_user_registration_admin_notification', true ) ) {
    wp_new_user_notification( $user_id );
}

It did not use the third parameter and adds a different way to disable the notification via filter.

So you need an additional filter to disable it:

<?php
add_filter( 'bp_core_send_user_registration_admin_notification', '__return_false' );

Attachments (3)

8081.patch (1.0 KB) - added by imath 6 months ago.
8081.2.patch (1.0 KB) - added by imath 6 months ago.
8081.unittest.patch (1.7 KB) - added by imath 6 months ago.

Download all attachments as: .zip

Change History (10)

#1 @r-a-y
8 months ago

  • Component changed from Core to Members
  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to Awaiting Contributions

#2 @imath
6 months ago

  • Keywords has-patch reporter-feedback added; good-first-bug removed
  • Milestone changed from Awaiting Contributions to 5.0.0
  • Version set to 1.2.2

Hi @zodiac1978

My understanding is that you want BuddyPress to take in account the 3rd parameter of wp_new_user_notification(): the one WordPress 4.6 introduced.

This means that by default, the notification should be sent to both and that the filter can now be used to define who should receive the notification (user, admin, both or no one - using false).

Is this correct ? See attached patch.

@imath
6 months ago

@imath
6 months ago

#3 @imath
6 months ago

Just checked, we are not doing 'both' but ''. Only the admin is notified ;)

#4 @zodiac1978
6 months ago

Hi @imath,

this a step in the right direction. But we still need to add the additional filter.

I was hoping for a solution which is re-using the core action hook:

do_action( 'register_new_user', $user_id );
add_action( 'register_new_user', 'wp_send_new_user_notifications' );

So that removing the function from register_new_user and re-hooking with different setting is sufficient to solve this for core and buddypress.

Would this be possible? Or do I oversee something ...

#5 @imath
6 months ago

  • Keywords close added; has-patch reporter-feedback removed
  • Milestone changed from 5.0.0 to Awaiting Contributions

Hi @zodiac1978

I see. I've looked into it deeply. I'm afraid, this is something we shouldn't do, even the change I've added in the patch actually.

BuddyPress tries to handle new user registrations in a similar way whether you use a multisite configuration or not. The process is:

  1. The user signs up using a specific form where he can set his password
  2. He receives an email with an activation link
  3. He clicks on the link to activate his account
  4. He uses the password he set in point 1. to log in.

If we'd use the hook you were referring to, then WordPress would send an email to ask the user to set a new password for the non multisite configs. I've checked using a unit test. This is not consistent with our process.

I'm going to leave this ticket open to let another member of the team (like @r-a-y for instance 😉) eventually contradict me, but (sorry) to me we should close this ticket as wontfix

#6 @imath
6 months ago

FYI: just added the unit test (8081.unittest.patch) I've used to check what's happening after a member activates his account.

#7 @zodiac1978
6 months ago

Hi @imath,

thank you very much for this thorough explanation!

I'm perfectly fine with closing this issue if this is not possible. It would be great if @r-a-y can have a look too to confirm this.

As a support guy (moderated the German support forums for 5 years) I am just stumbled upton the missing documentation. If you search for disabling notifications you find the solutions to fix this in WP core. And then you are wondering why you still get notifications with BuddyPress installed. I think this could be improved (talking UX and user/admin expectations).

How about adding an option? So that Admins can configure this in BuddyPress itself? And they do not have to find two different solutions to fix this (WP core action and BuddyPress filter)?

Another possible solution could be to add an extra filter just for the notification setting itself (user, admin, both or no one - using false) which is used from core *and* BuddyPress.

Thanks for considering!

Note: See TracTickets for help on using tickets.