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: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Query | Keywords: | has-patch has-unit-tests 2nd-opinion needs-refresh |
Focuses: | Cc: |
Description (last modified by )
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.
Attachments (1)
Change History (8)
#2
follow-up:
↓ 5
@
9 years ago
- Keywords has-patch has-unit-tests 2nd-opinion added
- Milestone changed from Awaiting Review to 4.6
This ticket was mentioned in Slack in #core by ocean90. View the logs.
9 years ago
#4
@
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
@
9 years ago
Awesome to see an implementation for this already :D
Replying to boonebgorges:
- 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
)
)
)
));
- 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.
- Possible compat breaks are cases where one is passing numeric values but is expecting string behavior. Eg:
"10" < "1"
(alpha sort) but10 > 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.
36652.diff is a first pass at how this might work. High-level overview:
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 eitherBETWEEN
orNOT BETWEEN
$wpdb->prepare()
-%d
,%f
,%s
The tests in 36652.diff should spell this out pretty clearly.
A few questions for consideration.
SIGNED
when 'compare' is=
or!=
. This means that certain kinds of meta queries common in core areCAST()
toSIGNED
when they probably don't have to be. This is especially true in places where you're storing IDs in meta (see egwp_get_associated_nav_menu_items()
) -meta_value = '3'
is the same asCAST(meta_value AS SIGNED) = 3
for these purposes. TheCAST
may be slightly slower, and breaks the use of any index that might exist. On the flip side, theCAST
is important if you want mathematical matches (3 == 3.00
). So I'm a bit torn on the expected behavior.%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."10" < "1"
(alpha sort) but10 > 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.