Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#53177 closed defect (bug) (fixed)

WP_User_Query does not accept fields as string except ID

Reported by: rilwis's profile rilwis Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: has-testing-info has-dev-note has-patch has-unit-tests needs-docs dev-reviewed fixed-major
Focuses: docs Cc:

Description

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?

Attachments (3)

53177.diff (615 bytes) - added by NomNom99 3 years ago.
Capture d’écran 2022-04-25 à 14.58.16.png (269.2 KB) - added by audrasjb 2 years ago.
omission corrected :)
53177.2.diff (1.2 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (56)

#1 @peterwilsoncc
3 years ago

  • Version changed from trunk to 4.4

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
3 years ago

#2 @NomNom99
3 years ago

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.

This ticket was mentioned in PR #1251 on WordPress/wordpress-develop by Sidsector9.


3 years ago
#3

  • Keywords has-patch added

#4 @desrosj
3 years ago

  • Summary changed from WP_User_Query doesn't not accept fields as string except ID to WP_User_Query does not accept fields as string except ID

#5 @JeffPaul
3 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 5.9

Has patch, worth reviewing in 5.9 release cycle.

This ticket was mentioned in Slack in #core-test by pbearne. View the logs.


3 years ago

This ticket was mentioned in PR #1779 on WordPress/wordpress-develop by pbearne.


3 years ago
#7

  • Keywords has-unit-tests added

Added some tests and fix a small bug in the query

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#9 @audrasjb
2 years ago

  • Milestone changed from 5.9 to 6.0

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

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


2 years ago

#12 @hellofromTonya
2 years ago

  • Keywords needs-refresh added

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?

#13 @hellofromTonya
2 years ago

  • Keywords needs-testing-info added

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?

#14 @rilwis
2 years ago

@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.
} );

#15 @hellofromTonya
2 years ago

  • Keywords has-testing-info added; needs-refresh needs-testing-info removed

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.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

#18 @Boniu91
2 years ago

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

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

#20 @audrasjb
2 years ago

  • Keywords commit assigned-for-commit added; needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to accepted

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.

#21 @audrasjb
2 years ago

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

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.

#23 @audrasjb
2 years ago

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
2 years ago

omission corrected :)

#25 @peterwilsoncc
2 years ago

  • Keywords has-patch has-unit-tests commit assigned-for-commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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 ticket was mentioned in PR #2656 on WordPress/wordpress-develop by felipeelia.


2 years ago
#26

  • Keywords has-patch has-unit-tests added

This PR addresses the adjustments outlined by @peterwilsoncc

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

#27 @peterwilsoncc
2 years ago

  • Keywords needs-docs added

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"
  }

felipeelia commented on PR #2656:


2 years ago
#28

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!

#29 @peterwilsoncc
2 years ago

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

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.

#31 @dd32
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Last edited 2 years ago by dd32 (previous) (diff)

#32 @pbearne
2 years ago

working on this

#33 @pbearne
2 years ago

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

Last edited 2 years ago by pbearne (previous) (diff)

#34 @kraftbj
2 years ago

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.

#35 @peterwilsoncc
2 years ago

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.

Last edited 2 years ago by peterwilsoncc (previous) (diff)

#36 @kraftbj
2 years ago

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.

This ticket was mentioned in Slack in #core by costdev. View the logs.


2 years ago

#39 follow-up: @peterwilsoncc
2 years ago

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?

#40 in reply to: ↑ 39 @dd32
2 years ago

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.

peterwilsoncc commented on PR #2676:


2 years ago
#41

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.

#42 @peterwilsoncc
2 years ago

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

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.

#44 @peterwilsoncc
2 years ago

  • Keywords dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

This ticket was mentioned in Slack in #core-committers by peterwilsoncc. View the logs.


2 years ago

#46 follow-up: @SergeyBiryukov
2 years ago

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.

#47 @SergeyBiryukov
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#48 in reply to: ↑ 46 ; follow-up: @peterwilsoncc
2 years ago

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.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

#50 @peterwilsoncc
2 years ago

In 53373:

Users: Improve WP_User_Query tests following [53362].

Props SergeyBiryukov.
See #53177.

#51 @peterwilsoncc
2 years ago

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

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.

#52 in reply to: ↑ 48 @SergeyBiryukov
2 years ago

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!

Note: See TracTickets for help on using tickets.