[wp-trac] [WordPress Trac] #58407: resetpassword action on users.php (users list page) handles retrieve_password() return incorrectly

[WordPress Trac]

#58407: resetpassword action on users.php (users list page) handles
retrieve_password() return incorrectly
-------------------------------------------------+-------------------------
 Reporter:  letraceursnork                       |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  6.4
Component:  Users                                |     Version:  6.2.2
 Severity:  trivial                              |  Resolution:
 Keywords:  good-first-bug has-patch needs-      |     Focuses:  ui,
  testing                                        |  administration
-------------------------------------------------+-------------------------
Changes (by hellofromTonya):

 * keywords:  good-first-bug has-patch changes-requested => good-first-bug
     has-patch needs-testing


Comment:

 Consistency - which to use?

 Example from [source:tags/6.3.2/src/wp-admin/includes/ajax-
 actions.php?marks=5595-5597#L5595 wp-admin/includes/ajax-actions.php]:
 > {{{
 > if ( true === retrieve_password( $user->user_login ) ) {
 >       wp_send_json_success(
 > } else {
 >       wp_send_json_error(
 > }
 > }}}

 The `true` comparison check is used for the success branch, with the
 `else` handling the error. That order makes for what it's doing: if
 success, else error.

 Example from [source:tags/6.3.2/src/wp-login.php?marks=796#L791 wp-
 login.php]:
 > {{{
 > $errors = retrieve_password();
 >
 > if ( ! is_wp_error( $errors ) ) {
 >       // redirect and exit;
 > }
 > // else process the errors.
 > }}}

 In this example, the code is designed to process the errors, i.e.
 `$errors` is expected to be an instance of `WP_Error`. But if not an
 error, then it redirects and exits.

 Yes, both of these examples are inconsistent in how they check for the
 return value from `retrieve_password()`, but both are appropriate for how
 the code is designed. I think inconsistency here is by design and okay.

 **Which should this new bugfix?**

 Only 2 values will be returned: `true` or an instance of `WP_Error`.

 `true` does not necessarily mean the email was sent, as it can be short-
 circuited by hooking into one or these filters
 `'send_retrieve_password_email'` or `'retrieve_password_message'` as
 @oglekler notes in comment:32. Looking at how it's documented in the
 `@return` annotation, returning `true` means "finished":
 {{{
 * @return true|WP_Error True when finished, WP_Error object on error.
 }}}
 where "finished" may or may not actually mean an email was sent. Huh,
 interesting. I suspect that's by-design. But if no and error or different
 value should be returned when short-circuiting, that would be out-of-scope
 for fixing this ticket. It is an interesting question though that would
 take a contextual dive back in time to review the changesets and tickets.

 Okay, so `true` means it "finished".

 For this bugfix, I think `true` is the information needed to determine
 whether or not to increment the reset count:

 {{{
 if ( true === retrieve_password( $user->user_login ) ) {
         ++$reset_count;
 }
 }}}

 If an instance of `WP_Error` is returned, then the count is not
 incremented. If later `retrieve_password()` is changed to where `true` no
 longer includes short-circuit, this bugfix change should still be valid.

 [attachment:"58407.diff"] is then ready for final testing and commit
 consideration.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/58407#comment:35>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform