WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 5 months ago

#44293 closed defect (bug) (fixed)

Discrepancy between documentation and implementation of wp_new_user_notification()

Reported by: 360zen Owned by: pento
Milestone: 5.1 Priority: normal
Severity: minor Version: 4.3
Component: Users Keywords: good-first-bug has-patch dev-feedback needs-unit-tests needs-refresh
Focuses: Cc:

Description

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
  1. 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.

Attachments (5)

44293.1.diff (542 bytes) - added by cthreelabs 12 months ago.
Patch added , Added snippet to check if notify is not empty and check for 'user','admin' or both as expected string
44293.2.diff (576 bytes) - added by cthreelabs 12 months ago.
Fixed issue from earlier path and tested , Allows default ' ', both,admin,and user as notify params and made as case sensitive.
44293.php (1.4 KB) - added by cthreelabs 12 months ago.
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 .
44293.3.diff (555 bytes) - added by cthreelabs 12 months ago.
Corrections made based on input form @swissspidy, used WPCS linter hope the patch follows the recommended coding standard.
class-tests-44293.php (1.0 KB) - added by cthreelabs 12 months ago.
Unit test extending user.php, tests the wp_new_user_notification().

Download all attachments as: .zip

Change History (20)

#1 @kraftbj
13 months ago

  • Component changed from Mail to Users
  • Version changed from 4.9.6 to 4.3

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?

#2 @kraftbj
13 months ago

  • Keywords good-first-bug added

#3 follow-up: @360zen
13 months ago

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.

#4 in reply to: ↑ 3 @360zen
13 months ago

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
12 months ago

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

#5 @cthreelabs
12 months ago

  • Keywords has-patch added

@cthreelabs
12 months ago

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

@cthreelabs
12 months ago

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 .

#6 @cthreelabs
12 months ago

  • Keywords has-unit-tests added

#7 follow-up: @cthreelabs
12 months ago

  • Keywords dev-feedback added

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

#8 in reply to: ↑ 7 @360zen
12 months ago

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.

This ticket was mentioned in Slack in #core by cthreelabs. View the logs.


12 months ago

#10 follow-up: @swissspidy
12 months ago

  • Keywords needs-unit-tests needs-refresh added; has-unit-tests removed
  • Milestone changed from Awaiting Review to Future Release

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
12 months ago

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

@cthreelabs
12 months ago

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

#11 in reply to: ↑ 10 @cthreelabs
12 months ago

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.

#12 @SergeyBiryukov
12 months ago

  • Milestone changed from Future Release to 5.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#13 @SergeyBiryukov
9 months ago

  • Milestone changed from 5.0 to 5.1

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

#14 @pento
5 months ago

  • Owner changed from SergeyBiryukov to pento
  • Status changed from reviewing to accepted

#15 @pento
5 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.