WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6476 closed defect (bug) (fixed)

2.5 Gallery shortcode orderby not working

Reported by: GeoffHarrison Owned by:
Milestone: 2.5.1 Priority: normal
Severity: normal Version: 2.5
Component: General Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (4)

gallery-orderby-desc-r7580.patch (540 bytes) - added by tellyworth 6 years ago.
6476.002.diff (2.1 KB) - added by markjaquith 6 years ago.
gallery-orderby-sanitized-r7585.patch (3.2 KB) - added by tellyworth 6 years ago.
6476.003.diff (2.8 KB) - added by markjaquith 6 years ago.
Latest effort

Download all attachments as: .zip

Change History (26)

comment:1 UncleZeiv6 years ago

  • Milestone changed from 2.7 to 2.5.1

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

comment:2 markjaquith6 years ago

  • Milestone 2.5.1 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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

comment:3 markjaquith6 years ago

  • Milestone set to 2.5.1
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Version set to 2.5

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

comment:4 UncleZeiv6 years ago

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

comment:5 tellyworth6 years ago

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.

comment:6 tellyworth6 years ago

  • Keywords has-patch added

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

comment:7 markjaquith6 years ago

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.

markjaquith6 years ago

comment:8 markjaquith6 years ago

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.

comment:9 tellyworth6 years ago

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

comment:10 Dickie6 years ago

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

comment:11 UncleZeiv6 years ago

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.

comment:12 UncleZeiv6 years ago

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.

comment:13 andy6 years ago

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.

comment:14 velenux6 years ago

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.

comment:15 tellyworth6 years ago

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

  1. orderby should allow discrete values, not arbitrary column names
  1. SQL is not a user interface
  1. 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.

comment:16 follow-up: Dickie6 years ago

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?

comment:17 in reply to: ↑ 16 ; follow-up: markjaquith6 years ago

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!

comment:18 in reply to: ↑ 17 ; follow-up: Dickie6 years ago

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 !

comment:19 UncleZeiv6 years ago

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?

comment:20 in reply to: ↑ 18 markjaquith6 years ago

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.

markjaquith6 years ago

Latest effort

comment:21 markjaquith6 years ago

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

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

comment:22 markjaquith6 years ago

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

Note: See TracTickets for help on using tickets.