Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#31045 closed feature request (fixed)

Ordering a Query Using Multiple Meta Keys

Reported by: funkatronic's profile Funkatronic Owned by: boonebgorges's profile boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Query Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Currently, WordPress doesn't support sorting a query by multiple meta keys; we can only sort by one, even though we can now query posts by multiple meta entries. My idea is that for meta queries, we allow the coder querying to assign the table aliases in the query arrays. Either by adding a new argument:

$args = array(
	'post_type'  => 'product',
	'meta_query' => array(
		array(
			'key'     => 'color',
			'value'   => 'blue',
			'compare' => 'NOT LIKE',
                        'alias'   => 'color'
		),
	),
        'orderby' => 'meta-color'  //prefix to prevent any collisiona
);

$query = new WP_Query( $args );

Or making array indexes the alias:

$args = array(
	'post_type'  => 'product',
	'meta_query' => array(
		'color' => array(
			'key'     => 'color',
			'value'   => 'blue',
			'compare' => 'NOT LIKE',
		),
	),
        'orderby' => 'meta-color'  //prefix to prevent any collisiona
);

$query = new WP_Query( $args );

Or just use the meta key as the alias:

$args = array(
	'post_type'  => 'product',
	'meta_query' => array(
		array(
			'key'     => 'color',
			'value'   => 'blue',
			'compare' => 'NOT LIKE',
		),
	),
        'orderby' => 'meta-color'  //prefix to prevent any collisiona
);

$query = new WP_Query( $args );

In either case, the query would denote the alias used for the table in the mySQL query, then you can specify in the orderby clause which ones you want to sort by, allowing for multiple if the need be.

Attachments (7)

