Make WordPress Core

Opened 14 years ago

Closed 10 years ago

#14308 closed defect (bug) (wontfix)

wp_insert_user in 3.0 is not backwards compatible

Reported by: ahupp's profile ahupp Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Users Keywords:
Focuses: Cc:

Description

Prior to WP3.0 the function wp_insert_user would always return an integer or falsy on failure. In 3.0 this function can return either an integer, or an instance of WP_Error. This behavior is not backwards compatible and can result in bad results.

A function that expects an integer return value will treat this WP_Error instance as an integer, which results in a '1'. This is the id of the administrator, resulting in possible corruption of the admin account. In particular, calling wp_update_user() with this WP_Error value will cast the error to (int) and operate on the administrator. First line of wp_update_user:

$ID = (int) $userdataID?;

Possible resolutions:

best: don't return WP_Error from wp_insert_user - this is not backwards compatible.
otherwise: check for is_wp_error() in wp_update_user(), and every other function that takes a user id.

Attachments (1)

wp_insert_user.error-catch.14308.diff (1.4 KB) - added by filosofo 14 years ago.

Download all attachments as: .zip

Change History (8)

#1 @filosofo
14 years ago

  • Component changed from General to Users
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.1

Probably best to go with catching the error at this point, since it's already out there.

#2 @coffee2code
14 years ago

Also see the related #14290.

There I'm fixing a downstream manifestation of this change introduced in 3.0 (register_new_user() in wp-login.php was expecting an int instead of a possible WP_Error).

We have to decide if we want to support the older (int) return value (as patched in this ticket) or allow the functions to potentially return WP_Error and change things accordingly within core. As filosofo pointed out, it's already out there, so people are likely building stuff with wp_insert_user() and wp_create_user() now possibly returning WP_Error.

I favor keeping the new behavior and having everything return a WP_Error rather than 0 if something is wrong. This creates consistency with the return values between wp_create_user(), wp_insert_user(), and wp_update_user(). We already broke backwards compatibility with the 3.0 change to wp_insert_user() (which I don't believe anyone is suggesting should be reverted), so there isn't much more harm in doing the same for the other two related functions (as long as we make sure it is properly handled throughout the code).

Bear in mind too that the WP_Error object is passing along the error message. We'll lose (and prevent outside code from recognizing) potentially beneficial information by simply swallowing it and returning 0.

I fear the consensus may be to break 3.0 compatibility to resume pre-3.0 compatibility for 2 of the 3 related functions. In which case some may want to roll with something equivalent to:

function is_valid_user( $user ) {
  return ( !is_wp_error( $user ) && $user );
}

#3 @ahupp
14 years ago

  • Cc ahupp added

I was actually suggesting reverting wp_insert_user() to 2.0 behavior, but I appreciate that has its own difficulties. Thanks for the quick response.

Another option is to make multiple versions of this function, one with verbose error reporting and one without. wp_insert_user_ex() would return int|WP_Error, and wp_insert_error() would continue to return 0 on error. I expect there is more code out there expecting the 2.0 behavior than the 3.0 behavior.

-Adam

#4 @nacin
14 years ago

Cross referencing #14767.

#5 @ryan
13 years ago

  • Milestone changed from 3.1 to Future Release
  • Resolution set to fixed
  • Status changed from new to closed

#6 @ryan
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#7 @johnbillion
10 years ago

  • Keywords has-patch removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

This ship has sailed.

Note: See TracTickets for help on using tickets.