WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#22858 closed defect (bug) (fixed)

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

Reported by: ryan Owned by: 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 16 months ago.
22858.patch (414 bytes) - added by SergeyBiryukov 16 months ago.
With proper formatting

Download all attachments as: .zip

Change History (17)

comment:1 nacin16 months ago

  • Milestone changed from Future Release to 3.5.1

n7studios16 months ago

comment:2 lukbarros16 months ago

  • Cc lukbarros added

hi.

you can check return

comment:3 follow-up: lukbarros16 months 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.

comment:4 in reply to: ↑ 3 nacin16 months 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.

comment:5 SergeyBiryukov16 months ago

  • Severity changed from trivial to normal

SergeyBiryukov16 months ago

With proper formatting

comment:6 SergeyBiryukov16 months ago

  • Keywords has-patch added

comment:7 lukbarros16 months 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?

comment:8 nacin16 months ago

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

comment:9 georgestephanis16 months 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?

comment:10 nacin16 months 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.

comment:11 nacin16 months 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].

comment:12 nacin16 months 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.

comment:13 nacin16 months ago

  • Keywords needs-unit-tests removed

comment:14 nacin16 months 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.

comment:15 pavelevap16 months ago

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

Note: See TracTickets for help on using tickets.