WP_User_Query does not accept fields as string except ID

[rilwis]

The documentation says that:

'fields'
(string|array) Which fields to return. Single or all fields (string), or array of fields. Accepts 'ID', 'display_name', 'user_login', 'user_nicename', 'user_email', 'user_url', 'user_registered'. Use 'all' for all fields and 'all_with_meta' to include meta fields. Default 'all'.

​https://developer.wordpress.org/reference/classes/wp_user_query/prepare_query/

However, when passing any user field rather than 'ID', it always return IDs.

Looking inside the code, I found this:

if ( is_array( $qv['fields'] ) ) {
	$qv['fields'] = array_unique( $qv['fields'] );

	$this->query_fields = array();
	foreach ( $qv['fields'] as $field ) {
		$field                = 'ID' === $field ? 'ID' : sanitize_key( $field );
		$this->query_fields[] = "$wpdb->users.$field";
	}
	$this->query_fields = implode( ',', $this->query_fields );
} elseif ( 'all' === $qv['fields'] ) {
	$this->query_fields = "$wpdb->users.*";
} else {
	$this->query_fields = "$wpdb->users.ID";
}

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-user-query.php#L235

which explains the issue well.

Is that a bug or a missing feature?

[peterwilsoncc]

Thanks for the report.

I think it's a bug and either WP_User_Query::fill_query_vars() or the if statement you highlight ought to ensure the fields value is an array. The if statement would probably need some re-ordering if the value were forced to an array.

In pseudo code:

<?php
$qv['fields'] = (array) $qv['fields'];

if ( in_array( 'all', $qv['fields'], true ) {
  // all fields
} elseif ( ! empty( $qv['fields'] ) {
  // Custom list of fields
} else {
  // ID
}

For now I've set the version to the introduction of WP_User_Query::fill_query_vars() but the issue may have been introduced/incorrectly documented earlier.

[NomNom99]

Issue occurs only when fields is not an array.
The bug was on line:

<?php
$this->query_fields = "$wpdb->users.ID";

where "ID" is hardcoded.
Added a patch.

Trac ticket: https://core.trac.wordpress.org/ticket/53177

[JeffPaul]

Has patch, worth reviewing in 5.9 release cycle.

Added some tests and fix a small bug in the query

Trac ticket: https://core.trac.wordpress.org/ticket/53177

[audrasjb]

As per today's bug scrub, let's move this ticket to the next milestone for proper review.

[hellofromTonya]

For ​PR 1779, I changed its target from master to trunk. It needs a rebase against trunk before a code review and testing can happen. @pbearne can you refresh the PR please?

[hellofromTonya]

For testers to test and provide a Test Report, step-by-step testing instructions are needed to reproduce the issue and then validate if the PR resolves it. What can be helpful is to also provide code samples of how to test and/or any dependencies (such as a script or plugin).

@rilwis could you provide testing instructions?

[rilwis]

@hellofromTonya ,

Here is a simple code to test:

add_action( 'init', function() {
	$users = get_users( [
		'fields' => 'display_name',
	] );
	print_r( $users ); // Expect an array of display names, but got an array of IDs.
} );

[hellofromTonya]

Thank you @pbearne for rebasing and refreshing the PR.

Thank you @rilwis for providing testing information.

Will ping in the #core-test Slack channel to alert testers this is ready for testing and a Test Report.

[Boniu91]

Test Report

Env

• WordPress 5.9.2 and 6.0-alpha-52448-src
• Windows 10
• Theme: Twenty Twenty One

Steps to test

1. On WordPress 5.9.2 add the code mentioned https://core.trac.wordpress.org/ticket/53177#comment:14
2. User ID is displayed on the frontend
3. Update the WordPress to this specific patch
4. User display_name as a string is displayed on the frontend

[audrasjb]

Tested with a fresh 6.0 alpha installation, 4 users created and the following code:

add_action( 'init', function() {
	$users = get_users( [
		'fields' => 'display_name',
	] );
	wp_die( print_r( $users ) );
} );

Before patch, it returns:

Array ( 
  [0] => 3 
  [1] => 1 
  [2] => 2 
  [3] => 4
)

After patch:

Array ( 
  [0] => Jack 
  [1] => jb 
  [2] => momo 
  [3] => Roberta
)

Marking as good for commit.

[audrasjb]

In 53255:

Users: Make sure WP_User_Query can be filtered using the fields parameter.

This change ensures the fields parameter is taken into account when running WP_User_Query by fixing the conditional statement used to process the fields param.

Props rilwis, peterwilsoncc, NomNom99, hellofromTonya, audrasjb, rilwis, Boniu91.
Fixes #53177.

Committed in https://core.trac.wordpress.org/changeset/53255

[audrasjb]

I forgot a props for @pbearne. I updated the props manually using the Core Props manual tool located on Make/Core. Sorry for the mistake :)

[audrasjb]

omission corrected :)

