WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#33654 closed defect (bug) (fixed)

wp_new_user_notification breaking change.

Reported by: alexander.rohmann Owned by: ocean90
Milestone: 4.3.1 Priority: normal
Severity: normal Version: 4.3
Component: Users Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

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.

Attachments (7)

33654.diff (3.5 KB) - added by kraftbj 4 years ago.
33654.2.diff (3.5 KB) - added by kraftbj 4 years ago.
Missed the mark on inline doc. null is of the null type, not a bool.
33654.3.diff (3.5 KB) - added by adamsilverstein 4 years ago.
33654-tests.diff (1.7 KB) - added by welcher 4 years ago.
Adding unit tests
33654-tests.2.diff (2.7 KB) - added by welcher 4 years ago.
Updated unit tests with docblocks and error messaging should a test fail
33654.4.diff (7.6 KB) - added by adamsilverstein 4 years ago.
33654.5.diff (7.0 KB) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (32)

#1 @SergeyBiryukov
4 years ago

  • Component changed from General to Users
  • Milestone changed from Awaiting Review to 4.3.1

This ticket was mentioned in Slack in #core-passwords by binarykitten. View the logs.


4 years ago

#3 @mordauk
4 years ago

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.

#4 @chriscct7
4 years ago

  • Keywords needs-patch dev-feedback added

#5 @alexander.rohmann
4 years ago

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
4 years ago

#6 @kraftbj
4 years ago

  • Keywords has-patch added; needs-patch removed

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
4 years ago

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

This ticket was mentioned in Slack in #core-passwords by sam. View the logs.


4 years ago

#8 @adamsilverstein
4 years ago

  • Keywords needs-unit-tests added

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}?

#9 follow-up: @kraftbj
4 years ago

@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
4 years ago

Adding unit tests

#11 @welcher
4 years ago

  • Keywords needs-unit-tests removed

This ticket was mentioned in Slack in #core-passwords by sam. View the logs.


4 years ago

@welcher
4 years ago

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

#13 @welcher
4 years ago

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

This ticket was mentioned in Slack in #core-passwords by welcher. View the logs.


4 years ago

#15 in reply to: ↑ 9 @adamsilverstein
4 years ago

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.

#16 follow-up: @adamsilverstein
4 years ago

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

#17 @adamsilverstein
4 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@ocean90
4 years ago

#18 in reply to: ↑ 16 ; follow-up: @ocean90
4 years ago

  • Keywords dev-feedback removed

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.

#19 @ocean90
4 years ago

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

Last edited 4 years ago by ocean90 (previous) (diff)

#20 in reply to: ↑ 18 @adamsilverstein
4 years ago

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.

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


4 years ago

#22 @chriscct7
4 years ago

  • Keywords commit added
  • Owner changed from adamsilverstein to chriscct7
  • Status changed from assigned to accepted

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

Moving to commit requisitioning

#23 @ocean90
4 years ago

  • Owner changed from chriscct7 to ocean90

#24 @ocean90
4 years ago

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

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)

#25 @ocean90
4 years ago

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.

Note: See TracTickets for help on using tickets.