WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 11 months ago

Last modified 11 months ago

#25538 closed defect (bug) (fixed)

WP_Query: OR relation breaks orderby meta_value

Reported by: darrengrant Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: major Version:
Component: Query Keywords:
Focuses: Cc:

Description

When using the WP_Query class either for a custom query or the main query using an OR relation in a meta_query will prevent orderby meta_value working as it should. For instance:

$args = array(
   'post_type' => 'post',
   'meta_key' => 'order',
   'meta_value' => 1,
   'meta_compare' => '>=',
   'orderby' => 'meta_value'
 );

will render in a different order to:

$args = array(
   'post_type' => 'post',
   'meta_key' => 'order',
   'meta_value' => 1,
   'meta_compare' => '>=',
   'orderby' => 'meta_value',
   'meta_query' => array(
       'relation' => 'OR',
       array(
           'key' => 'order',
           'compare' => '>=',
           'value' => 1
       )
   )
 );

Even though the two queries are the same. Replacing the relation with AND will give the correct ordering however. Obviously this isn't a useful real-world example, but highlights the issue best.

Attachments (7)

meta.php.diff (1.1 KB) - added by jackreichert 19 months ago.
Pulls meta_key condition to outside of OR parentheses
unit-test-25538.diff (1.7 KB) - added by oso96_2000 19 months ago.
unit-test-25538-1.diff (1.5 KB) - added by jackreichert 18 months ago.
new unit test
unit-test-25538-2.diff (2.0 KB) - added by jackreichert 18 months ago.
Unit test tested and new patch created with updated version in header.
25538.unitspassed.diff (1.6 KB) - added by jackreichert 17 months ago.
refactored patch, now passes unittesting
25538.parens.patch (484 bytes) - added by boonebgorges 15 months ago.
25538.unit-test.2.patch (1.5 KB) - added by boonebgorges 12 months ago.

Download all attachments as: .zip

Change History (40)

comment:1 @nacin20 months ago

  • Component changed from General to Query

Thanks for the report, darrengrant. Let me make sure the right people see this.

comment:2 @wonderboymusic20 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

comment:3 @jackreichert19 months ago

I figured out the problem. I'm net yet fluent with the inner workings of the WP_Query class, yet, to find where to fix the problem. I'll dig into that next, but I wanted to post my findings first so someone else can jump in if they know where.

If you spit out both queries the first is:

SELECT SQL_CALC_FOUND_ROWS wp_posts.* 
FROM wp_posts 
	INNER JOIN wp_postmeta ON (wp_posts.ID = wp_postmeta.post_id) 
WHERE 1=1 
	AND wp_posts.post_type = 'post' 
	AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'private') 
	AND ( (wp_postmeta.meta_key = 'order' 
	AND CAST(wp_postmeta.meta_value AS CHAR) >= '1') ) 
GROUP BY wp_posts.ID 
ORDER BY wp_postmeta.meta_value DESC 
LIMIT 0, 10

The second is:

SELECT SQL_CALC_FOUND_ROWS wp_posts.* 
FROM wp_posts 
	INNER JOIN wp_postmeta ON (wp_posts.ID = wp_postmeta.post_id) 
	INNER JOIN wp_postmeta AS mt1 ON (wp_posts.ID = mt1.post_id) 
WHERE 1=1 
	AND wp_posts.post_type = 'post' 
	AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'private') 
	AND ( (wp_postmeta.meta_key = 'order' 
		AND CAST(wp_postmeta.meta_value AS CHAR) >= '1') 
		OR (mt1.meta_key = 'order' 
		AND CAST(mt1.meta_value AS CHAR) >= '1') ) 	
GROUP BY wp_posts.ID 
ORDER BY wp_postmeta.meta_value DESC 
LIMIT 0, 10

In order to see what's happening I changed the select from wp_posts.* to *.

When you do this it becomes clear what is happening. For efficiency sake I'll only show wp_posts.ID, wp_postmeta.*, mt1.*.

Here are the results:
http://www.jackreichert.com/asset/d3dc35aa/8f2175db.png/

As you can see. The OR comes out in the wrong place, so when it running the condition, it finds mt1 where meta_key = 'order' and doesn't care what meta_key the first joined table joins on. So we get wp_postmeta.meta_key = '_edit_last' in this case.