[pbearne]

dev note ​https://docs.google.com/document/d/1IzIHWsfHfjCx3nmldzJ8EC9kaSVNzskvX6U8vb7CPV0/edit#heading=h.wqjp0pdjlall

[peterwilsoncc]

Reopening this for a few reasons:

• data_returning_fields() needs a test for a lower case ID
• An array of fields doesn't validate against the allowed fields
• The query generated when passing an invalid value has changed from ID to *

This PR addresses the adjustments outlined by ​@peterwilsoncc

Trac ticket: https://core.trac.wordpress.org/ticket/53177

[peterwilsoncc]

The ​WP_User_Query return fields documentation will need an update as this changes the results a little.

An array of IDs, stdClass objects, or WP_User objects, depending on the value of the ‘fields‘ parameter.

• If ‘fields‘ is set to ‘all’ (default), or ‘all_with_meta’, it will return an array of WP_User objects (does not include related user meta fields even with ‘all_with_meta’ set) .
• If ‘fields‘ is set to an array of wp_users table fields, it will return an array of stdClass objects with only those fields.
• If ‘fields‘ is set to any individual wp_users table field, an array of IDs will be returned.

The last case has changed so that if a single field is set then an array of the single filed will be returned:

// specifying user name, new behavior
array(1) {
    [0] =>
    string(5) "admin"
  }

// specifying user name, old behavior
  array(1) {
    [0] =>
    string(1) "1"
  }

Good catch @peterwilsoncc, didn't notice the include[space] there. I've addressed all those things in my last commit. Do you mind giving it another look? Thanks!

[peterwilsoncc]

In 53327:

Users: Validate WP_User_Query's fields argument.

Improve validation of WP_User_Query's fields argument when passed as an array to ensure it only accepts permitted values. This prevents the invalid values being included in the generated database query.

Expand unit tests to include passing invalid values as part of an array, the lower case value id. Correct earlier unit tests to limit database query to one result.

Follow up to [53255].

Props felipeelia.
Fixes #53177.

Merged in https://core.trac.wordpress.org/changeset/53327, thank you!

[dd32]

This behaviour was being relied upon by BuddyPress, and seems that the fields that this is now limited to, might not cover all cases used by other plugins - particularly authentication or user-sync plugins.

I'm re-opening this with that in mind,

​https://buddypress.trac.wordpress.org/ticket/8694

I'll also note, that passing 'fields' => 'all' to WP_User_Query results in it returning more fields than what passing all the 'valid' fields will result in.

eg. passing 'all' allows for returning the user_pass, user_activation_key , user_status, spam, and deleted fields which are not present in the allowed fields list in r53255.

edit: My assumption is that the whitelisted fields could probably just be expanded to include all of the possible wp_user.* fields, with or without being documented as a valid value. I think that would resolve any potential back-compat issues this introduced.

[pbearne]

working on this

[pbearne]

I have updated my patch for this @peterwilsoncc can you look at this

[kraftbj]

Noting a change that happens with this, I believe.

In WordPress 5.9, $query = new \WP_User_Query( array( 'fields' => array( 'id' ), 'role' => 'administrator', ) );

would return a results value that returned id lowercase:

  ["results":"WP_User_Query":private]=>
  array(1) {
    [0]=>
    object(stdClass)#2379 (1) {
      ["id"]=>
      string(1) "1"
    }
  }

In trunk, it returns the same thing with capitalized ID in the response:

  ["results":"WP_User_Query":private]=>
  array(1) {
    [0]=>
    object(stdClass)#2470 (1) {
      ["ID"]=>
      string(1) "1"
    }
  }

It may have been a fluke that it returned the matching capitalization and on our plugin, we've corrected it, but wanted to note it.

[peterwilsoncc]

I did some testing with 5.9 vs 6.0 and tested the results, it seems it's itended that the property be lower-case according the majority of cases.

Fields argument	5.9 property name	6.0 property name
['id']	$id	$ID
['Id']	$id	$ID
['ID']	$ID	$ID
['iD']	$id	$ID

As I see it there are a few options, listed in my order of preference:

1. maintain the inconsistency
2. return in both upper and lower case always
3. return in lower case always (a back compat break)

As requesting a subset of fields return an array of stdClass, there isn't the option to use a custom getter to make the property case insensitive.

[kraftbj]

I believe the documentation has stated ID as the option in constructing or in the return, even if that's not what it did.

I would lean towards 2 as the ID is the expected casing but maintain lowercase for BC for now. Deprecate the lowercase usage in some form.

https://core.trac.wordpress.org/ticket/53177

[peterwilsoncc]

In the ​linked pull request:

• both ID and id are returned when fields is an array containing ID
• The fields parameter is case insensitive. This matches the old behavior as the prior values were passed to the database which is case insensitive.
• split up tests to separate an array of fields from a single field to reduce logic within the test
• added all fields in the table to allowed fields (props @pbearne, I cherry picked from Paul's PR)

Obviously my code is perfect and flawless in every way but @dd32 could you please check it fixes your bug, @kraftbj could you please check it doesn't reintroduce what you've already fixed?

[dd32]

Replying to peterwilsoncc:

dd32 could you please check it fixes your bug

Yes, this appears to fix the BuddyPress bug ​https://buddypress.trac.wordpress.org/ticket/8694

I have not tested it other than that, but the code appears good and the unit test coverage wide.

both ID and id are returned when fields is an array containing ID

I agree that this seemed to be the best BC way here - it seems unlikely (and wrong) for anyone to have been expecting a lower-case id but given how easy it is to support, and the case insensitivity of the database, this seemed like an easy win to avoid potential breakage.

Thanks for the review.

Let's not introduce a doing it wrong.

As table names are case insensitive, I've been pondering overnight whether WPDB::get_results() should return a custom object that uses a magic method to make the properties case insensitive. A topic for another day on another ticket.

[peterwilsoncc]

In 53362:

Users: Allow any DB field to be returned by WP_User_Query.

Restore behaviour of fields parameter in WP_User_Query to allow developers to specify any database field to be returned either individually or as part of a subset. Add these fields to the documentation.

When a subset of fields includes the id paramater, include it in the results in both upper and lowercase to maintain backward compatibility.

Follow up to [53327].

Props dd32, pbearne, kraftbj, peterwilsoncc.
Fixes #53177.

Merged in https://core.trac.wordpress.org/changeset/53362

[peterwilsoncc]

Reopening for merging [53362] to the 6.0 branch pending sign off from another committer.

[SergeyBiryukov]

I would prefer if the expected results in the tests matched the actual results a bit more closely, specifically in returning the uppercase ID before id. It looks like that is always the case here. See 53177.2.diff​.

Otherwise, [53362] looks good to backport.

[peterwilsoncc]

Replying to SergeyBiryukov:

I would prefer if the expected results in the tests matched the actual results a bit more closely, specifically in returning the uppercase ID before id.

I don't think it matters. As WPDB returns the results as a stdClass() the tests already need to cast them to an array to be able to do the tests. assertSameSetsWithIndex() exists precicly because order doesn't matter. Whe it does, assertSame is used.

[peterwilsoncc]

In 53373:

Users: Improve WP_User_Query tests following [53362].

Props SergeyBiryukov.
See #53177.

[peterwilsoncc]

In 53374:

Users: Allow any DB field to be returned by WP_User_Query.

Restore behaviour of fields parameter in WP_User_Query to allow developers to specify any database field to be returned either individually or as part of a subset. Add these fields to the documentation.

When a subset of fields includes the id paramater, include it in the results in both upper and lowercase to maintain backward compatibility.

Follow up to [53327].

Props dd32, pbearne, kraftbj, peterwilsoncc, SergeyBiryukov.
Merges [53362,53373] to the 6.0 branch.
Fixes #53177.

[SergeyBiryukov]

Replying to peterwilsoncc:

I don't think it matters. As WPDB returns the results as a stdClass() the tests already need to cast them to an array to be able to do the tests. assertSameSetsWithIndex() exists precicly because order doesn't matter.

Yeah, it doesn't matter for the tests to pass, but having some consistency here makes it easier for me to build a mental map of how it works and what is expected. Thanks for the commit!

[spacedmonkey]

Related: #55594