WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#33358 closed defect (bug) (fixed)

Passwords: Second param of wp_new_user_notification() has been removed

Reported by: ocean90 Owned by: markjaquith
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Users Keywords: has-patch commit
Focuses: Cc:

Description (last modified by ocean90)

Originally reported by @Justin_K in ticket:33209:9.

In 4.2 wp_new_user_notification() has a second param $plaintext_pass. If it's not set the user won't get a notification, only the site admin. Now in trunk this param has been removed ([33023]) and the user get's always an email, see ticket:33209:9.

Attachments (4)

33358.diff (1.6 KB) - added by markjaquith 5 years ago.
Simple option — remove the calls.
33358.2.diff (3.4 KB) - added by ocean90 5 years ago.
untested
33358.4.diff (3.6 KB) - added by markjaquith 5 years ago.
33358.5.diff (3.8 KB) - added by ocean90 5 years ago.
Adds changelog entry to wp_new_user_notification()

Download all attachments as: .zip

Change History (20)

#1 @ocean90
5 years ago

  • Description modified (diff)
  • Keywords dev-feedback added

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


5 years ago

#3 @obenland
5 years ago

  • Owner set to markjaquith
  • Status changed from new to reviewing

@markjaquith
5 years ago

Simple option — remove the calls.

@ocean90
5 years ago

untested

#4 @obenland
5 years ago

  • Keywords has-patch needs-testing added; dev-feedback removed

#5 follow-up: @markjaquith
5 years ago

@ocean90 what's the thought behind 'both' ? Why not just have it be a boolean $email_user flag?

#6 in reply to: ↑ 5 @ocean90
5 years ago

Replying to markjaquith:

@ocean90 what's the thought behind 'both' ? Why not just have it be a boolean $email_user flag?

Mainly because of this http://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#self-explanatory-flag-values-for-function-arguments.

I think we need to take a step back here first:

  • Was it a good idea to change a pluggable function like this?
  • Shouldn't we introduce a new function instead?
  • If not, can we reuse the second param like we want to do or do we have to introduce a third one?
  • Or can we change it to $args[] and do a type check?
  • How are plugins using this function? (Thanks Helen for the list.)

#7 follow-up: @valendesigns
5 years ago

@ocean90 what other values would this param except as valid besides both?

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


5 years ago

#9 in reply to: ↑ 7 @markjaquith
5 years ago

Replying to valendesigns:

@ocean90 what other values would this param except as valid besides both?

We could add 'admin' as an alias for '' (empty, which will skip the user e-mail). And then for back compat, any other non-empty value means 'both' (as the patch currently does). Doing that now.

@markjaquith
5 years ago

#10 follow-up: @markjaquith
5 years ago

There.

I'm also not worried about this being pluggable. Things overriding pluggable functions are on their own.

#11 in reply to: ↑ 10 @valendesigns
5 years ago

We could add 'admin' as an alias for (empty, which will skip the user e-mail). And then for back compat, any other non-empty value means 'both' (as the patch currently does). Doing that now.

Sounds good to me.

I'm also not worried about this being pluggable. Things overriding pluggable functions are on their own.

I agree, you're on your own if you want to override pluggable functions.

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


5 years ago

#13 @helen
5 years ago

  • Keywords commit added; needs-testing removed

Looks good to me. That's a hard argument to try to understand without having worked on this at all :)

@ocean90
5 years ago

Adds changelog entry to wp_new_user_notification()

#14 @obenland
5 years ago

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

In 33620:

Passwords: Restore second parameter for wp_new_user_notification().

After [33023] users would always be notified, this restores previous behavior.

Props markjaquith, ocean90.
Fixes #33358.

#15 @ocean90
5 years ago

Follow-up: #33654

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


5 years ago

Note: See TracTickets for help on using tickets.