Make WordPress Core

Opened 2 years ago

Last modified 13 days ago

#62003 new defect (bug)

User_Query cache triggers fatal error by ignoring 'fields' query var

Reported by: sermitr Owned by:
Priority: normal Milestone: 7.2
Component: Users Version: 6.3
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses: performance

Description

Method WP_User_Query::query() generates different results depending on whether $this->query_vars['fields'] an array (array of objects) or a string (array of scalar values).

However, if it contains only one field, the generated SQL query will be the same.

Function generate_cache_key() doesn't take into account the fields query var, and relies on the generated SQL query instead. This leads to the unpredictability of the returned data format (array of objects or array of scalar values).

How to reproduce:

  1. Clean the cache: wp cache flush.
  2. Fetch the users passing fields as an array:
    <?php
    $wp_user_search = new WP_User_Query( array( 'fields' => array( 'ID' ), 'count_total' => false ) );
    $users = $wp_user_search->get_results();
    var_dump( $users );
    // Result is an array of objects, which is now cached: array(1) { [0]=> object(stdClass)#1268 (2) { ["ID"]=> string(1) "1" ["id"]=> string(1) "1" } } 
    
  3. Run the same request with the fields being a string: 'fields' => 'ID'. The result will be an array of objects pulled from cache, although array of strings is expected.
  4. Fetch the users by CLI command: wp user list. The result is expected to be an array of strings, so the fatal error is triggered:
    Fatal error: Uncaught TypeError: Illegal offset type in wp-includes/class-wp-user-query.php:892
    
  5. Clean the cache: wp cache flush
  6. Run the request from step 3, where fields is a string:
    <?php
    $wp_user_search = new WP_User_Query(array('fields' => 'ID', 'count_total' => false));
    $users = $wp_user_search->get_results();
    var_dump( $users);
    // Result is an array of strings, which is now cached: array(1) { [0]=> string(1) "1" } 
    
  7. Run the request from step 2, where fields is an array. The result be the same array of strings pulled from cache, which is different from what we had on step 2.
  8. Run the CLI command wp user list. There will be no fatal error, because the data pulled from cache is an array of strings as expected.

Change History (8)

#1 @sermitr
2 years ago

The workaround here is cleaning the cache and disabling it for requests where fields is an array of one element:

<?php
add_filter( 'users_pre_query', function( $null, $query ) {
    if ( ! empty( $query->query_vars['fields'] ) && is_array( $query->query_vars['fields'] ) && count( $query->query_vars['fields'] ) === 1 ) {
        $query->query_vars['cache_results'] = false;
    }

    return $null;
}, 10, 2 );

#2 @hellofromTonya
2 years ago

  • Focuses performance added
  • Version 6.6.1 → 6.3

Hello @sermitr,

Welcome to WordPress Core's Trac. Thank you for all of the details, including steps to reproduce, in this ticket's report.

I'm doing triage for 6.6.x cycle. The Version that introduced this code was 6.3, via #40613 and changeset [55657]. Updating the Version to reflect when the code was introduced. Also noting, no changes were made in this method or its usage in 6.6.1.

The original enhancement that added this code was marked for performance. Adding that focus here.

To help contributors, sharing the ​dev note link here for contextual awareness.

#3 @sirakashaman
3 weeks ago

  • Keywords needs-patch added

Hi, I was able to reproduce this on the latest release and on trunk, following the steps in the ticket description.

Environment

  • WordPress: 7.1.2 and 7.2-alpha-63166-src (trunk @ 87e64abf54)
  • PHP: 8.5.3
  • Server: nginx/1.26.1
  • Database: MySQL 8.4.0
  • Object cache: Redis 8.6.2, PhpRedis 6.3.0, Redis Object Cache drop-in 3.0.0
  • WP-CLI: 2.13.0
  • Users: 15 (all roles)

Steps and results

Each step is a separate WP-CLI process, so the only thing shared between them is the persistent object cache, the same as two page loads. Steps 2, 3, 6 and 7 use 'count_total' => false, as in the ticket description.

Step Code Expected Actual (7.1.2 and trunk)
1 wp cache flush
2 'fields' => array( 'ID' ) objects objects ✅
3 'fields' => 'ID' strings objects ❌
4 wp user list user list Fatal error: Uncaught TypeError: Cannot access offset of type stdClass on array in wp-includes/class-wp-user-query.php:894 ❌
5 wp cache flush
6 'fields' => 'ID' strings strings ✅
7 'fields' => array( 'ID' ) objects strings ❌
8 wp user list user list user list ✅

✅ Reproduced as described. On current versions the fatal error is reported on line 894 rather than 892.

Additional findings

