Gallery shortcode orderby does not support multiple column sorting

[dglingren]

In wp-includes/media.php, function gallery_shortcode, lines 689-694 are:

// We're trusting author input, so let's at least make sure it looks like a valid orderby statement
if ( isset( $attr['orderby'] ) ) {
	$attr['orderby'] = sanitize_sql_orderby( $attr['orderby'] );
	if ( !$attr['orderby'] )
		unset( $attr['orderby'] );
}

The sanitize_sql_orderby function requires commas between multiple column names. If you enter, for example orderby="menu_order ID", the lack of a comma causes the parameter to be deleted from the query.

Just after this test, on line 698, a multi-column sort without the comma delimiter is used as the default value.

If you enter commas to get past the sanitize_sql_orderby test, logic in wp-includes/query.php, function get_posts will not accept the commas (lines 2358 - 2361) and all but the last of the columns are ignored.

This issue should be considered along with those raised in #17065 and #16584. Thank you.

[dglingren]

If you pursue this issue, consider a closer look at "sanitize_sql_orderby".

Unlike the other WordPress "sanitize" functions, it doesn't actually modify the parameter passed to it. It tests the incoming value and, if it passes, returns the incoming value unchanged.

Also, the testing has some issues. For example, it accepts "orderby=ASC" as a valid ORDER BY clause. The [gallery] shortcode will discard this value, but other plugins that rely on this function might have more serious issues.

[dglingren]

In the WordPress.com Support section, there's a multi-column sorted [gallery] example:

[gallery columns="5" orderby="title DESC, ID ASC"]

​http://en.support.wordpress.com/images/gallery/#adding-a-gallery-or-slideshow

It's down in the Gallery Shortcode portion of the page. The example does not work, but it shows the preferred syntax for a corrected implementation.

[kovshenin]

Related: #17065

[dglingren]

The new, "more powerful ORDER BY in WordPress 4.0" is a good reason to revisit this ticket. It looks like nothing has changed in the Gallery shortcode or the "sanitize_sql_orderby" function since this ticket was started back in version 3.5.1.

[wonderboymusic]

sanitize_sql_orderby() needs a reboot, or we need to use something else in its place. It has not kept up with the times.

[tyxla]

After testing for a while I discovered that this check is not necessary anymore.

The gallery shortcode will use get_children(), which uses get_posts(), which already does the necessary checks to filter out any incorrect orderby statements. In case anything unexpected is used in the orderby attribute, get_posts() will order by the post_date column.

So the following check can be removed:

// We're trusting author input, so let's at least make sure it looks like a valid orderby statement
if ( isset( $attr['orderby'] ) ) {
	$attr['orderby'] = sanitize_sql_orderby( $attr['orderby'] );
	if ( ! $attr['orderby'] ) {
		unset( $attr['orderby'] );
	}
}

This way multi-column sorting can be used in the shortcode call, and even the new powerful orderby options can be used in the post_gallery and shortcode_atts_gallery filters.

[dglingren]

Thanks, @tyxla, for your update. I removed the check and did a bit of testing. Without the check, you can specify multiple fields, e.g., orderby="author date title".

Since the shortcode parameters are passed as strings, the one thing you can't do is specify column-by-column ASC/DESC values.

It seems like a bit of enhancement to the /wp-includes/query.php function parse_orderby() would be able to support syntax like orderby="author date DESC title", applying ASC or DESC to the column name that immediately precedes it. I can't think of a drawback to this addition offhand.

[madasha]

Here is a patch implementing what dglingren last suggested - the ordeaby query param can now accept a string like "author date DESC title" or even "author ASC date DESC title" or whatever combination of valid fields optionally followed by order directions. I hope to upload some unit tests soon, asserting proper functionality.

[madasha]

Second version of the patch, some couple of bugs fixed (after unit-testing), some method scopes changed.

[madasha]

The unit-tests file, should reside in the tests/phpunit/tests/query/ dir.

[wonderboymusic]

Because these values will eventually be passed to WP_Query, only valid values will make it through ->parse_orderby(), so I think this strict check is unnecessary here.

The only place sanitize_sql_orderby() is used is in the gallery shortcode - it's used in the playlist shortcode too, but I only added it there because it was in gallery.

[wonderboymusic]

In 30068:

The gallery shortcode used to accept a SQL chunk for the value of the orderby attribute. The reason? get_posts() used to be called in the shortcode handler with a query-string blob of arguments passed to it. To mitigate breakage, sanitize_sql_orderby() was created in [7592].

sanitize_sql_orderby() expects a comma to be present when multiple orderby values were passed. The correct syntax for multiple fields is space-delimited. Since [29027], comma-separated values would never be parsed correctly when passed to WP_Query->parse_orderby().

sanitize_sql_orderby() is used nowhere else in core, save for the playlist shortcode - I only added it there because I was mimic'ing the gallery logic. The function call can be removed from both shortcode handlers.

See #6476.
Fixes #23873.