Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#38442 new defect (bug)

Error when WP_Query parses orderby array in function parse_order

Reported by: oloynet's profile oloynet Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.2
Component: Query Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Example of query where I want to order by two custom fields in meta query
start_date_order and is_sticky

<?php
$args = array(
    'no_found_rows'  => true ,
    'posts_per_page' => 4,
    'post_type'      => 'event',
    'post_status'    => 'publish',

    'meta_query' => array(
        'relation' => 'AND',
        array(
            'key'  => 'start_date_order',
            'type' => 'UNSIGNED',
        ),
        array(
            'key'  => 'is_sticky',
            'type' => 'UNSIGNED',
        ),
    ),
    'orderby' => array(
        'start_date_order' => 'DESC',
        'is_sticky'        => 'ASC',
    ),
);

$result_query = new WP_Query( $args );

echo $result_query->request;

The wrong SQL query generated is

SELECT wp_posts.ID
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_postmeta.meta_key = 'start_date_order'
    AND mt1.meta_key = 'is_sticky'
)
AND wp_posts.post_type = 'event'
AND ((wp_posts.post_status = 'publish'))
GROUP BY wp_posts.ID
ORDER BY CAST(wp_postmeta.meta_value AS UNSIGNED) DESC
LIMIT 0, 3

The '$primary_meta_query' var in method parse_order( $order ) is set forever with the first item of '$meta_clauses' array

see line 2336 /wp-includes/query.php

<?php
primary_meta_query = reset( $meta_clauses );

I quickly fix the problem with the following PHP code. Could use a native array function from PHP.

<?php
//$primary_meta_query = reset( $meta_clauses );

$primary_meta_query = array();
foreach( $meta_clauses as $meta_clause ) {
    if( $meta_clause['key'] == $orderby ) {
        $primary_meta_query = $meta_clause;
        break;
    }
}

Now the good SQL query is generated with two columns for ordering:

SELECT wp_posts.ID
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_postmeta.meta_key = 'start_date_order'
    AND mt1.meta_key = 'is_sticky'
)
AND wp_posts.post_type = 'event'
AND ((wp_posts.post_status = 'publish'))
GROUP BY wp_posts.ID
ORDER BY CAST(wp_postmeta.meta_value AS UNSIGNED) DESC, CAST(mt1.meta_value AS UNSIGNED) ASC
LIMIT 0, 3

Attachments (3)

patch-wp-query.txt (811 bytes) - added by oloynet 7 years ago.
38442.patch (913 bytes) - added by oloynet 7 years ago.
Patch for bur report 38442 with svn diff
38442.diff (3.0 KB) - added by desrosj 5 years ago.

Download all attachments as: .zip

Change History (11)

#1 @oloynet
7 years ago

The correct fix is

<?php
$primary_meta_query = false;
foreach( $meta_clauses as $meta_clause ) {
    if ( $meta_clause['key'] == $orderby ) {
        $primary_meta_query = $meta_clause;
        break;
    }
}
if( ! $primary_meta_query ) {
    $primary_meta_query = reset( $meta_clauses );
}

#2 @oloynet
7 years ago

  • Keywords has-patch 2nd-opinion added
  • Severity changed from normal to major

#3 @swissspidy
7 years ago

  • Component changed from Filesystem API to Query

#4 @swissspidy
7 years ago

  • Keywords needs-unit-tests added
  • Severity changed from major to normal

Hey there,

Thank you very much for your report and welcome to WordPress Trac! The change looks reasonable at first glance.

It would be nice to have unit tests showing this bug and how the patch resolves it. Also, the patch should ideally have a .patch or .diff extension and not contain any start patch and end patch comments.

Let me know if you're familiar with automated testing or not. I'm sure someone else might be able to step up here.

@oloynet
7 years ago

Patch for bur report 38442 with svn diff

#5 @oloynet
7 years ago

Hello,

I'm not a fluent user with unit tests on Wordpress.
I've made a new patch with svn diff to do the job

Olivier

@desrosj
5 years ago

#6 @desrosj
5 years ago

  • Keywords has-unit-tests added; 2nd-opinion needs-unit-tests removed
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 4.6.1 to 4.2

I dug into this to understand it a bit better. In the current state, ordering by multiple meta keys only works when using explicit indexes. For example, if the query in your example is changed to this, @oloynet, I am seeing the correct SQL generated:

'meta_query' => array(
	'relation' => 'AND',
	'date_meta' => array(
		'key'  => 'start_date_order',
		'type' => 'UNSIGNED',
	),
	'sticky_meta' => array(
		'key'  => 'is_sticky',
		'type' => 'UNSIGNED',
	),
),
'orderby' => array(
	'date_meta'.  => 'DESC',
	'sticky_meta' => 'ASC',
),

38442.diff is a refresh of the original patch accompanied by unit tests and a few changes:

  • We don't need to declare $primary_meta_query = false;. That is redundant since the same declaration is just outside the if statement.
  • Use strict comparison wghen comparing key with $orderby.
  • Add a check that $meta_clause['key'] is actually set to avoid PHP notices.

If you only add the unit test, the tests will fail. Adding the other part of the patch will result in the tests passing.

@boonebgorges Are you able to review?

#7 @boonebgorges
5 years ago

Thanks, @desrosj. I'll review next week.

#8 @boonebgorges
5 years ago

Logic of 38442.diff looks good to me.

Ideally, we'd also have a test that demonstrates the bug's effect on query results, not just the MySQL query string.

Note: See TracTickets for help on using tickets.