While testing, I came across a few related cases that may be helpful when working on a fix:

  1. Two regular get_users() calls are enough to trigger it. get_users() always sets 'count_total' => false, which is why the steps in the ticket include it. count_total changes the SQL, so only queries with the same count_total value share a cache entry.
    <?php
    get_users( array( 'fields' => array( 'ID' ) ) ); // Caches objects.
    
    $users = get_users(); // Receives the cached objects.
    // WP_User objects with an empty user_login, user_email, display_name, etc.
    
    PHP then emits the following, and wp user list triggers the same fatal error:
    Warning: Object of class stdClass could not be converted to int in wp-includes/meta.php on line 1195 (line 1156 on 7.1.2)
    Notice: Function _get_non_cached_ids was called incorrectly. Object ID must be an integer, object given.
    
  2. The fatal error comes from 'all_with_meta'. wp user list uses it, and line 894 uses the cached stdClass objects as array keys:
    <?php
    $r[ $userid ] = new WP_User( $userid, '', $qv['blog_id'] );
    
  3. Any single-field array is affected, not only ID. For example, array( 'user_email' ) and 'user_email' also share a cache entry. It also happens with extra query arguments such as 'role' => 'author', because remove_placeholder_escape() makes those cache keys identical as well.
  4. The incorrect entry persists. With a persistent object cache it is served to later requests until the users last-changed value is updated (by any user or user meta change, including a login) or the cache is flushed, and it is stored again whenever the triggering query runs first. Without a persistent cache it only affects a single page load, although two plugins using both formats on the same page would still conflict.
  5. On multisite, network-wide queries are shared across sites. user-queries is a global cache group, so queries with 'blog_id' => 0 share entries across the network. On 7.1.2 with Redis, running this on a sub-site:
    <?php
    new WP_User_Query(
            array(
                    'number'  => 20,
                    'offset'  => 0,
                    'search'  => '',
                    'blog_id' => 0,
                    'fields'  => array( 'ID' ),
            )
    );
    
    causes a fatal error on Network Admin → Users (WP_MS_Users_List_Table::prepare_items(), line 894), and wp user list --network shows the same fatal error. Per-site queries are not shared, because each site's role meta key (wp_capabilities / wp_2_capabilities) makes the SQL different.
  6. Only array vs. non-array fields values conflict. Arrays with 2–3 fields produce different SQL, and arrays with more than 3 fields are not cached (line 800). 'ID' and 'all' share an entry, but they cache the same format (a list of IDs), so that combination works correctly.

Root cause

WP_User_Query::query() fetches the same SQL in two different formats (lines 844 and 846):

<?php
if ( is_array( $qv['fields'] ) ) {
        $this->results = $wpdb->get_results( $this->request ); // array of objects
} else {
        $this->results = $wpdb->get_col( $this->request );     // array of values
}

generate_cache_key() (line 1052) builds the key from the SQL only (line 1058), so both formats are stored under the same key:

<?php
$key = md5( $sql );
return "get_users:$key";

For example, with the default count_total, array( 'ID' ), 'ID' and 'all' all produce the same SQL and key:

SELECT SQL_CALC_FOUND_ROWS wp_users.ID FROM wp_users WHERE 1=1 ORDER BY user_login ASC
→ get_users:5f863aef1ab4d4484e63b8f69c410bda

WP_Query also leaves fields out of its cache key, but it always caches the same format (post IDs) and converts the results afterwards, so it doesn't run into this. WP_User_Query caches the results in whichever format it has just fetched.

This also seems to differ from the behaviour described in the ​6.3 dev note: "subsequent queries with the same parameters will retrieve the data from the cache". Here, queries with different fields values receive the same cache entry.

Next steps

I'm working on a fix, along with unit tests covering the cases above, and will open a PR soon. Any feedback on these findings would be very welcome.

This ticket was mentioned in ​PR #13727 on ​WordPress/wordpress-develop by ​@irozum.


3 weeks ago
#4

  • Keywords has-patch has-unit-tests added; needs-patch removed

WP_User_Query::generate_cache_key() builds the cache key purely from the generated SQL string, but a single-element 'fields' array (e.g. array( 'ID' )) produces exactly the same SQL as the equivalent scalar string ('ID'), even though the two are expected to return differently shaped results — an array of objects for the array form, an array of scalars for the string form. Because the cache key collides, whichever request runs first "wins": the second request silently gets back the wrong shape from cache instead of running its own query, which can surface as a fatal TypeError: Illegal offset type when code that expects scalar user IDs receives cached stdClass objects instead (or vice versa).

This adds the 'fields' shape (array vs. scalar) into the cache key so the two request shapes no longer collide, and adds a regression test in tests/phpunit/tests/user/queryCache.php that reproduces the collision (queries with fields => array( 'ID' ) then fields => 'ID' for the same user, asserting the second query returns scalars, not cached objects).

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Sonnet 5
Used for: Implementation, tests, and PR description. Reviewed by Igor Rozum.

#5 @SergeyBiryukov
3 weeks ago

  • Milestone Awaiting Review → 7.2

This ticket was mentioned in ​PR #13754 on ​WordPress/wordpress-develop by ​@sirakashaman.


3 weeks ago
#6

WP_User_Query fetches results in two different formats depending on fields: an array of fields returns rows as objects (get_results()), anything else returns a flat list of values (get_col()). A single-field array such as array( 'ID' ) generates exactly the same SQL as 'ID' or 'all', and generate_cache_key() builds the key from the SQL only. So whichever query runs first decides the format that every later query receives from the cache.

