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

[jrf]

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().

[hellofromTonya]

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 )
        );
}

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

[jrf]

PR ​GH #3344 is open to address this.

[hellofromTonya]

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

[hellofromTonya]

Preparing the commit.

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

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

[hellofromTonya]

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.

Option 2 was committed via https://core.trac.wordpress.org/changeset/55325.

Committed via https://core.trac.wordpress.org/changeset/55325.

[jrf]

Thanks @hellofromTonya !