WP meta queries comparing apples to oranges

[Denis-de-Bernardy]

As seen when hooking into the posts_selection hook while debugging a site:

( (ts_wp_postmeta.meta_key = 'financial_max_annual' AND CAST(ts_wp_postmeta.meta_value AS SIGNED) <= '100000') )

The thing that led to that was:

                $meta_query[] = array(
                    'key'       => $meta_key,
                    'value'     => $max_fee,
                    'compare'   => '<=',
                    'type'      => 'NUMERIC',
                );

Can I suggest that, when we CAST, we do so on both sides?

[wonderboymusic]

Yes.

[wonderboymusic]

27272.diff​ implements this for SIGNED and UNSIGNED only. Other casts were behaving weirdly.

[nacin]

Do we actually need to cast both sides? Could we just make sure that a numeric cast on one side ends up with an integer on the other?

[wonderboymusic]

Could use %d and %u as $cast_string for those instead of the explicit cast

[wonderboymusic]

27272.2.diff​ is better - could add more cases as necessary

[Denis-de-Bernardy]

I think I preferred the first patch. Casting both side leaves the door open to doing:

where cast(meta_key as date) > cast(%s as date)

At a conceptual level, I'd suggest the following high-level test case:

1. Insert post where meta_value = 2
2. Verify that it's found if the meta value query is >= 1
3. Verify that it's NOT found if the meta value query is >= 10

[wonderboymusic]

I'll incorporate that into a unit test - will ask some others to weigh in on the generated SQL

[samuelsidler]

This has been like this since meta queries were introduced in 3.1. It was reported fairly late in the cycle. We can deal with it properly in 4.0.

[boonebgorges]

What is it that we're trying to accomplish with the casting? MySQL does some somewhat intelligent type juggling based on the operators you use ​https://dev.mysql.com/doc/refman/5.0/en/type-conversion.html. So the kind of example Denis-de-Bernardy describes here https://core.trac.wordpress.org/ticket/27272#comment:6 is actually already supported. (See 27272.patch.) As for things like date, my tests suggest that as long as you're passing the proper 'type' param for casting the 'meta_value' column, MySQL will automatically convert the type of your comparison value. So CAST('2012-04-04' AS DATE) > CAST(meta_value AS DATE) is redundant.

That said, it's possible that I'm missing something. Can we get a unit test that demonstrates how the suggested casting makes a difference in results?

[Denis-de-Bernardy]

Replying to boonebgorges:

That said, it's possible that I'm missing something. Can we get a unit test that demonstrates how the suggested casting makes a difference in results?

See comment-6 further up:

1. insert post meta where value = 2
2. test that a query where value >= 1 yields the row
3. test that a query where value >= 10 does NOT yield the row

The test currently fails miserably at step 3, because the SQL it produces is like:

CAST(wp_postmeta.meta_value AS SIGNED) <= '10'

The left-hand side cast is basically ignored, because 10 got sanitized as a string.

Therefor my initial suggestion: rather than introduce potential for problems by not systematically sanitizing things, just cast both sides of the operator when a plugin is explicitly requesting a typed meta query.

[boonebgorges]

The test currently fails miserably at step 3

See 27272.test.patch​. It demonstrates that these queries do *not* currently fail for types SIGNED, NUMERIC, and DATETIME. Note that the key here is that, in each case, a 'type' is passed with the tax query clause. These same queries will fail if you don't pass an explicit 'type', because the cast will fall back on strings. But as long as you are manually handling the casting on one side of the comparison, MySQL will handle it on the other side.

There is a further question about whether we should be kinder to developers, by assuming that they mean "SIGNED" when they pass a numeric 'value' but don't specify the 'type'. IMO this would probably be a decent enhancement, but I think it's different from what's being suggested here.

[gitlost]

Queries will fail though (depending on system) if you trigger the MySQL string to float conversion mentioned in ​https://dev.mysql.com/doc/refman/5.0/en/type-conversion.html by using large numbers, see appended updated meta patch and test patch.

MySQL does do good type-juggling, but it seems frankly silly to invoke it unnecessarily. Quoting numerics ain't natural!

[gitlost]

Patch ( 2nd wonderboy one) updated for 4.1.

[gitlost]

Unit tests for patch, large numerics.

[gitlost]

Refresh for 4.4.

[gitlost]

Refresh for 4.4.

[gitlost]

Refreshes for gardening drive.

[gitlost]

Refresh for 4.5.

[gitlost]

Refresh for 4.5 Bug Scrub (just removes "Hunk succeeded" messages). 27272_test2.diff still good.

[gitlost]

Added back original tests from 27272.2.diff.

[gitlost]

@boonebgorges per ​https://wordpress.slack.com/archives/core/p1454693086001759, added back tests that were missed (originally in "27272.2.diff").

[gitlost]

Didn't mean to change the resolution or status...

[ericlewis]

It seems the original bug reported by this ticket is not a bug (see comment:13). I suggest we close this ticket as worksforme after tying up loose ends, as a few engaging points have been brought up.

• @boonebgorges wrote a unit test to express that the original bug report is invalid. Was that intended as a prop for confirming the bug wasn't a bug, or should we consider merging that test?
• There's some thought about using the meta_type do decide which numeric format to use when building the SQL query (@wonderboymusic's attachment:27272.2.diff​ which @gitlost has kept fresh). This is a separate issue from the original bug report, although related. We should consider using the type of the value passed in the meta query too for a good default that doesn't require a type parameter (basically what @boonebgorges suggested in comment:13). I've opened #36652 to that end.
• @gitlost reported a bug in its own right in comment:14. @gitlost — can you create a separate ticket and outline the problem?

[gitlost]

@ericlewis wilco...