With a persistent object cache, this causes:

  • objects returned where strings are expected, and vice versa;
  • WP_User objects with an empty user_login, user_email, etc. from get_users() after get_users( array( 'fields' => array( 'ID' ) ) );
  • a fatal error for 'all_with_meta' (Cannot access offset of type stdClass on array in class-wp-user-query.php), e.g. in wp user list, Users → All Users, and on multisite Network Admin → Users.

Full reproduction details are in comment:3 on the ticket.

Changes

src/wp-includes/class-wp-user-query.php: generate_cache_key() now includes the result format in the key:

$format = is_array( $this->query_vars['fields'] ) ? 'objects' : 'values';

$key = md5( $sql );

return "get_users:$format:$key";
  • Readable keys: array queries use get_users:objects:<hash>, and all other queries use get_users:values:<hash>.
  • Same hit rate: 'ID', 'all' and 'all_with_meta' still share an entry, because they cache the same list of IDs.
  • Upgrade-safe: every key changes, so entries cached incorrectly before this change are not read afterwards.

tests/phpunit/tests/user/queryCache.php: adds test_query_cache_should_respect_fields_return_format() with a data provider of 6 cases:

Case First query (primes the cache) Second query (read from the cache)
string then array 'ID' array( 'ID' )
array then string array( 'ID' ) 'ID'
array then all array( 'ID' ) 'all'
all then array 'all' array( 'ID' )
array then all_with_meta array( 'ID' ) 'all_with_meta'
array then string field array( 'user_email' ) 'user_email'

Each case compares the cached result with the same query run with 'cache_results' => false. It checks the format of every item with assertSame(), then the values.

Testing

Tested on trunk with Redis as the persistent object cache:

  • New test: 6/6 pass with the fix. Without it: 4 failures ("Cached results should have the same format as uncached results.") and 2 errors from the 'all' / 'all_with_meta' cases.
  • phpunit --filter Tests_User_Query_Cache: 42 tests, all pass.
  • phpunit --group user: 1350 tests, no failures.
  • PHPCS: no errors.
  • Manual: the steps in the ticket description, wp user list, the Users list table, and Network Admin → Users on a multisite network all work. Entries cached incorrectly before the change are not returned after it.

#13727 by @irozum takes the same approach, with a hash-based key and a single-case test. This PR differs in its readable objects / values key and its broader test coverage. Happy to consolidate into one PR, or to have the data provider added to #13727, whichever reviewers prefer.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Investigating the root cause, reproduction scripts, implementation, tests and this description. I reproduced the issue myself on local sites, and I reviewed and revised the implementation and tests.

---
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See ​GitHub Pull Requests for Code Review in the Core Handbook for more details.

🤖 Generated with ​Claude Code

#7 @sirakashaman
2 weeks ago

I had already been working on a fix since my reproduction in comment:3, so I opened ​PR #13754 to share it alongside ​PR #13727 rather than as a competing fix. Both change the cache key the same way, but ​#13754 aims for readability: the key includes the format (get_users:objects:<hash> / get_users:values:<hash>), which is easier to spot when inspecting a persistent cache, and the test uses a data provider with clear assertion messages. It also adds wider coverage, with 6 cases including the reverse order and the 'all' / 'all_with_meta' path behind the wp user list fatal. Happy to go with whichever PR reviewers prefer, or combine the tests into one.

Last edited 2 weeks ago by sirakashaman (previous) (diff)

#8 @muneebashraf
13 days ago

Patch Testing Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/13754 (head 98a51bc)

Environment

  • WordPress: trunk dc829be (reproduction) and the PR head (patch)
  • PHP: 8.4.26 (CLI, Windows)
  • Database: SQLite (SQLite Database Integration 3.0.2)
  • Test method: PHPUnit test. The object cache persists within the test, which stands in for a persistent object cache.

Steps

  1. get_users( array( 'fields' => array( 'ID' ) ) ), then get_users().
  2. Flush the cache. get_users( array( 'fields' => array( 'ID' ) ) ), then get_users( array( 'fields' => 'ID' ) ).
  3. Flush the cache. get_users( array( 'fields' => array( 'ID' ) ) ), then get_users( array( 'fields' => 'all_with_meta' ) ).

Actual results

Step Trunk With patch
1. get_users() Warning: Object of class stdClass could not be converted to int WP_User objects with user_login populated
2. fields => 'ID' returns objects instead of ID strings returns strings
3. all_with_meta Warning: Object of class stdClass could not be converted to int returns the users, no error
  • ✅ Bug reproduced on trunk: whichever format runs first is served from the cache to the other.
  • ✅ Patch fixes all three cases.
  • ✅ Tests_User_Query_Cache (tests/phpunit/tests/user/queryCache.php) on the PR head: OK (42 tests, 175 assertions).

Additional notes

​https://github.com/WordPress/wordpress-develop/pull/13727 (head cdbde46) fixes the same three cases in my test, with a smaller change. Its Tests_User_Query_Cache passes too (37 tests, 165 assertions).

AI assistance: Claude Code (Claude Opus 5.5) helped write the test. The results above come from running it.

Note: See TracTickets for help on using tickets.