[wp-trac] [WordPress Trac] #25538: WP_Query: OR relation breaks orderby meta_value

[WordPress Trac]

#25538: WP_Query: OR relation breaks orderby meta_value
---------------------------------+-----------------------------
 Reporter:  darrengrant          |       Owner:  wonderboymusic
     Type:  defect (bug)         |      Status:  reopened
 Priority:  normal               |   Milestone:  4.0
Component:  Query                |     Version:
 Severity:  major                |  Resolution:
 Keywords:  has-patch 4.0-early  |     Focuses:
---------------------------------+-----------------------------
Changes (by boonebgorges):

 * status:  closed => reopened
 * resolution:  fixed =>


Comment:

 I've just closed #29285 as a duplicate. In brief, the changes in r28659
 have broken meta_query when passing more than one clause, when those
 clauses have 'compare' parameters. This will be a pretty major regression
 in WP 4.0, and I think the best course of action for now is to revert
 r28659.

 There's a lot to say below, so here's the executive summary:

 1. The fix committed in r28659 breaks the logic of the OR relation
 2. The unit test committed in r28659 doesn't demonstrate the reported
 issue
 3. I'm not able to reproduce the original bug, given some peculiarities of
 the way that `'meta_query'` parameters are parsed together with
 `'meta_value'`, `'meta_key'`, and `'meta_compare'` args passed directly to
 WP_Query
 4. A better way forward is to refactor the parsing mentioned in #3, and
 pass back a table alias + column name back to WP_Query for constructing
 the ORDER BY clause.

 ===

 The gory details.

 1. This changeset results in a significant change in the behavior of OR
 clauses. After this ticket, the following SQL generation takes place:

 {{{
 'meta_query' => array
     'relation' => 'OR',
     array(
         'key' => 'foo',
         'value' => 'foovalue',
         'compare' => '=',
     ),
     array(
         'key' => 'bar',
         'value' => 'barvalue',
         'compare' => '=',
     ),
 ),
 }}}

 becomes (relevant WHERE clause only, whitespace changed for clarity):

 {{{
 AND (
     (CAST(wptests_postmeta.meta_value AS CHAR) = 'foovalue2')
     OR
     (CAST(mt1.meta_value AS CHAR) = 'barvalue2')
 )
 AND
 (
     wptests_postmeta.meta_key = 'foo'
     AND
     mt1.meta_key = 'bar'
 )
 }}}

 Breaking the 'meta_key' matches out of the OR relation means that a post
 will only match if it has a meta value for 'foo' *and* one for *bar*. This
 is not what 'relation=OR' is intended to mean, and in any case it's a
 fairly major change from the current behavior. The meta_query listed above
 should match posts 1 and 2 in the schema below, but it matches neither:

 {{{
 post_id    meta_key    meta_value
 1          foo         foovalue
 2          bar         barvalue
 }}}

 This is the main reason why I think the changeset should be reverted.

 2. The unit test, as originally written, passes for me before the original
 patch was applied. Part of this is because the test is not very precisely
 written - it creates posts with post_date order that could match the
 custom 'order' param, and it does a loose equality comparison of two
 different get_posts() queries rather than doing a strict check of what
 those queries actually turn up.

 I've cleaned up the unit test in 25538.unit-test.2.patch. However, it's
 still not showing anything for me: it passes on r28658 (just before it was
 committed) as well as after. Some debugging convinces me that..

 3. I can't reproduce the original bug. If you pass meta_key, meta_value,
 and meta_compare arguments alongside a meta_query into WP_Query, the
 standalone arguments are parsed into the meta_query in
 `WP_Meta_Query::parse_query_vars()`. So the arguments described in the
 second example of the OP turn into:

 {{{
 'meta_query' => array(
     'relation' => 'OR',
     array(
         'key' => 'order',
         'value' => 1,
         'compare' => '>=',
     ),
     array(
         'key' => 'order',
         'value' => 1,
         'compare' => '>=',
     ),
 ),
 }}}

 Note that the two clauses are duplicates. That's because
 `parse_query_vars()` turns meta_key + meta_value + meta_compare into a
 standard meta_query clause and then merges it with the other clauses. (I
 guess it fails to pass a strict equivalence test, which is why there's a
 dupe.) This then translates to the following SQL (note that this is before
 the patch, when the meta_key clauses were inside of the OR clauses):

 {{{
  AND ( (wptests_postmeta.meta_key = 'order' AND
 CAST(wptests_postmeta.meta_value AS CHAR) >= '1')
 OR  (mt1.meta_key = 'order' AND CAST(mt1.meta_value AS CHAR) >= '1') )
 }}}

 The table alias wptests_postmeta is from the
 meta_key+meta_value+meta_compare args, while mt1 is from the 'meta_query'
 clause. This is important, because when WP_Query translates
 'orderby=meta_value' into an ORDER BY clause, it uses `$wpdb->postmeta` as
 the table alias: https://core.trac.wordpress.org/browser/tags/3.9.2/src
 /wp-includes/query.php#L2653 As a result, *the alias will always match
 whatever is passed in meta_key+meta_value+meta_compare*, which means that
 the ordering should always be faithful, regardless of JOINs.

 4. In the future, I think a better strategy for handling order +
 meta_query would involve the following:

 - As WP_Meta_Query transforms meta_query clauses into SQL, it should store
 an array of the table aliases it creates, and they should get passed back
 to WP_Query
 - When WP_Query builds ORDER BY out of orderby=meta_value, it should do
 some logic like this:
     - meta_value corresponding to which column? For this, look at the
 meta_key query var
     - Then look up the table alias for that meta_key in the array passed
 from WP_Meta_Query::get_sql()
     - Use that table alias when building the ORDER BY clause ("ORDER BY
 mt2.meta_value")
     - Maybe build in support for multiple orderby, for tie-breaking etc
 ("ORDER BY mt2.meta_value ASC, mt1.meta_value DESC")

--
Ticket URL: <https://core.trac.wordpress.org/ticket/25538#comment:26>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform