Make WordPress Core

Opened 20 months ago

Closed 15 months ago

Last modified 15 months ago

#56662 closed defect (bug) (fixed)

WP_REST_Users_Controller::update_item(): 'rest_user_invalid_id ' error will never be thrown as `$user` will never be falsey.

Reported by: jrf's profile jrf Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch commit
Focuses: Cc:

Description

While looking at the code of the WP_REST_Users_Controller::update_item() method, I noticed a bug in WP_REST_Users_Controller::update_item().

At the start of the function, the user is retrieved and it is verified that this doesn't result in a WP_Error.

Next, the WP_User $user variable is used to retrieve the user id.

Then in the next condition, it is checked if the $user is falsey. This condition would never match as WP_REST_Users_Controller::get_user() returns either a WP_User object or WP_Error and we already know it's not a WP_Error and an instantiated WP_User object will never evaluate to falsey.

As the WP_Error being thrown _within_ the condition refers to an "invalid user id", I believe the _intention_ of the code was to check that $id is not falsey, which would make more sense, as even though it would be unlikely that a WP_User object would not have a valid ID, that condition _could_ potential match and trigger the WP_Error.

There are already tests in place which (try to) cover this code, but as the WP_REST_Users_Controller::get_user() method throws the same error, it went unnoticed that this condition is incorrect.

In all honesty, IMO, this whole condition can be removed, as it is already handled by the call to WP_REST_Users_Controller::get_user().

Change History (13)

#1 @hellofromTonya
20 months ago

  • Owner set to hellofromTonya
  • Status changed from new to accepted

Additional contextual information:

  • The conditional check was introduced in [38832]:
    $user = get_userdata( $id );
    if ( ! $user ) {
            return new WP_Error( 'rest_user_invalid_id', __( 'Invalid resource id.' ), array( 'status' => 404 ) );
    }
    

Notice that it was checking what's returned from get_userdata() which returns WP_User on success or false on failure.

  • Changeset [39954] added the WP_REST_Users_Controller::get_user() method (which returns WP_Error on failure and throws the 'rest_user_invalid_id' error) and a is_wp_error() conditional check. But it retained the original error code from [38832].

Looking at this history, changeset [33954] moved error code logic and ensured a valid WP_User is returned.

I agree with @jrf that this code in WP_REST_Users_Controller::update_item() can safely be removed, as the conditional will never be falsey and the error will never be thrown.

if ( ! $user ) {
        return new WP_Error(
                'rest_user_invalid_id',
                __( 'Invalid user ID.' ),
                array( 'status' => 404 )
        );
}

This ticket was mentioned in PR #3344 on WordPress/wordpress-develop by jrfnl.


20 months ago
#2

  • Keywords has-patch added

As per https://github.com/WordPress/wordpress-develop/pull/3188#discussion_r981041394, this PR contains two commits to show the different solution directions.

---

### WP_REST_Users_Controller::update_item(): bug fix

While looking at the code of the WP_REST_Users_Controller::update_item() method, I also noticed another bug.

At the start of the function, the user is retrieved and it is verified that this doesn't result in a WP_Error.
Next, the WP_User $user variable is used to retrieve the user id.

Then in the next condition, it is checked if the $user is falsey. This condition would never match as WP_REST_Users_Controller::get_user() returns either a WP_User object or WP_Error and we already know it's not a WP_Error and an instantiated WP_User object will never evaluate to falsey.

As the WP_Error being thrown _within_ the condition refers to an "invalid user id", I believe the _intention_ of the code was to check that $id is not falsey, which would make more sense, as even though it would be unlikely that a WP_User object would not have a valid ID, that condition _could_ potential match and trigger the WP_Error.

There are already tests in place which (try to) cover this code, but as the WP_REST_Users_Controller::get_user() method throws the same error, it went unnoticed that this condition is incorrect.

In all honesty, IMO, this whole condition can be removed as it is already handled by the call to WP_REST_Users_Controller::get_user()....

### WP_REST_Users_Controller::update_item(): remove unnecessary condition

The condition could never be true, so is redundant.

See the commit message of the first commit and the discussion in PR #3188 for full details.

Trac ticket: https://core.trac.wordpress.org/ticket/56662

#3 @jrf
20 months ago

PR GH #3344 is open to address this.

#4 @SergeyBiryukov
20 months ago

  • Milestone changed from Awaiting Review to 6.2

#5 @hellofromTonya
20 months ago

  • Component changed from Users to REST API

#6 @hellofromTonya
20 months ago

  • Keywords commit added

I'm adding commit for the 2nd PR option that removes the dead code (ie the code that will never run).

#7 @hellofromTonya
15 months ago

  • Status changed from accepted to assigned

Preparing the commit.

This ticket was mentioned in PR #4066 on WordPress/wordpress-develop by @hellofromTonya.


15 months ago
#8

Follow-up to https://github.com/WordPress/wordpress-develop/pull/3344. Adjustment is needed for https://github.com/WordPress/wordpress-develop/pull/3344/commits/4d9abf9941a2ce9c79a9661488616fd55a0b6a31 as it was applied against option 1.

This PR is a confidence check to run the change in the CI jobs with the latest version trunk.

Trac ticket: https://core.trac.wordpress.org/ticket/56662

@hellofromTonya commented on PR #3344:


15 months ago
#9

https://github.com/WordPress/wordpress-develop/pull/4066 is a follow-up PR to remove the redundant code.

#10 @hellofromTonya
15 months ago

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

In 55325:

REST API: Remove 'Invalid user ID' error in WP_REST_Users_Controller::update_item().

Removes the WP_Error code for 'Invalid user ID.'. Why?

tl;dr
This branch will never be entered as the $user will never be falsey.

Longer reasoning:

[39954] introduced WP_REST_Users_Controller::get_user() method to encapsulate getting the user and handling the 'Invalid user ID.' WP_Error. It replaced get_userdata() in WP_REST_Users_Controller::update_item() but left the existing 'Invalid user ID.' WP_Error introduced in [38832].

The code removed in this changeset will never be reached because $user will never be falsey. Rather, WP_REST_Users_Controller::get_user() will always return an instance of WP_Error or WP_User.

Could the user's ID be falsey?
No. Why? WP_REST_Users_Controller::get_user() checks that the user exists, which checks if the ID is falsey.

Therefore, the code can safely be removed.

Follow-up to [39954], [38832].

Props jrf, costdev, hellofromTonya, SergeyBiryukov.
Fixes #56662.

#13 @jrf
15 months ago

Thanks @hellofromTonya !

Note: See TracTickets for help on using tickets.