WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 10 months ago

#23873 closed defect (bug) (fixed)

Gallery shortcode orderby does not support multiple column sorting

Reported by: dglingren Owned by: wonderboymusic
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.5.1
Component: Gallery Keywords: has-patch needs-testing
Focuses: Cc:

Description

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.

Attachments (4)

ticket_23873_patch001.diff (4.5 KB) - added by madasha 11 months ago.
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.
ticket_23873_patch002.diff (4.6 KB) - added by madasha 11 months ago.
Second version of the patch, some couple of bugs fixed (after unit-testing), some method scopes changed.
orderby.php (7.6 KB) - added by madasha 11 months ago.
The unit-tests file, should reside in the tests/phpunit/tests/query/ dir.
23873.diff (1.4 KB) - added by wonderboymusic 10 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 @dglingren2 years ago

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.

comment:2 @dglingren2 years ago

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.

comment:3 @SergeyBiryukov2 years ago

  • Component changed from Shortcodes to Gallery

comment:5 @dglingren12 months ago

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.

comment:6 @wonderboymusic12 months ago

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

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

comment:7 @tyxla11 months ago

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.

comment:8 @dglingren11 months ago

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.

@madasha11 months ago

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.

comment:9 @madasha11 months ago

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

@madasha11 months ago

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

@madasha11 months ago

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

comment:10 @madasha11 months ago

  • Keywords needs-testing added; needs-unit-tests removed

@wonderboymusic10 months ago

comment:11 @wonderboymusic10 months ago

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.

comment:12 @wonderboymusic10 months ago

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

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.

Note: See TracTickets for help on using tickets.