Discrepancy between documentation and implementation of wp_new_user_notification()

[360zen]

Based on the documentation, the options for recipients are 'admin', 'user', and 'both'. However, the way to send an email to both admin and user is actually to enter any arbitrary value for at least one of the two unnecessary parameters.

wp_new_user_notification($user_id) or wp_new_user_notification($user_id, null, 'admin')sends only the admin email.

wp_new_user_notification($user_id, null, 'user') sends only the user email.

wp_new_user_notification($user_id, null, 'anythingYouWant') sends both.

One of two things is happening here:

1. The function is more permissive than intended, allowing any value in the $notify parameter. Perhaps the function should actually check for 'both' instead of simply returning if the two parameters are empty. OR

2. The documentation is making an unnecessary reference to the word 'both' where in reality, any string except 'admin' and 'user' will result in both emails being sent. Perhaps the documentation should be changed to suggest that acceptable values are 'admin', 'user' or literally any other string that returns false on the empty() test on line 1873 of wp-includes/pluggable.php.

[kraftbj]

Howdy! Thanks for noticing and for writing up a report.

If either option is chosen, I'd go with option 1. Option 2 would make any future changes more difficult since there isn't a canonically stated way to utilize the function (e.g. if we add a new option for the third argument of "alladmins" (to send a notice all users with the role of admin), for example, that could create headaches for anyone who happened to utilize that as it is.

The change goes back to 4.3 with the new stronger password UI and flow. Previously, the second arg was the plaintext password and, if present, it would be sent in plaintext via e-mail. If the arg was empty, an e-mail would not be sent out. So, the code is reflecting the old method of the second argument, which was added back when adding the third argument to provide a way to control the notification.

Let's validate both, admin, and user to prevent something unexpected working and putting us into a corner later.

@360zen are you interested in contributing a patch?

[360zen]

I'd love to make a contribution. As it would be my first, I may need a little guidance, but I will make sure to read the contribution guidelines and materials prior to bugging anyone with mundane questions.

A couple points of clarification. The description of the function says "Email login credentials to a newly-registered user." However, as it is now, the default functionality, if no $notify parameter is set, is to email only the admin. This seems broken to me, as the point of the function is to send a new user their credentials (or in the newer versions of WP, their password reset link).

I would propose that, by default, the function should send both emails when called with only the $user_id parameter. So wp_new_user_notification($user_id) would send both. This could be achieved by changing the default of the $notify parameter from '' to 'both' or modifying the logic used to return early. However, this could potentially cause some unexpected emails to go out to users if anyone is using the short version of the call ( wp_new_user_notification($user_id)) expecting to send only admin emails.

Do you have any thoughts about how much trouble that would cause? IMO, it seems worth it to have the function behave intuitively, but I understand the need for caution with changes like this.

[360zen]

Replying to 360zen:

I would propose that, by default, the function should send both emails when called with only the $user_id parameter. So wp_new_user_notification($user_id) would send both. This could be achieved by changing the default of the $notify parameter from '' to 'both' or modifying the logic used to return early. However, this could potentially cause some unexpected emails to go out to users if anyone is using the short version of the call ( wp_new_user_notification($user_id)) expecting to send only admin emails.

After further review and discussion with another dev, I've concluded that changing the default behavior here is probably ill-advised. However, there should still be better checking of the $notify parameter. I'll plan to submit a patch in the next week or so with that change unless directed otherwise by someone.

[cthreelabs]

Patch added , Added snippet to check if notify is not empty and check for 'user','admin' or both as expected string

[cthreelabs]

Fixed issue from earlier path and tested , Allows default ' ', both,admin,and user as notify params and made as case sensitive.

[cthreelabs]

Unit test file , tests the block of the code added in the path, test is made in such a way that allowed param in notify should fail .

[cthreelabs]

Hi @360zen thanks for reporting the issues, a patch with unit test has been submitted, hope this would resolve the issue. thanks.

[360zen]

Replying to cthreelabs:

Hi @360zen thanks for reporting the issues, a patch with unit test has been submitted, hope this would resolve the issue. thanks.

Great! I had been waiting for feedback on this, but I hadn't noticed a patch had been submitted. Looks good to me. Thanks.

[swissspidy]

Few notes on the patch:

• The patch doesn't follow the core coding standards (esp. missing spacing), see ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/
• A simple if ( ! in_array( $notify, array( 'user', 'admin', 'both', '' ), true ) ) { return; } would do the trick. No other check is needed.
• The unit test doesn't really test the wp_new_user_notification function. It basically tests whether in_array works.
• We already have tests for wp_new_user_notification using a data provider (see tests/phpunit/tests/user.php). This could be extended with a test using jibrish as an input.

[cthreelabs]

Corrections made based on input form @swissspidy, used WPCS linter hope the patch follows the recommended coding standard.

[cthreelabs]

Unit test extending user.php, tests the wp_new_user_notification().

[cthreelabs]

Replying to swissspidy:
Thanks for the useful feedback, I have modified the patch and used wpcs linters to avoid wp coding standard issues. I have also updated the test to check the wp_new_user_notificaion function. Hope this is helpful.

Few notes on the patch:

• The patch doesn't follow the core coding standards (esp. missing spacing), see ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/
• A simple if ( ! in_array( $notify, array( 'user', 'admin', 'both', '' ), true ) ) { return; } would do the trick. No other check is needed.
• The unit test doesn't really test the wp_new_user_notification function. It basically tests whether in_array works.
• We already have tests for wp_new_user_notification using a data provider (see tests/phpunit/tests/user.php). This could be extended with a test using jibrish as an input.

[SergeyBiryukov]

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

[pento]

In 44611:

Users: Add extra checking to wp_new_user_notification().

Prevent a notification from being sent when an unrecognised value is passed in the $notify parameter.

Props cthreelabs, 360zen.
Fixes #44293.