Make WordPress Core

Opened 2 years ago

Last modified 9 months ago

#56028 reviewing defect (bug)

Pass the user object instead of recall get user function

Reported by: pbearne's profile pbearne Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: Cc:

Description (last modified by pbearne)

IN wp_ajax_send_password_reset we have

<?php
        $user    = get_userdata( $user_id );
        $results = retrieve_password( $user->user_email );

then in retrieve_password we have

<?php
        if ( empty( $user_login ) ) {
                $errors->add( 'empty_username', __( '<strong>Error:</strong> Please enter a username or email address.' ) );
        } elseif ( strpos( $user_login, '@' ) ) {
                $user_data = get_user_by( 'email', trim( wp_unslash( $user_login ) ) );
                if ( empty( $user_data ) ) {
                        $errors->add( 'invalid_email', __( '<strong>Error:</strong> There is no account with that username or email address.' ) );
                }
        } else {
                $user_data = get_user_by( 'login', trim( wp_unslash( $user_login ) ) );
        }

This patch allows us to pass a user object and bypass the second call to find the user

Change History (10)

This ticket was mentioned in PR #2838 on WordPress/wordpress-develop by pbearne.


2 years ago
#1

  • Keywords has-patch has-unit-tests added

#2 @pbearne
2 years ago

  • Component changed from General to Users

this also fixes #53634

Last edited 9 months ago by SergeyBiryukov (previous) (diff)

#3 @pbearne
2 years ago

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

#4 follow-up: @spacedmonkey
2 years ago

What the benefit of passing the WP_User here? The WP_User is in cache here, so getting it out of cache isn't that bad for performance. What problem are we trying to solve here.

#5 @pbearne
2 years ago

  • Description modified (diff)

#6 in reply to: ↑ 4 @pbearne
2 years ago

Replying to spacedmonkey:

What the benefit of passing the WP_User here? The WP_User is in cache here, so getting it out of cache isn't that bad for performance. What problem are we trying to solve here.

It is a tidy-up and gives us another way to call the function
It came out of fixing #53634 where the fix was to change what I passed to this function Been able to pass the use it solve having to pre-select what field to use to find the user.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

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


2 years ago

#8 @davidbaumwald
2 years ago

  • Owner changed from pbearne to davidbaumwald
  • Status changed from assigned to reviewing

#9 @davidbaumwald
21 months ago

  • Keywords 2nd-opinion added

Thinking about this a bit more, I am a little concerned about changing the signature here based on its age and the fact that this shares commonality/consistency with the retrieve_password filter that's used elsewhere.

A directory search for retrieve_password shows some false positives(custom functions, etc.) and some point to the usage of the filter, not the function.

The spirit of this function is heavily influenced by the user's login, either passed directly or referenced in the current $_REQUEST vars.

Also, I agree with @spacedmonkey about the caching being used here, so I think the hit might be negligible.

I think it's possible to update the function to allow for WP_User object to be passed and grab the user_login prop if it was indeed passed.

Marking this for 2nd-opinion to hopefully get some guidance.

#10 @davidbaumwald
9 months ago

  • Owner davidbaumwald deleted
Note: See TracTickets for help on using tickets.