Skip to content

Fix busted IN, NOT IN query system [1]#977

Closed
lkraav wants to merge 1 commit into
xwp:developfrom
lkraav:issue-958-fix-busted-in-not-in-query-system
Closed

Fix busted IN, NOT IN query system [1]#977
lkraav wants to merge 1 commit into
xwp:developfrom
lkraav:issue-958-fix-busted-in-not-in-query-system

Conversation

@lkraav
Copy link
Copy Markdown
Contributor

@lkraav lkraav commented Apr 23, 2018

(retrying #958)

Trying to use any __in or __not_in results in a database error:

WordPress database error You have an error in your SQL syntax; check the
manual that corresponds to your MariaDB server version for the right syntax to
use near ''context' IN ('')

It looks like the algorithm here has been busted for years. array_shift()
mutates the underlying $values array, losing data user has passed into the
query parameters.

This required rewriting some of the algorithm and throwing some stuff out.

I modelled the fix mostly by grepping for prepare.*\\\ IN\\\ for examples
and picked 1

EXAMPLE QUERY (custom connector posts-viewed)

$stream = wp_stream_get_instance();

 $records = $stream->db->get_records( [
     'action' => 'viewed',
     'author' => get_current_user_id(),
     'connector' => 'posts-viewed',
     'context__in' => [ 'course', 'lesson' ],
 ] );

Trying to use any `__in` or `__not_in` results in a database error:

```
WordPress database error You have an error in your SQL syntax; check the
manual that corresponds to your MariaDB server version for the right syntax to
use near ''context' IN ('')
```

It looks like the algorithm here has been busted for years. `array_shift()`
mutates the underlying `$values` array, losing data user has passed into the
query parameters.

This required rewriting some of the algorithm and throwing some stuff out.

I modelled the fix mostly by grepping for `prepare.*\\\ IN\\\ ` for examples
and picked [1]

EXAMPLE QUERY (custom connector `posts-viewed`)
```
$stream = wp_stream_get_instance();

 $records = $stream->db->get_records( [
     'action' => 'viewed',
     'author' => get_current_user_id(),
     'connector' => 'posts-viewed',
     'context__in' => [ 'course', 'lesson' ],
 ] );
```

[1]: https://github.com/WordPress/WordPress/blob/4.9.1/wp-includes/class-wp-comment-query.php#L794
Copy link
Copy Markdown
Contributor

@lukecarbis lukecarbis left a comment

Choose a reason for hiding this comment

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

Sorry it's taken me almost a year to review this! Left a couple of comments, but keen to include this fix in a release soon.

Comment thread classes/class-query.php
$field = str_replace( array( 'record_', '__in' ), '', $key );
$field = empty( $field ) ? 'ID' : $field;
$type = is_numeric( array_shift( $value ) ) ? '%d' : '%s';
$type = is_numeric( $value[ 0 ] ) ? '%d' : '%s';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This won't work if the first value in the array isn't 0. Could we use this as a non-mutating solution, instead?
array_shift( array_values( $value ) );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But in what case wouldn't the first item's array index be exactly 0? Can there be string array index values, or?

I've yet to see any PHP warnings or anything in the 1 year this has been running in production. Wouldn't there be?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It wouldn't be unreasonable to filter the post ids I'm sending as an arg. For example:

$post_ids = array( 1, 2, 3, 4, 5, 6, 7, 8, 9 );
$args['__in'] = array_filter( $post_ids, function( $id ) {
    // Returns whether the integer is odd
    return($var & 1);
} );

That would break the zero index. Also unsetting it as in this example:

foreach ( $args['__in'] as $key => $value ) {
    if ( $value > 100 ) {
        unset( $args['__in'][ $key ];
    }
}

Comment thread classes/class-query.php
$field = str_replace( array( 'record_', '__not_in' ), '', $key );
$field = empty( $field ) ? 'ID' : $field;
$type = is_numeric( array_shift( $value ) ) ? '%d' : '%s';
$type = is_numeric( $value[ 0 ] ) ? '%d' : '%s';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here – can we use the non-mutating solution above?

@lkraav
Copy link
Copy Markdown
Contributor Author

lkraav commented Jun 22, 2019

#WCEU notes w/ @kasparsd

  • sanitize prepare statement $field name vs table field names, because prepare will single-quote field name and generate invalid SQL
  • $value needs to handle arrays for IN statement. array_values() suggestion by @lukecarbis seems to work well for this purpose.

Comment thread classes/class-query.php

if ( ! empty( $value ) ) {
$format = '(' . join( ',', array_fill( 0, count( $value ), $type ) ) . ')';
$where .= $wpdb->prepare( " AND $wpdb->stream.%s IN {$format}", $field, $value ); // @codingStandardsIgnoreLine prepare okay
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$wpdb->stream.%s always becomes $wpdb->stream.'FIELDNAME' which never works. We need to specify the field name picking it from a list of known "property" field names and use it directly as $wpdb->stream.{$field}.

@kasparsd kasparsd mentioned this pull request Jun 24, 2019
2 tasks
@lkraav lkraav closed this Mar 9, 2022
@lkraav lkraav deleted the issue-958-fix-busted-in-not-in-query-system branch March 9, 2022 11:34
@lkraav lkraav restored the issue-958-fix-busted-in-not-in-query-system branch March 9, 2022 11:34
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.

4 participants