The solution is, instead of the query condition being:

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

it should be:

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

Pulling the meta_key comparison out of the OR, so the meta_key in both HAS to equal 'order'; the condition should ONLY be applied to the meta_value condition.

(edit: formatting)

Last edited 19 months ago by jackreichert (previous) (diff)

@jackreichert19 months ago

Pulls meta_key condition to outside of OR parentheses

comment:4 @jackreichert19 months ago

  • Keywords has-patch added; needs-patch removed

It turns out the problem was in wp-includes/meta.php.

My solution was to pull the meta_key equation outside of the meta_value OR condition.

comment:5 @ircbot19 months ago

This ticket was mentioned in IRC in #wordpress-dev by jackreichert. View the logs.

comment:6 @oso96_200019 months ago

  • Keywords 2nd-opinion added; needs-unit-tests removed

@jackreichert nice work! I was trying to fix this, but got stuck somewhere. However I had some unit tests using the example provided in the report.

Worth noticing that if we use AND instead of OR, it outputs the same result. Not sure if it's the correct behavior.

comment:7 @jackreichert19 months ago

@oso96_2000 thanks! Took some digging to figure out. Switching AND & OR works because the meta keys are the same, if you did AND with different keys the answers would differ some.

comment:8 @ircbot18 months ago

This ticket was mentioned in IRC in #wordpress-dev by jackreichert1. View the logs.

comment:9 @wonderboymusic18 months ago

  • Keywords 2nd-opinion removed

@oso96_2000: your unit tests pass without the patch...
@jackreichert: can one of you whip up some tests that account for all above comments and demonstrate the need for the patch by failing without it?

comment:10 @jackreichert18 months ago

Now's as good a time as any to hop on the PHPUnit train :)

I'll see what I can do.

@jackreichert18 months ago

new unit test

comment:11 @ircbot18 months ago

This ticket was mentioned in IRC in #wordpress-dev by oso96_2000. View the logs.

comment:12 @jackreichert18 months ago

The svn repo for develop-wordpress is taking me forever to download. So I wasn't able to run this test yet.

But looking it over I think I found why it's not failing and amended the test.

I will confirm it as soon as I can if someone else doesn't get around to it first. But I thought I'd upload it to get it out there.

@jackreichert18 months ago

Unit test tested and new patch created with updated version in header.

comment:13 @jackreichert18 months ago

@wonderboymusic I rechecked the updated unit test. It now reflects the actual problem presented in the bug and will fail. Once the patch is implemented, the test passes.

comment:14 @wonderboymusic17 months ago

Did you run all of the unit tests with your patch? The SQL generation is leaving out a JOIN and producing a bunch of database errors on random meta-related tests

comment:15 @jackreichert17 months ago

I'll take a look.

@jackreichert17 months ago

refactored patch, now passes unittesting

comment:16 @jackreichert17 months ago

phpunit is pretty awesome. Wish I new about it when I first wrote the patch.

So here's what happened. The JOINs were being dropped because on line 825 the method was checking to see if $where[k] was empty, but I did not update that to check $where_meta_key as well.

The second problem was more fundamental to the patch itself (lines 817-821) -- If you have a meta_query with a compare, the AND should not be included inside the OR condition otherwise the condition will always be true (see above).
But the test that was failing (/tests/phpunit/tests/post/query.php:46), was failing because that query is designed to be structured as the method intended it to be.

I think for a future release, or for documentation, I'll take a crack at parsing out that method and annotating the heck out of it.

Last edited 17 months ago by jackreichert (previous) (diff)

comment:17 follow-up: @nacin17 months ago

It is unfortunately just way too late to be messing with meta query. I would love to fix this in 4.0.

jackreichert, thanks so much for the tests and patch! Two minor things I noticed (that did not influence whether this would be in 3.9):

  • They should be the same patch, it's easier.
  • We use tabs, not spaces.

comment:18 @nacin17 months ago

  • Keywords 4.0-early added
  • Milestone changed from 3.9 to Future Release

comment:19 in reply to: ↑ 17 @jackreichert17 months ago

Replying to nacin:

