wp_new_user_notification breaking change.

[alexander.rohmann]

In 4.3 the $plaintext_pass argument of wp_new_user_notification wasn't deprecated properly, and instead was repurposed to do something else.

Because this is a pluggable function, it's a pretty big breaking change. You have all these plugged modifications out there that are essentially sending this:

Username: correct
Password: both

Seeing your password as "both" is very confusing for end users, and what's worse is that in many cases they're unable to login at all.

$plaintext_pass should have been deprecated, and $notify should have been introduce as a new argument.

[mordauk]

This broke an email in Easy Digital Downloads: ​https://github.com/easydigitaldownloads/Easy-Digital-Downloads/blob/master/includes/user-functions.php#L442

When accounts are registered, we send a welcome email like this: wp_new_user_notification( $user_id, __( '[Password entered at checkout]', 'edd' ) );

That doesn't work any more.

[alexander.rohmann]

Honestly, I also find it odd that the activation code is generated in a function named wp_new_user_notification. I would kinda expect with a function named that way that it's responsible for sending a "notification" based on a user_id, not update my database and prepare an activation key.

[kraftbj]

Added first pass at a patch. That said, either way plugins would need to patch themselves to not expect the plaintext password so the benefit that I see is a blank password in the e-mail is perhaps less confusing than "both".

[kraftbj]

Missed the mark on inline doc. null is of the null type, not a bool.

[adamsilverstein]

I reviewed the patch, and the previous work on #33358 that led to the function change. The _deprecated_argument call is perfect. I think I see a stray comma in the function definition for wp_new_user_notification (L1701)

33654.3.diff​:

• Refresh against trunk
• Remove stray comma

Some unit tests would be nice here and would prevent us from breaking this again.

Should we consider sending a filler instead of blank? something like {plaintext password removed}?

[kraftbj]

@adamsilverstein: Are you thinking something along the lines of

wp_new_user_notification( $user_id, 
       apply_filter('password_removed_text', __( '{plaintext password removed}' ) ), 
       'both' );

Then update the function to throw deprecated if anything else is returned besides the filtered value? We can't change the function itself since it is pluggable.

[welcher]

Adding unit tests

[welcher]

Updated unit tests with docblocks and error messaging should a test fail

[welcher]

Updated unit tests added. Inline docs and adding the context parameter for asserts should they fail.

[adamsilverstein]

Actually, I don't think we need this now that I understand the issue fully.

Replying to kraftbj:

@adamsilverstein: Are you thinking something along the lines of

wp_new_user_notification( $user_id, 
       apply_filter('password_removed_text', __( '{plaintext password removed}' ) ), 
       'both' );

Then update the function to throw deprecated if anything else is returned besides the filtered value? We can't change the function itself since it is pluggable.

[adamsilverstein]

In 33654.4.diff​:

• Roll up the patches including latest from @welcher
• Add an additional test verifying calling function with second argument thrown the deprecated error as expected
• $plain_text is now deprecated, and $notify is now the (optional) third argument for wp_new_user_notification

[ocean90]

Replying to adamsilverstein:

In 33654.4.diff​:

• Add an additional test verifying calling function with second argument thrown the deprecated error as expected

@expectedDeprecated takes care of the deprecation notice, $this->deprecated_called isn't needed. Please have also in mind, that unit tests have to pass on PHP 5.2 too, so no closures in tests.

[ocean90]

@DrewAPicture Can you please take a look at 33654.5.diff​? Does the last sentence for $notify still make sense?

[adamsilverstein]

Good to know that @expectedDeprecated takes care of that, I'll read up to learn more. Also, thanks for pointing out 5.2 requirement, wasn't sure that was required for unit tests.

I did know my approach was convoluted, so thanks for cleaning that all up!

Replying to ocean90:

Replying to adamsilverstein:

In 33654.4.diff​:

• Add an additional test verifying calling function with second argument thrown the deprecated error as expected

@expectedDeprecated takes care of the deprecation notice, $this->deprecated_called isn't needed. Please have also in mind, that unit tests have to pass on PHP 5.2 too, so no closures in tests.

[chriscct7]

@DrewAPicture is good with the docs:
​https://wordpress.slack.com/archives/core/p1442212363001334

Moving to commit requisitioning

[ocean90]

In 34116:

Passwords: Deprecate second parameter of wp_new_user_notification().

The second parameter $plaintext_pass was removed in [33023] and restored as $notify in [33620] with a different behavior. If you have a plugin overriding wp_new_user_notification() which hasn't been updated you would get a notification with your username and the password "both".
To prevent this the second parameter is now deprecated and reintroduced as the third parameter.

Adds unit tests.

Props kraftbj, adamsilverstein, welcher, ocean90.
Fixes #33654.

(Don't ask for new pluggables kthxbye)

[ocean90]

In 34118:

Passwords: Deprecate second parameter of wp_new_user_notification().

The second parameter $plaintext_pass was removed in [33023] and restored as $notify in [33620] with a different behavior. If you have a plugin overriding wp_new_user_notification() which hasn't been updated you would get a notification with your username and the password "both".
To prevent this the second parameter is now deprecated and reintroduced as the third parameter.

Adds unit tests.

Merge of [34116] to the 4.3 branch.

Props kraftbj, adamsilverstein, welcher, ocean90.
See #33654.

[johnbillion]

Follow-up: #33867