2.5 Gallery shortcode orderby not working

[GeoffHarrison]

The orderby gallery shortcode has no effect on the order of the images in the gallery. The SQL it generates is incorrect. A gallery shortcode [gallery columns="4" orderby="post_name ASC"] generates the SQL query

SELECT DISTINCT * FROM wp_posts  WHERE 1=1 AND post_type = 'attachment'   AND wp_posts.post_parent = 8  AND (post_mime_type LIKE 'image/%')  GROUP BY wp_posts.ID ORDER BY "post_name ASC" DESC

The ORDER BY clause is incorrect. In fact, even a default [gallery] shortcode generates an invalid ORDER BY:

...ORDER BY "menu_order ASC, ID ASC" DESC

It should be unquoted and without the final DESC.

[UncleZeiv]

I'm experiencing a related problem. The generated SQL query ends like this in my case:

ORDER BY \"menu_order ASC, ID ASC\" DESC

thus generating an invalid query. The result is that no gallery shows up in the post. (My opinion: gallery_shortcode should not return in case of failure). I guess this has to do with some wrong server setting, since I seem to be the only person with this problem (running mysql 5.0.32).

I'm afraid I can't upgrade until this gets corrected...

[markjaquith]

Duplicate of #6476 I'm working on a patch right now.

[markjaquith]

Apparently I had this bug open in two windows. It's a duplicate of itself! ;-)

[UncleZeiv]

sorry, had a formatting problem in my previous comment: I was suggesting that gallery_shortcode should not return empty.

[tellyworth]

Looks like that query (with the superfluous DESC) has been that way from the beginning. The strange part is that it works perfectly on some (many?) MySQL versions.

[tellyworth]

The patch removes the trailing DESC. It leaves the quotes in place - are they safe?

[markjaquith]

The quotes kill the orderby clause. I have no idea why they were in there. I'd like an orderby value that has the column and a direction all in one to be supported by get_posts(), rather than the hacky solution of passing a blank order value. We also need to do common sense regex testing to make sure that the orderby is in the proper format for an orderby clause. Patch coming.

[markjaquith]

6476.002.diff does:

1. detection of ASC/DESC in $orderby in get_posts() and ignores $order if present
2. regex screening of orderby clause passed through the gallery shortcode
3. removes the quotes

[gallery orderby="RAND()"] is kinda fun.

[tellyworth]

I'd suggest putting the preg block in a separate function for unit testing and reuse.

[Dickie]

Please see this ticket ​http://trac.wordpress.org/ticket/6508
It is a duplicate (which I raised as I did not see this one), but my patch (which has the same effect as markjaquith's (as far as I can see) does not work on some setups.
Seems this needs care...

[UncleZeiv]

actually, I wouldn't say this is a duplicate of ​http://trac.wordpress.org/ticket/6508 Here, SQL syntax is being accepted by the server, but is conceptually wrong in the end. In 6508, a SQL query that gets accepted all over the world, results in a syntax error; 6508 is probably not a Wordpress bug but a configuration issue, still I think it should be investigated further.

[UncleZeiv]

Applying 6476.002.diff I get:
Fatal error: Cannot unset string offsets in [...]/wp-includes/media.php on line 346

Please note that I'm passing a plain shortcode: [gallery].

Removing that line and the preceding if, the gallery shows up as expected.

[andy]

Don't remove it. Make it safe. Wrap it:

if ( isset($attr['orderby']) ) {
	preg_match('/(^([a-z0-9_]+( +(ASC|DESC))?(, +?|$))+|RAND\(\))/i', $attr['orderby'], $obmatches);
	if ( !$obmatches[0] )
		unset($attr['orderby']);
}

Tested with gallery shortcodes with and without orderby, all good now.

[velenux]

I managed to track back why the query works on some systems and doesn't work on some others. I think it's related to the magic_quotes setting in php.ini; if you have magic_quotes active, mysql gets the following query:

SELECT DISTINCT * FROM wp_posts  WHERE 1=1 AND post_type = 'attachment'   AND wp_posts.post_parent = 5  AND (post_mime_type LIKE 'image/%')  GROUP BY wp_posts.ID ORDER BY \"menu_order ASC, ID ASC\" DESC

The \" of course renders the query invalid.

[tellyworth]

allery-orderby-sanitized-r7585 improves on Mark's patch. Requirements were:

1. orderby should allow discrete values, not arbitrary column names

2. SQL is not a user interface

3. the same syntax and sanitizing should be reusable and consistent elsewhere (it could be useful for widgets or other shortcodes)

It supports syntax like this:

[gallery orderby="id"]
[gallery orderby="menu_order, id"]
[gallery orderby="-menu_order, +name"]
[gallery orderby="random"]

'+' means ASC (the default), '-' means DESC. Accepted values are:

		'id' => 'ID',
		'menu_order' => 'menu_order',
		'name' => 'post_name',
		'date' => 'post_date',
		'title' => 'post_title',
		'caption' => 'post_excerpt',
		'random' => 'rand()',

That last one is just for Mark.

Unit tests are in TestSanitizeOrderby:

​http://svn.automattic.com/wordpress-tests/wp-testcase/test_includes_formatting.php

The sanitize_orderby() function can be reused elsewhere to provide the same syntax.

[Dickie]

What about backwards compatibility for those who have already started using
[gallery orderby="<something> ASC, <also> DESC"] in 2.5 ?

Maybe it's not that many?

[markjaquith]

I'd use "rand" and "ID" for consistency with orderby code in query.php We can go through on another ticket and make similar code test a lowercase version of the orderby param and test both "rand" and "random"

I also wouldn't call it sanitize_orderby() -- it's not sanitization as much as it is translation from one format to another. {{convert_human_orderby_to_sql()}}} or something.