It is unfortunately just way too late to be messing with meta query. I would love to fix this in 4.0.

jackreichert, thanks so much for the tests and patch! Two minor things I noticed (that did not influence whether this would be in 3.9):

  • They should be the same patch, it's easier.
  • We use tabs, not spaces.

Thanks nacin. Totally understand. Looking forward to the release!
Ack! New code editor. I'll configure it.

comment:20 @rmccue15 months ago

Just ran into this. We're in 4.0-early now, let's hit it. :)

comment:21 @wonderboymusic15 months ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 28659:

Fix SQL generation when meta_query has an 'relation' => 'OR' for its queries and wants to 'orderby' => 'meta_value'.

Adds unit test.

Props jackreichert.
Fixes #25538.

comment:22 @wonderboymusic15 months ago

  • Milestone changed from Future Release to 4.0

comment:23 @boonebgorges15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change broke some functionality in BuddyPress. For an admittedly lame reason, we perform some regular expressions and string manipulation on the SQL generated by WP_Meta_Query. r28659 changes the format of the WHERE clauses, by not wrapping each individual one in parentheses, eg

AND wp_postmeta.meta_key = 'foo'

instead of

AND (wp_postmeta.meta_key = 'foo' )

So we can no longer rely on the parentheses as reliable markers of the individual clauses.

I know that this request is totally lame, but perhaps for consistency's sake, you could adopt the same punctuation for the $where_meta_key clause as for the old $where. See 25538.parens.patch. (I've kept the idiosyncratic spacing, but that's not important.)

comment:24 @wonderboymusic15 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 28665:

Wrap individual meta query WHERE clases in parens, missed in 28659.

Props boonebgorges.
Fixes #25538.

comment:25 @boonebgorges12 months ago

#29285 was marked as a duplicate.

comment:26 @boonebgorges12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

  1. 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..

  1. 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.

  1. 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")

comment:27 @wonderboymusic12 months ago

I love Meta Query

comment:28 @ircbot12 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:29 @nacin12 months ago

In 29650:

Meta Query: Revert [28659] (and [28665]) due to regressions.

props boonebgorges.
fixes #29285. see #25538.

comment:30 @nacin12 months ago

  • Keywords needs-patch added; has-patch 4.0-early removed
  • Milestone changed from 4.0 to Future Release

comment:31 @boonebgorges11 months ago

#29604 was marked as a duplicate.

comment:32 @boonebgorges11 months ago

  • Keywords needs-patch removed
  • Milestone changed from Future Release to 4.1
  • Owner changed from wonderboymusic to boonebgorges
  • Status changed from reopened to accepted

The duplicate ticket #29604 prompted me to do some more investigation of this issue. I managed to confirm it, with the critical piece of the puzzle being that the bug only arises when you have more than one meta_query clause being linked by the 'OR' relation. This wasn't stated clearly in the original bug report, and the unit test that jackreichert originally suggested didn't set up the data in the right way to illustrate the problem.

The crux of the issue is along the lines of what jackreichert discusses here https://core.trac.wordpress.org/ticket/25538#comment:3. It breaks down like this: 'orderby=meta_value&meta_key=foo' requires that each located item have a value for 'foo'. But when this is coupled with a meta_query whose relation is 'OR', the query will return results that *don't* have 'foo', as long as they meet one of the other OR conditions. Schematically, if you have 'orderby=meta_value&meta_key=foo' along with meta_query = (color = blue OR vegetable = celery), that should compile down to a nested query that links the orderby clause together with the rest of the meta query with an AND. In other words, the end result needs to be something like:

mt1.meta_key = 'foo' AND (
    ( mt2.meta_key = 'color' AND mt2.meta_value = 'blue' )
    OR
    ( mt3.meta_key = 'vegetable' AND mt3.meta_value = 'celery' )
)

Part of the problem with the approaches previously ventured to solve this problem was that WP_Meta_Query didn't support the construction of this sort of nested syntax, but since #29642 [29887] it does. (Tada!) So now we can fix this problem properly.

Last edited 11 months ago by boonebgorges (previous) (diff)

comment:33 @boonebgorges11 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

[29964] missed the ticket.

Note: See TracTickets for help on using tickets.