WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 3 weeks ago

#44293 new defect (bug)

Discrepancy between documentation and implementation of wp_new_user_notification()

Reported by: 360zen Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 4.3
Component: Users Keywords: good-first-bug
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.

Change History (4)

#1 @kraftbj
3 weeks 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
3 weeks ago

  • Keywords good-first-bug added

#3 follow-up: @360zen
3 weeks 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
3 weeks 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.

Note: See TracTickets for help on using tickets.