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:
- Clean the cache:
wp cache flush. - Fetch the users passing
fieldsas 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" } }
- Run the same request with the
fieldsbeing a string:'fields' => 'ID'. The result will be an array of objects pulled from cache, although array of strings is expected. - 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
- Clean the cache:
wp cache flush - Run the request from step 3, where
fieldsis 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" }
- Run the request from step 2, where
fieldsis an array. The result be the same array of strings pulled from cache, which is different from what we had on step 2. - 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)
#2
@
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
@
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:
- 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_totalchanges the SQL, so only queries with the samecount_totalvalue share a cache entry.PHP then emits the following, and<?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.
wp user listtriggers 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.
- The fatal error comes from
'all_with_meta'.wp user listuses it, and line 894 uses the cachedstdClassobjects as array keys:<?php $r[ $userid ] = new WP_User( $userid, '', $qv['blog_id'] );
- 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', becauseremove_placeholder_escape()makes those cache keys identical as well. - The incorrect entry persists. With a persistent object cache it is served to later requests until the
userslast-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. - On multisite, network-wide queries are shared across sites.
user-queriesis a global cache group, so queries with'blog_id' => 0share entries across the network. On 7.1.2 with Redis, running this on a sub-site:causes a fatal error on Network Admin → Users (<?php new WP_User_Query( array( 'number' => 20, 'offset' => 0, 'search' => '', 'blog_id' => 0, 'fields' => array( 'ID' ), ) );
WP_MS_Users_List_Table::prepare_items(), line 894), andwp user list --networkshows 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. - Only array vs. non-array
fieldsvalues 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.
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_Userobjects with an emptyuser_login,user_email, etc. fromget_users()afterget_users( array( 'fields' => array( 'ID' ) ) );- a fatal error for
'all_with_meta'(Cannot access offset of type stdClass on arrayinclass-wp-user-query.php), e.g. inwp 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 useget_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.
Related
#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
@
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.
#8
@
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
get_users( array( 'fields' => array( 'ID' ) ) ), thenget_users().- Flush the cache.
get_users( array( 'fields' => array( 'ID' ) ) ), thenget_users( array( 'fields' => 'ID' ) ). - Flush the cache.
get_users( array( 'fields' => array( 'ID' ) ) ), thenget_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.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
The workaround here is cleaning the cache and disabling it for requests where
fieldsis an array of one element: