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

Fixes typo in the ORDER BY clause in a user query. #97

Merged
merged 6 commits into from
Aug 4, 2021

Conversation

brettshumaker
Copy link

Description

One of the users queries contained a typo. The $args['orderby'] was used twice instead of using $args['order'] for the second arg. This resulted in the ORDER BY clause being ORDER BY 'ID' 'ID'.

Checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally (or has an appropriate fallback).
  • This change works and has been tested on a Go sandbox.
  • This change has relevant unit tests (if applicable).
  • This change has relevant documentation additions / updates (if applicable).

`$args['orderby']` was used twice. The second time should have been `$args['order']`.
@brettshumaker brettshumaker requested a review from WPprodigy July 7, 2021 18:17
Copy link

@WPprodigy WPprodigy left a comment

Choose a reason for hiding this comment

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

Before:

SELECT SQL_CALC_FOUND_ROWS ID FROM wp_users ORDER BY 'ID' 'ID' LIMIT 0, 350

(vitess not a fan)

After:

SELECT SQL_CALC_FOUND_ROWS ID FROM wp_users ORDER BY 'ID' 'asc' LIMIT 0, 350

(vitess still not a fan because of quotes around asc >.<)

Previously, the order by direction was quoted: `ORDER BY 'ID' 'desc'` and Vitess didn't like that.

This passes the `ORDER BY` clause through `sanitize_sql_orderby()`, and if that doesn't return `false`, sets up the clause. We then use that directly in the SQL statement.
Copy link

@WPprodigy WPprodigy left a comment

Choose a reason for hiding this comment

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

Vitess approves now :)

@rinatkhaziev rinatkhaziev merged commit 9bdcb2c into develop Aug 4, 2021
@rinatkhaziev rinatkhaziev deleted the fix/typo-in-user-query branch August 4, 2021 19:48
@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] Ready to Deploy [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.

6 participants