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 non-post indexing #112

Merged
merged 6 commits into from
Sep 2, 2021
Merged

Fix non-post indexing #112

merged 6 commits into from
Sep 2, 2021

Conversation

pschoffer
Copy link

@pschoffer pschoffer commented Sep 1, 2021

Description of the Change

When testing #111 I found that the offset problem is more widespread. This fixes the infinite loop for other indexable.

Introduced in #106

Also, the current version of the users query that was recently changed is not supported with mariadb mariadb:10.3.27-debian-10-r84 image we use in local dev-env.

cc @brettshumaker

Test steps

  1. Enable user feature vip dev-env exec -- wp elasticpress activate-feature users
  2. vip dev-env exec -- wp vip-search index --indexables=user succeeds

@rebeccahum
Copy link

@pschoffer Do you have any testing steps for reproducing?

@pschoffer
Copy link
Author

@rebeccahum I added some to the description. Thanks for the prompt. In general, whenever you try to index any non-post in bulk (default) fashion it does not work. That brings me to the idea that using --no-bulk is a probably working workaround for any tickets coming in.

But hopefully we can get this out today.

*/
$objects = $wpdb->get_results( $wpdb->prepare( "SELECT SQL_CALC_FOUND_ROWS ID FROM {$wpdb->users} %s LIMIT %d, %d", $orderby, (int) $args['offset'], (int) $args['number'] ) );
// @codingStandardsIgnoreStart
Copy link
Author

Choose a reason for hiding this comment

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

Could use input on this one. But with the sanitize_order_by above I think this is safe.

Choose a reason for hiding this comment

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

Seems pretty strict on what is allowed. The only alternative would be to provide an allowlist of valid $args['orderby'] columns and ensure it's one of them - but I don't feel like that is necessary here as it's no a top-level user API really.

@WPprodigy
Copy link

Can we drop the changes in includes/classes/Command.php here and just do the user orderby fix? Then should apply cleanly w/ #114

includes/classes/Indexable/User/User.php Outdated Show resolved Hide resolved
includes/classes/Indexable/User/User.php Outdated Show resolved Hide resolved
includes/classes/Indexable/User/User.php Outdated Show resolved Hide resolved
Co-authored-by: Caleb Burks <caleb.burks@automattic.com>
*/
$objects = $wpdb->get_results( $wpdb->prepare( "SELECT SQL_CALC_FOUND_ROWS ID FROM {$wpdb->users} %s LIMIT %d, %d", $orderby, (int) $args['offset'], (int) $args['number'] ) );
// @codingStandardsIgnoreStart

Choose a reason for hiding this comment

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

Seems pretty strict on what is allowed. The only alternative would be to provide an allowlist of valid $args['orderby'] columns and ensure it's one of them - but I don't feel like that is necessary here as it's no a top-level user API really.

@pschoffer pschoffer merged commit d9a8b20 into develop Sep 2, 2021
@pschoffer pschoffer deleted the fix/user_indexing branch September 2, 2021 08:31
@rebeccahum rebeccahum added the [Status] Upstream fix If this has been fixed upstream or in progress to be. label Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Upstream fix If this has been fixed upstream or in progress to be.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants