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

SQL_CALC_FOUND_ROWS statements refactored #1203

Merged
merged 1 commit into from Jan 5, 2021

Conversation

kidunot89
Copy link
Contributor

Fixes #1200 .

Checklist

  • SQL_CALC_FOUND_ROWS removed from MySQL statements.
  • Result count assigned a modified count query.

@kidunot89 kidunot89 self-assigned this Dec 10, 2020
Comment on lines +249 to +263
// Build result count query.
$count_query = "SELECT COUNT(*) as found
FROM $wpdb->stream
{$join}
WHERE 1=1 {$where}";

/**
* Filter allows the result count query to be modified before execution.
*
* @param string $query
* @param array $args
*
* @return string
*/
$count_query = apply_filters( 'wp_stream_db_count_query', $count_query, $args );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivankruchkoff Here's result count query.

Comment on lines +269 to +270
'items' => $wpdb->get_results( $query ), // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
'count' => absint( $wpdb->get_var( $count_query ) ), // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivankruchkoff @kasparsd @derekherman I added explicit ignore comments here. I think don't anymore sanitation needs to be done because all dynamic parts of the query are run thru $wpdb->prepare() earlier in the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, all the dynamic parts seem to be fine. However, we're now passing those queries through filters as the very last thing. I guess we need to trust that whoever is using that filter will do their job to sanitize the adjusted parts.

*
* @return string
*/
$count_query = apply_filters( 'wp_stream_db_count_query', $count_query, $args );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a filter here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it will be needed, If someone is modifying the select query, they would need to modify the count query as well to make the count match the results shown.

Copy link
Contributor

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

@kidunot89 Looks like everyone is good with the approach here. Can you please merge this in?

@kidunot89 kidunot89 added this to the 3.6.1 milestone Jan 4, 2021
@kidunot89 kidunot89 merged commit b8a08b5 into develop Jan 5, 2021
@kidunot89 kidunot89 deleted the bugfix/deprecated-mysql-fix branch January 5, 2021 22:10
@kidunot89 kidunot89 mentioned this pull request Jan 12, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove SQL_CALC_FOUND_ROWS
4 participants