meta.php.patch (757 bytes) - added by Funkatronic 10 years ago.
Patch 1 checks for and applies user defined alias in meta query in joins
meta-query.patch (3.5 KB) - added by Funkatronic 10 years ago.
New patch
meta-query.2.patch (3.5 KB) - added by Funkatronic 10 years ago.
Fixed an error in the last patch
31045.diff (7.3 KB) - added by boonebgorges 10 years ago.
31045.patch (4.5 KB) - added by Funkatronic 10 years ago.
Fixed a lapse in coding judgement by me
31045.2.patch (7.4 KB) - added by Funkatronic 10 years ago.
Added documentation to parse_orderby filter
31045.2.diff (7.7 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (39)

#1 @SergeyBiryukov
10 years ago

  • Component changed from Database to Query
  • Focuses performance removed

#2 @boonebgorges
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

A big +1. I've had the same idea in the past; see #29447 and point 4 in https://core.trac.wordpress.org/ticket/25538#comment:26.

Adding an 'alias' argument to meta_query clauses seems like the most consistent way forward. WP_Query already supports multiple ORDER BY clauses (see https://make.wordpress.org/core/2014/08/29/a-more-powerful-order-by-in-wordpress-4-0/ and #17065), so the 'orderby' syntax would have to integrate with that. I would lean toward *not* using a 'meta-' prefix for aliases, since (a) the possibility of clashes is very slight, and (b) developers have full control over 'alias' and so it's not unreasonable to expect them to check for clashes themselves.

Ideally, these aliases would also be used in building the SQL itself, which would make certain kinds of SQL filters easier to write. Aliases are currently generated here https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/meta.php#L1353, and this would need a bit of reworking to ensure that ordering works properly with shared table aliases.

#3 @Funkatronic
10 years ago

I have diff on applying custom aliases for meta queries. Next step is figuring out the syntax for using them in an orderby. Any ideas, @boonebgorges?

@Funkatronic
10 years ago

Patch 1 checks for and applies user defined alias in meta query in joins

#4 @boonebgorges
10 years ago

  • Keywords needs-unit-tests added

Cool, thanks for the initial patch. For syntax, we should tap into the existing WP_Query orderby code. Check out parse_orderby() https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/query.php#L2207. It gets a "primary_meta_key" from the meta query and checks it against the keys passed to orderby. We need to keep some of this logic, because orderby=meta_value will continue to work this way. But for the more general case, I suppose we'll have to loop through the meta query clauses and compile a list of available aliases.

#5 @Funkatronic
10 years ago

So, 'orderby' => 'alias'?

Also, a WP_Meta_Query object stores its aliases in an array(WP_Meta_Query->table_aliases), so we can use that to keep track of aliases. Still studying the logic so I can code something that doesn't break anything.

#6 @boonebgorges
10 years ago

I'm thinking:

$q = new WP_Query( array(
    'meta_query' => array(
        array(
            'key' => 'foo',
            'value' => 'foo value',
            'alias' => 'foo',
        ),
        array(
            'key' => 'bar',
            'value' => 'bar value',
            'alias' => 'bar123',
        ),
    ),
    'orderby' => array(
        'foo' => 'ASC',
        'bar123' => 'DESC',
    ),
) );

I believe the 'table_aliases' array is a flat array that's mainly used for counts and incrementing. It doesn't store any associations with the query clauses. I'm not sure if that'll be sufficient - needs testing.

#7 follow-up: @Funkatronic
10 years ago

My idea to change parse_orderby:

  • Get all aliases, both custom and ones created by WP_Meta_query, in query
  • Check if orderby matches any of the aliases
  • If it matches, create an ORDER BY clause that uses that alias: alias.meta_value

You don't have to know what the related join clauses contain, just as long as you know the alias thats being used for the join, you can use it for the ORDER BY, at least imho.

As far as I can tell, table_aliases keeps track of all the aliases used, though like you said, we need to test to make sure we cover all bases. Also, does WP have any sanitation functions for table aliases?

#8 in reply to: ↑ 7 @boonebgorges
10 years ago

Replying to Funkatronic:

My idea to change parse_orderby:

  • Get all aliases, both custom and ones created by WP_Meta_query, in query
  • Check if orderby matches any of the aliases
  • If it matches, create an ORDER BY clause that uses that alias: alias.meta_value

You don't have to know what the related join clauses contain, just as long as you know the alias thats being used for the join, you can use it for the ORDER BY, at least imho.

This all sounds good to me. You're probably right about not needing to know what the alias is associated with. I'd need to see the unit tests :)

As far as I can tell, table_aliases keeps track of all the aliases used, though like you said, we need to test to make sure we cover all bases. Also, does WP have any sanitation functions for table aliases?

Use sanitize_key(). It's not specific to table aliases or even to MySQL, but it's the most restrictive of our sanitizers, and it's what register_post_type() uses for post type strings, etc.

#9 @Funkatronic
10 years ago

And as I write the code, I realize you would need to know if the meta_query is typed so you can cast the orderby. Coding wouldn't be coding without challenges.

#10 @Funkatronic
10 years ago

New crazy idea that I think might work:

  • Every meta query clause gets aliased, even the main one. Any orderby call to meta_value will default to the first one.
  • let WP_Meta_Query create the ORDER BY clauses and store them in an array where the aliases are the keys
  • parse_orderby looks for aliases; if it finds one, it pulls the right clause from the meta_query object. If it finds meta_value or meta_value_num, it calls the first/main clause.

It'll take more rewriting than Id like but I think its a sound way to go forward. It'll also take care of casting the orderby properly. Also, lots of testing. Does this sound like a good idea?

#11 @boonebgorges
10 years ago

Every meta query clause gets aliased, even the main one. Any orderby call to meta_value will default to the first one.

Yes. We pretty much already do this; see https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/meta.php#L1353. It makes sense to me to then fill in the 'alias' key for each clause with the calculated value.

let WP_Meta_Query create the ORDER BY clauses and store them in an array where the aliases are the keys

I hesitated at first, but on reflection I think it's a good idea for WP_Meta_Query to do some of this work. That way, it'll be possible to do the same orderby magic in other places where meta queries are used (WP_Comment_Query, WP_User_Query). However, I'd rather not attempt to build partial SQL statements and pass them around. Here's an alternative thought: WP_Tax_Query builds a flat list of "queried_terms" as it parses the arguments passed to the class. https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/taxonomy.php#L774 What we did something similar in WP_Meta_Query? This flat array would contain aliases (and perhaps even be keyed by alias, though I don't think it matters too much), and could be used in WP_Query to look up the necessary cast for the meta value. I think this is somewhat better than assembling the orderby clauses in WP_Meta_Query itself, for two reasons: (1) it's not great practice to pass around partial SQL chunks between classes (my suggestion would pass a structured array instead, and leave it up to the "client" - WP_Query - to build the SQL), and (2) my suggestion would leave the door open for different kinds of custom query manipulation than just ORDER BY, since all the clause data would be readily available in a flat array.

parse_orderby looks for aliases; if it finds one, it pulls the right clause from the meta_query object. If it finds meta_value or meta_value_num, it calls the first/main clause.

+1

Does this make enough sense to do a proof of concept?

@Funkatronic
10 years ago

New patch

#12 @Funkatronic
10 years ago

Posted a new patch. Didn't notice your last post till just now.

@Funkatronic
10 years ago

Fixed an error in the last patch

#13 @boonebgorges
10 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

Thanks for these patches. I started working on your patch last night, and it wasn't working properly for me - but maybe this was fixed by your version 2 :)

In any case, in the process of playing with table aliases, I started to have second thoughts. The way table aliases are currently built in WP_Meta_Query is like this: the first JOINed table is aliased as $wpdb->postmeta, and subsequent tables as mt1, mt2, etc. That first "unaliased" JOIN is then referenced in a number of places in WP_Query, and my guess is that there are plugins that are depending on it as well. For this reason, I'm hesitant to change table aliases. And, notwithstanding my earlier comments, it's not really necessary for what we want to accomplish here anyway.

31045.diff takes a slightly different tack. Aliases are built in the same way as before. Meta queries now accept a 'name' parameter. The meta query object now has a method get_clauses() which returns a flattened array of query clauses. This array is keyed by 'name', and contains 'cast' values, which are then used by WP_Query to build the ORDER BY clause. See the unit tests for examples.

What do you think?

@boonebgorges
10 years ago

#14 follow-up: @Funkatronic
10 years ago

Looks good. Where can I find the unit test file?

Also, glad we can collaborate on this. I'm learning a bit about team coding.

Last edited 10 years ago by Funkatronic (previous) (diff)

#15 in reply to: ↑ 14 @boonebgorges
10 years ago

Replying to Funkatronic:

Looks good. Where can I find the unit test file?

Also, glad we can collaborate on this. I'm learning a bit about team coding.

Happy to collaborate!

The unit tests are at the bottom of 31045.diff.

#16 @Funkatronic
10 years ago

I meant the class those functions are in. Are they part of the WP download anywhere? I've never used the official unit test files (if there are any)

#17 @boonebgorges
10 years ago

Ah. Here's where the PHPUnit tests live in trun: http://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/ Get a checkout of develop.svn.wordpress.org or develop.git.wordpress.org to see them - they don't come with the regular build download. Check out https://make.wordpress.org/core/handbook/ - especially https://make.wordpress.org/core/handbook/automated-testing/

#18 @Funkatronic
10 years ago

Alright, fixed a bug I made in my second to last patch that you transferred into yours. I haven't run "official" unit tests yet, but I ran a query and output the resulting mySQL code and looks like it checks out.

@Funkatronic
10 years ago

Fixed a lapse in coding judgement by me

@Funkatronic
10 years ago

Added documentation to parse_orderby filter

#19 @Funkatronic
10 years ago

Seems to work, what's next?

#20 @boonebgorges
10 years ago

  • Milestone changed from Future Release to 4.2
  • Owner set to boonebgorges
  • Status changed from new to reviewing

Thanks for your continued work on this, Funkatronic. It's funny - your "lapse in coding judgment" actually worked, because the array_key_exists() check would resolve to true or false as appropriate, meaning that the previous logic actually worked, though of course it was not straightforward and perhaps accidental :)

Regarding the 'parse_orderby' filter: I'm going to remove it, as per our informal policy of not adding filters to SQL fragments unless they're absolutely necessary. Anyway, there is already a filter 'posts_orderby' that does essentially the same thing.

I'm going to do a bit more documentation cleanup and review, and then I think we're good to go with this. Thanks for the ticket and for the help in getting this enhancement ready to go.

#21 @boonebgorges
10 years ago

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

In 31312:

Improve support for ordering WP_Query results by postmeta.

WP_Meta_Query clauses now support a 'name' parameter. When building a
WP_Query object, the value of 'orderby' can reference this 'name', so that
it's possible to order by any clause in a meta_query, not just the first one
(as when using 'orderby=meta_value'). This improvement also makes it possible
to order by multiple meta query clauses (or by any other eligible field plus
a meta query clause), using the array syntax for 'orderby' introduced in [29027].

Props Funkatronic, boonebgorges.
Fixes #31045.

#22 @DrewAPicture
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'd like to take a little more time to consider the implementation here. I really like the concept of the idea, I'm just not sold on the name argument. I think it has the possibility of introducing confusion -- especially since we're effectively "aliasing" a key/value clause so we can reference it later in the orderby argument.

Going from name to referencing it in orderby isn't obvious to me.

#23 @Funkatronic
10 years ago

Maybe id or meta_id? We were using alias before we decided not to mess with the mysql aliasing directly

#24 @DrewAPicture
10 years ago

  • Keywords needs-patch added; has-patch removed

After talking to @boonebgorges in Slack, my best suggestion is that we go with clause_id for the argument name. I share his concerns about using the word "alias" in the argument name because of the implication that it has any effect on the actual alias used when building the query.

The purpose of the discussion is to -- as simply as possible -- draw a line between setting the value in the clause and referencing it in the orderby. By avoiding the use of the word "orderby" in the new argument name, we also don't lock ourselves into that vernacular should we find another good use for this argument in the future.

I'd also recommend updating the docs for the orderby argument in WP_Query.

Last edited 10 years ago by DrewAPicture (previous) (diff)

#25 @Funkatronic
10 years ago

@DrewAPicture: one of my original ideas was to have the id just be the key of that particular clause:

$args = array(
	'post_type'  => 'product',
	'meta_query' => array(
		'post_color' => array(
			'key'     => 'color',
			'value'   => 'blue',
			'compare' => 'NOT LIKE',
		),
	),
        'orderby' => 'post_color'  
);

$query = new WP_Query( $args );

Does that make more sense as a syntax alternative?

#26 @boonebgorges
10 years ago

Just to bring the discussion from Slack back here to the ticket: I think Funkatronic's suggestion of using array indexes as orderby aliases is the most elegant solution to the problem. The main reason why I shied away from it when he suggested it in an earlier patch is because it doesn't seem WordPressy. I can't think of any other API where devs can pass arbitrary array indexes and have them interpreted in a meaningful way. The other consideration was that we do (or at least at one time we did) at times refer to the 0th item in meta_query, a reference that would break if we allowed associative keys. We can fix our own references, but it's possible that some plugins are doing the same thing, which introduces the possibility of breakage (however remote that may be).

All of this being said, I'd be glad to go with the suggestion, as I think it's the most natural solution. So let's do it. We'll need a patch that updates the parsing logic, the unit tests, and the inline docs. We'll also need to make sure that we're not referencing the 0th member of the meta query anywhere - we should be using reset().

#27 @boonebgorges
10 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

31045.2.diff changes the functionality so that the syntax works like Funkatronic suggests here: https://core.trac.wordpress.org/ticket/31045#comment:25

It's a pretty straightforward patch, but I'm struggling with the documentation. I've tried my best to describe the feature in the inline docs, but I'm afraid it's still pretty unclear. It's the kind of thing that's pretty obvious if you can see an example, but is hard to explain without getting jargony. I have a very slight suspicion that my difficulty explaining the feature suggests that maybe it's not the best implementation, but I'd be happy to be shown wrong on this point.

DrewAPicture and Funkatronic, would you mind having a look at the patch to see if the documentation changes are sufficient to explain what's going on?

#28 @boonebgorges
10 years ago

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

In 31340:

Modify `meta_query orderby syntax to use array keys as clause "handles".

The implementation of meta_query orderby introduced in [31312] put clause
identifiers into a 'name' parameter of the clause. For greater clarity, this
changeset updates the syntax to use the associative array key used when
defining meta_query parameters, instead of the 'name' parameter.

Props Funkatronic, DrewAPicture.
Fixes #31045.

#29 @boonebgorges
10 years ago

In 31467:

Improve 'orderby' syntax for WP_Comment_Query.

Since [29027], WP_Query has supported an array of values for the $orderby
parameter, with field names as array keys and ASC/DESC as the array values.
This changeset introduces the same syntax to WP_Comment_Query.

We leverage the new support for multiple ORDER BY clauses to fix a bug that
causes comments to be queried in an indeterminate order when sorting by the
default comment_date_gmt and comments share the same value for
comment_date_gmt. By always including a comment_ID subclause at the end of
the ORDER BY statement, we ensure that comments always have a unique fallback
for sorting.

This changeset also includes improvements paralleling those introduced to
WP_Query in [31312] and [31340], which allow $orderby to accept array keys
from specific $meta_query clauses. This change lets devs sort by multiple
clauses of an associated meta query. See #31045.

Fixes #30478. See #31265.

#30 @vampyrse
9 years ago

Hey there,

Has this feature been scratched or changed for 4.3/4.4?
Seeing a lot of reports of people having problems with it, and I am too. Just wanted to check before I possibly open a new ticket.

#31 @boonebgorges
9 years ago

@vampyrse The feature is in place and should be working properly since 4.2. If you are having reproducible issues, please open a ticket with details.

#32 @boonebgorges
7 years ago

#29447 was marked as a duplicate.

Note: See TracTickets for help on using tickets.