Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WP_Query_User when it tries to sort the results #2226

Merged
merged 11 commits into from
Jun 24, 2021

Conversation

Rahmon
Copy link
Contributor

@Rahmon Rahmon commented Jun 1, 2021

Description of the Change

This changes updates to use keyword fields to sort in the WP_User_Query and adds tests to ensure we are using these fields.

Alternate Designs

Benefits

Elasticsearch will not fail and fall back to MySQL.

Possible Drawbacks

Verification Process

  1. Enable Users feature
  2. Go to a page/post and add a paragraph block
  3. Type @ and try to search a user e.g. @admin
  4. Check if the results were provided by Elasticsearch.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Closes: #2225

Changelog Entry

Fixed sort for WP_User_Query

@Rahmon Rahmon added this to the 3.6.0 milestone Jun 1, 2021
@Rahmon Rahmon requested a review from felipeelia June 1, 2021 21:09
Comment on lines 625 to 632
/**
* Check if Zoey is the first user because Elasticsearch sort
* is case-sensitive
*
* Ref: https://www.elastic.co/guide/en/elasticsearch/guide/2.x/sorting-collations.html#case-insensitive-sorting
*
*/
$this->assertEquals( 'Zoey', $users_display_name_fetched[0] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rahmon it seems to be counterintuitive to rely on a case-sensitive sorting while WP's is case insensitive. Also, for fields in the posts indexable we have a "sortable" field that we could replicate here. Do you mind taking care of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Thanks for that @felipeelia

@felipeelia felipeelia assigned Rahmon and unassigned felipeelia Jun 10, 2021
@Rahmon Rahmon assigned felipeelia and unassigned Rahmon Jun 11, 2021
felipeelia
felipeelia previously approved these changes Jun 16, 2021
@felipeelia
Copy link
Member

@Rahmon did you test it with Elasticsearch 5.6.16 (the version shipped with WP Local Docker)? I'm using the following snippet in theme's header.php:

new WP_User_Query(
	[
		'search'  => 'test',
		'orderby' => 'display_name',
	]
);

As is, I receive a 400 error saying:

Fielddata is disabled on text fields by default. Set fielddata=true on [display_name.sortable] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead.

If I change wp-content/plugins/elasticpress/includes/mappings/user/initial.php to look like this, it works:

'display_name'    => array(
	'type'   => 'text',
	'fields' => array(
		'raw'      => array(
			'type'                => 'keyword',
			'ignore_above' => 10922,
		),
		'sortable' => array(
			'type'         => 'keyword',
			'ignore_above' => 10922,
			'normalizer'   => 'lowerasciinormalizer',
		),
	),
),

Can you please check it? Thanks

@felipeelia felipeelia self-requested a review June 16, 2021 16:12
@felipeelia felipeelia assigned Rahmon and unassigned felipeelia Jun 16, 2021
@Rahmon
Copy link
Contributor Author

Rahmon commented Jun 21, 2021

@Rahmon did you test it with Elasticsearch 5.6.16 (the version shipped with WP Local Docker)? I'm using the following snippet in theme's header.php:

new WP_User_Query(
	[
		'search'  => 'test',
		'orderby' => 'display_name',
	]
);

As is, I receive a 400 error saying:

Fielddata is disabled on text fields by default. Set fielddata=true on [display_name.sortable] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead.

If I change wp-content/plugins/elasticpress/includes/mappings/user/initial.php to look like this, it works:

'display_name'    => array(
	'type'   => 'text',
	'fields' => array(
		'raw'      => array(
			'type'                => 'keyword',
			'ignore_above' => 10922,
		),
		'sortable' => array(
			'type'         => 'keyword',
			'ignore_above' => 10922,
			'normalizer'   => 'lowerasciinormalizer',
		),
	),
),

Can you please check it? Thanks

@felipeelia I've updated the mapping and updated some tests to check if the query uses the Elasticsearch to retrieve the users.

@Rahmon Rahmon assigned felipeelia and unassigned Rahmon Jun 21, 2021
@felipeelia felipeelia merged commit 75825c2 into develop Jun 24, 2021
@felipeelia felipeelia deleted the fix/sort-user-search-issue-2225 branch June 24, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: the search fails when trying to find a user in a block
3 participants