Make WordPress Core

Opened 9 years ago

Last modified 6 years ago

#36652 reviewing enhancement

Use meta_value in a meta query to decide type format in SQL clause

Reported by: ericlewis's profile ericlewis Owned by: ericlewis's profile ericlewis
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch has-unit-tests 2nd-opinion needs-refresh
Focuses: Cc:

Description (last modified by ericlewis)

The SQL clause generated for a meta query quotes the `meta_value` in a string.

This means that if there's a post with a postmeta field for likes set to 2 and you run the query looking for posts with 10 or more likes

<?php
$query = new WP_Query( array(
        'meta_query' => array(
                array(
                        'key' => 'likes',
                        'value' => 10,
                        'compare' => '>='
                )
        )
) );

the query will return the post with 2 likes. This is because the SQL is doing a string comparison, as both the column value and the compared-to value are strings.

The fix for the developer is to supply a type parameter like NUMERIC in the meta query clause which coerces a numeric MySQL comparison.

We could use the meta_value's type to decide the type format the value takes in the SQL clause, so that a query like this works as expected without the type parameter.

This was suggested by @boone in #27272.

Attachments (1)

36652.diff (10.5 KB) - added by boonebgorges 9 years ago.

Download all attachments as: .zip

Change History (8)

#1 @ericlewis
9 years ago

  • Description modified (diff)

@boonebgorges
9 years ago

#2 follow-up: @boonebgorges
9 years ago

  • Keywords has-patch has-unit-tests 2nd-opinion added
  • Milestone changed from Awaiting Review to 4.6

36652.diff is a first pass at how this might work. High-level overview:

  • Takes a conservative approach to determining when 'type' can be inferred:
    • is_int( $value ) || is_float( $value ), and 'compare' is of the type that makes sense with numeric casts (=, !=, <, <=, >, >=)
    • $value is an array, and *each* member of the array is a float or an integer, and 'compare' is either BETWEEN or NOT BETWEEN
  • Inferring 'type' is not enough. We also have to infer the proper type selectors for $wpdb->prepare() - %d, %f, %s

The tests in 36652.diff should spell this out pretty clearly.

A few questions for consideration.

  1. 36652.diff opts to infer SIGNED when 'compare' is = or !=. This means that certain kinds of meta queries common in core are CAST() to SIGNED when they probably don't have to be. This is especially true in places where you're storing IDs in meta (see eg wp_get_associated_nav_menu_items()) - meta_value = '3' is the same as CAST(meta_value AS SIGNED) = 3 for these purposes. The CAST may be slightly slower, and breaks the use of any index that might exist. On the flip side, the CAST is important if you want mathematical matches (3 == 3.00). So I'm a bit torn on the expected behavior.
  2. I'm using the term "type selector" for %s etc because that's what the PHP docs use http://php.net/manual/en/function.sprintf.php. If anyone has a better idea ("placeholder"?), shoot.
  3. Possible compat breaks are cases where one is passing numeric values but is expecting string behavior. Eg: "10" < "1" (alpha sort) but 10 > 1 (numerical sort). It's only in certain corner cases that this problem will ever arise. My guess is that the type-inferring suggested in this ticket will cause a number of bugs like this that is far smaller than the number of type-related bugs it'll fix. But a second opinion would be helpful.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


9 years ago

#4 @ocean90
9 years ago

  • Owner set to ericlewis
  • Status changed from new to reviewing

@ericlewis Can you review @boonebgorges's patch and decide if this is still something for 4.6? Looks like the change should go in ASAP.

#5 in reply to: ↑ 2 @ericlewis
9 years ago

Awesome to see an implementation for this already :D

Replying to boonebgorges:

  1. 36652.diff opts to infer SIGNED when 'compare' is = or !=.

Casting float values to SIGNED will make them integers, and miss matches. e.g.

update_post_meta( 1, 'glerf', '1.1' );

$q = new WP_Query(array(
  'meta_query' => (
    array(
      array(
        'key' => 'glerf',
        'value' => 1.1
      )
    )
  )
));
  1. I'm using the term "type selector" for %s etc because that's what the PHP docs use http://php.net/manual/en/function.sprintf.php. If anyone has a better idea ("placeholder"?), shoot.

Adhering to PHP's verbiage here would be nice.

  1. Possible compat breaks are cases where one is passing numeric values but is expecting string behavior. Eg: "10" < "1" (alpha sort) but 10 > 1 (numerical sort). It's only in certain corner cases that this problem will ever arise. My guess is that the type-inferring suggested in this ticket will cause a number of bugs like this that is far smaller than the number of type-related bugs it'll fix. But a second opinion would be helpful.

That's a good question. In this ticket we would introduce support for passing the meta_value parameter as
an int or float, which has been supported previously, but not in the inline documentation. The case you suggest
does seem rather edge — expecting alpha sort on comparing 10 to 1. Sidenote, we should
probably change the docs for meta_value if we move forward here
.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


9 years ago

#7 @ocean90
9 years ago

  • Keywords needs-refresh added
  • Milestone changed from 4.6 to Future Release

Thanks for the review @ericlewis. Punting since it needs a refresh and beta 1 is tomorrow.

Note: See TracTickets for help on using tickets.