Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#22858 closed defect (bug) (fixed)

wp_update_user() does not check the return of get_userdata() before calling to_array()

Reported by: ryan's profile ryan Owned by: nacin's profile nacin
Milestone: 3.5.1 Priority: normal
Severity: normal Version: 3.5
Component: Users Keywords: has-patch commit
Focuses: Cc:

Description

get_userdata() can return false, resulting in "Call to a member function to_array() on a non-object" when to_array() is called. This is triggered very rarely, but it does happen.

Attachments (2)

user.diff (414 bytes) - added by n7studios 11 years ago.
22858.patch (414 bytes) - added by SergeyBiryukov 11 years ago.
With proper formatting

Download all attachments as: .zip

Change History (18)

#1 @nacin
11 years ago

  • Milestone changed from Future Release to 3.5.1

@n7studios
11 years ago

#2 @lukbarros
11 years ago

  • Cc lukbarros added

hi.

you can check return

#3 follow-up: @lukbarros
11 years ago

  • Cc lukbarros removed
  • Severity changed from normal to trivial

hi.

you can check return...

You should check your implementation as quoted in attached diff. Close this ticket that it does not proceed, will break several wp deployed when someone atualiazr and have implemented waiting for the return as false on failure.

#4 in reply to: ↑ 3 @nacin
11 years ago

Replying to lukbarros:

You should check your implementation as quoted in attached diff. Close this ticket that it does not proceed, will break several wp deployed when someone atualiazr and have implemented waiting for the return as false on failure.

Hi lukbarros, I think you're saying that if we use user.diff, it will break plugins that expect a return as false on failure, rather than WP_Error. You have a point, for sure. But in this case, wp_insert_user() can already return WP_Error, and because of that, so can wp_update_user(). Both functions are documented to return a user ID on success and WP_Error on failure, and that's exactly what occurs now.

#5 @SergeyBiryukov
11 years ago

  • Severity changed from trivial to normal

@SergeyBiryukov
11 years ago

With proper formatting

#6 @SergeyBiryukov
11 years ago

  • Keywords has-patch added

#7 @lukbarros
11 years ago

Hello nacin there really is a point to be noted about the impact of change. Since users have other functions that return and the return of wp_erro should be adopted over time as the standard way to return errors and failures calls the core wp change is needed.

I still wonder what impact those implemented for plugins and routines that may await the return FALSE in case of no existence of the user with the id parameter for passaddo.

Who has the power to decide whether we change or not, giving acceptance to the possible?

#8 @nacin
11 years ago

  • Keywords commit needs-unit-tests added
  • Version set to 3.5

#9 @georgestephanis
11 years ago

+1

We're keeping with the existing documentation that it will return the ID or an error. I support this change, and don't anticipate it will negatively affect many plugins.

That's why plugin authors test using release candidates, riiiight?

#10 @nacin
11 years ago

In 1176/tests:

Test that wp_update_user() does not fatal error when called with a non-existent user ID. It should instead return WP_Error. see #22858.

#11 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 23210:

Return WP_Error from wp_update_user() on a non-existent user, avoiding a fatal error in the process.

props n7studios, SergeyBiryukov.
fixes #22858 for trunk.
Unit tests: [11776/tests].

#12 @nacin
11 years ago

In 23211:

Return WP_Error from wp_update_user() on a non-existent user, avoiding a fatal error in the process.

Merges [23210] to the 3.5 branch.

props n7studios, SergeyBiryukov.
fixes #22858.

#13 @nacin
11 years ago

  • Keywords needs-unit-tests removed

#14 @nacin
11 years ago

In 23225:

Switch to a string already available in the wordpress.pot, via the XML-RPC server class. The other string is only available in the wordpress-admin.pot.

Prevents any string movements in the 3.5 branch. see #22858.

#15 @pavelevap
11 years ago

But 3.5 branch in GlotPress was influenced? There are 2 similar strings now: "Invalid user ID" and "Invalid user ID."

#16 @SergeyBiryukov
9 years ago

#23005 was marked as a duplicate.

Note: See TracTickets for help on using tickets.