Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#45746 closed enhancement (fixed)

get_password_reset_key should use wp_update_user

Reported by: spacedmonkey's profile spacedmonkey Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: good-first-bug has-patch needs-testing
Focuses: Cc:

Description

In the function, get_password_reset_key, the user record is updated, however this function call doesn't use wp_update_user (wp_insert_user), none of the actions or filters are run. Worse yet, the clean_user_cache function and actions are not called.

Attachments (2)

45746.diff (1.3 KB) - added by jayswadas 6 years ago.
45746-2.diff (1.3 KB) - added by jayswadas 6 years ago.

Download all attachments as: .zip

Change History (13)

#2 @spacedmonkey
6 years ago

  • Component changed from General to Users

@jayswadas
6 years ago

#3 @jayswadas
6 years ago

  • Keywords has-patch added; needs-patch removed

#4 @spacedmonkey
6 years ago

Thanks @jayswadas for the patch.

Looking at the patch, there a couple of issues, I had with it. I would change it the following

$key_saved = wp_update_user( array( 'ID' => $user->ID, 'user_activation_key' => $hashed, 'user_login' => $user->user_login ) );
if ( is_wp_error( $key_saved ) ) {
   return $key_saved;
}

Couple of differences.

  1. Removed clean_user_cache call, as this is already classed in wp_update_user
  2. Check the value $key_saved and return. As wp_update_user doesn't allows successfully.

If you can update the patch @jayswadas and run the phpunit tests, we could nearly be there.

@jayswadas
6 years ago

#5 @jayswadas
6 years ago

  • Keywords needs-testing added

#6 @donmhico
5 years ago

@spacedmonkey I applied the latest patch submitted by @jayswadas (45746-2.diff) and run the phpunit tests.

This is the phpunit output.

OK, but incomplete, skipped, or risky tests!
Tests: 9634, Assertions: 73861, Skipped: 39.

I'm not entirely sure if this is good enough because of the skipped tests.

Last edited 5 years ago by donmhico (previous) (diff)

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


5 years ago

#8 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @garrett-eclipse
5 years ago

Hi @donmhico if there are no failing tests then you should be good to go. A skipped test occurs when your environment doesn't support the test those are usually tests for specific database drivers or other environment-specific tests.

#10 @SergeyBiryukov
5 years ago

In 45713:

Users: Reorganize user_nicename, user_url, user_registered setting in wp_insert_user() for consistency with the order or fields in the database.

See #45746.

#11 @SergeyBiryukov
5 years ago

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

In 45714:

Users: Use wp_update_user() in get_password_reset_key().

Props jayswadas, spacedmonkey, donmhico, SergeyBiryukov.
Fixes #45746.

Note: See TracTickets for help on using tickets.