Make WordPress Core

Opened 2 years ago

Closed 38 hours ago

#24635 closed enhancement (fixed)

update_user_caches() should support accepting a WP_User instance

Reported by: dd32 Owned by:
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.0
Component: Users Keywords: has-patch needs-unit-tests
Focuses: Cc:


At present update_user_caches($user) blindly stores the value from $user into the object cache.
Although core only ever passes the raw data from wp_users into this function, due to the naming of it, and the phpdoc, plugin devs may pass a WP_User instance to the function.

The result is that a WP_User instance is stored within the object cache rather than stdClass of a wp_users row as expected.
For most intents this causes little issue, the function and cache still work, so the developer will see no side effects.

But this has the cause that when that user's data is retrieved from the cache, the original WP_User object is restored -- including all meta keys that were stored in it, If one of those meta keys is a serialized object which calls a not-yet-loaded function, it can cause the request to fatal error. It's a pretty specific issue, but can simply be avoided by never storing a WP_User instance in the cache.

Attached patch simply tests for the data and acts appropriately. mostly untested.

Attachments (1)

24635.diff (726 bytes) - added by dd32 2 years ago.

Download all attachments as: .zip

Change History (13)

@dd322 years ago

comment:1 @dd322 years ago

PHPDoc untouched, Worth noting that clean_user_cache() accepts int|WP_User, which is why both of those branches were added to this function.

comment:2 @nacin2 years ago

  • Keywords 3.7-early added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 3.0

comment:3 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:4 @ryan2 years ago

Is $user = $user->data necessary? It seems the first conditional would results in $user being WP_User and the second in stdClass.

Version 0, edited 2 years ago by ryan (next)

comment:5 follow-up: @ryan2 years ago

Is $user = $user->data necessary?

comment:6 in reply to: ↑ 5 @nacin2 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.7 to 3.8

Replying to ryan:

Is $user = $user->data necessary?

Yeah, that's the operative part of the patch. It's the underlying stdClass DB row that we want to store in the cache.

This needs tests, I'd say.

comment:7 @nacin22 months ago

  • Milestone changed from 3.8 to Future Release
  • Type changed from defect (bug) to enhancement

Still needs unit tests, and is an enhancement.

comment:8 @chriscct742 hours ago

  • Keywords 3.7-early removed

comment:9 @boonebgorges40 hours ago

  • Milestone changed from Future Release to 4.4

comment:10 @boonebgorges40 hours ago

In 34918:

Add tests for update_user_caches().

See #24635.

comment:11 @boonebgorges40 hours ago

In 34919:

Handle WP_User objects properly in update_user_caches().

We should not be storing the WP_User object in the cache, as it may contain
usermeta and other data that's cache elsewhere.

Props dd32.
See #24635.

comment:12 @boonebgorges38 hours ago

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

Other update_*_cache() plugins don't accept object IDs as parameters, so I don't think we need to here either.

Note: See TracTickets for help on using tickets.