Replying to Dickie:

What about backwards compatibility for those who have already started using
[gallery orderby="<something> ASC, <also> DESC"] in 2.5 ?

Maybe it's not that many?

orderby doesn't work currently, so it's not like we'd be breaking it for them!

[Dickie]

Replying to markjaquith:

Replying to Dickie:

What about backwards compatibility for those who have already started using
[gallery orderby="<something> ASC, <also> DESC"] in 2.5 ?

Maybe it's not that many?

orderby doesn't work currently, so it's not like we'd be breaking it for them!

OK, but as far as I can see the orderby works fine (if the gallery works at all that is), If I change the orderby on my WAMP setup it changes the order nicely, (and yes RAND() is fun !!).

Many people are saying the gallery is working fine, it's just a few of us will odd setups ( I think) and that is why I raised my initial ticket because my gallery function did not work at all on my hosting account. See #6508

There may be many people for whom the orderby is working just fine !

[UncleZeiv]

While we are at it: it doesn't seem to be correct to me that adjacent_image_link() doesn't honour the orderby option given in the gallery shortcode. I know that the option is not saved anywhere in the database, and that different gallery shortcodes with different options could refer to the same images, still it feels very awkward to see the pictures in a particular order in the post and not being able to navigate through them in the same order.

Another way to put it is: where's the UI to change menu_order?

[markjaquith]

Replying to Dickie:

OK, but as far as I can see the orderby works fine (if the gallery works at all that is), If I change the orderby on my WAMP setup it changes the order nicely, (and yes RAND() is fun !!).

Many people are saying the gallery is working fine, it's just a few of us will odd setups ( I think) and that is why I raised my initial ticket because my gallery function did not work at all on my hosting account. See #6508

Hm, well if a quoted ORDER BY clause is working for some SQL setups, then we shouldn't break it. On my SQL setup, it doesn't work:

mysql> select ID, post_name from wptrunk_posts ORDER BY "post_name DESC" limit 5;
+----+----------------------+
| ID | post_name            |
+----+----------------------+
| 32 | testing-123rwefwef   | 
| 24 | tag-post             | 
|  2 | about                | 
| 18 | another-barcamp-post | 
| 17 | at-barcamporlando    | 
+----+----------------------+
5 rows in set (0.00 sec)

mysql> select ID, post_name from wptrunk_posts ORDER BY "post_name ASC" limit 5;
+----+----------------------+
| ID | post_name            |
+----+----------------------+
| 32 | testing-123rwefwef   | 
| 24 | tag-post             | 
|  2 | about                | 
| 18 | another-barcamp-post | 
| 17 | at-barcamporlando    | 
+----+----------------------+
5 rows in set (0.00 sec)

mysql> select ID, post_name from wptrunk_posts limit 5;
+----+----------------------+
| ID | post_name            |
+----+----------------------+
| 32 | testing-123rwefwef   | 
| 24 | tag-post             | 
|  2 | about                | 
| 18 | another-barcamp-post | 
| 17 | at-barcamporlando    | 
+----+----------------------+
5 rows in set (0.00 sec)

As you can see, both ASC and DESC quoted ORDER BY clauses are the same as having no ORDER BY clause.

[markjaquith]

Latest effort

[markjaquith]

(In [7592]) Fix gallery shortcode orderby param for all SQL setups. Sanitize orderby. fixes #6476 for trunk

[markjaquith]

(In [7593]) Fix gallery shortcode orderby param for all SQL setups. Sanitize orderby. fixes #6476 for 2.5.1

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