WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#30480 closed defect (bug) (fixed)

wp_get_archives() should support a secondary ORDER BY column

Reported by: boonebgorges Owned by: boone
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: General Keywords: 4.2-early has-patch
Focuses: Cc:

Description (last modified by boonebgorges)

See #30478 for a similar issue in WP_Comment_Query.

wp_get_archives() tests are periodically failing on older versions of MySQL due to indeterminate ordering of results when all posts have the same post_date_gmt. See https://travis-ci.org/pento/develop.wordpress/jobs/41917241. The ORDER BY clause built by wp_get_archives() should look something like: ORDER BY post_date $order, ID $order.

Attachments (2)

30480.patch (3.0 KB) - added by herbmillerjr 6 years ago.
30480.2.patch (671 bytes) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (9)

#1 @herbmillerjr
6 years ago

  • Keywords has-patch added; needs-patch removed

I think I got them all, if I'm understanding you correctly. If this is possibly a "good-first-unit-test," too, could someone walk me through writing one or point me to documentation? I had no idea of the concept until yesterday, and I'm totally new to this patch contributing stuff so a measure of hand-holding is appreciated.

@herbmillerjr
6 years ago

#2 @boonebgorges
6 years ago

  • Description modified (diff)

#3 @boonebgorges
6 years ago

  • Keywords 4.2-early added; needs-unit-tests good-first-bug removed

Hi herbmillerjr - Thanks for the patch.

I spent a bit more time looking at this issue, and while the fix is very simple, testing it (and writing unit tests for it) is not simple at all. The bug *only* seems to occur when all of the following conditions hold (and all of which are demonstrated in the failing test at https://travis-ci.org/pento/develop.wordpress/jobs/41917241):

  • using 'type=postbypost'
  • using a LIMIT clause
  • running MySQL 5.1.x

There seems to be something in this series of MySQL that causes the LIMIT clause to trigger a different set of rules for breaking the 'post_date' tie than all other versions of MySQL than I tested - in 5.1.x, 'post_date DESC' implies 'post_date DESC, ID ASC'.

So, the fix that I've put in 30480.2.patch fixes the problem. The clauses that you added in 30480.patch don't actually make a difference in the case where there are ties with 'post_date', since the results are grouped by month/year/day etc anyway.

My apologies for the misleading description that I gave above, where I suggested that it was necessary to use $order dynamically - since this only arises in the case of 'postbypost', which hardcodes 'post_date DESC' (ignoring $order), we can hardcode the secondary ORDER BY clause there too. And I don't think another unit test here would help us - the existing (failing) test_wp_get_archives_limit() is pretty much a duplicate of the test I'd have to write.

Thanks again for the patch. If you come across another ticket with a needs-unit-tests flag, feel free to ping me and I'll be happy to point you in the right direction :)

#4 @iseulde
5 years ago

  • Milestone changed from Future Release to 4.2

has-patch 4.2-early so moving to 4.2.

This ticket was mentioned in Slack in #core by drew. View the logs.


5 years ago

#6 @DrewAPicture
5 years ago

  • Owner set to boone
  • Status changed from new to reviewing

#7 @boonebgorges
5 years ago

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

In 31452:

Provide a secondary sort order for wp_get_archives() when type=postbypost.

Sorting by post_date alone can cause indeterminacy problems on different
versions of MySQL when post_date ties need to be broken. Using ID as a
secondary sort ensures that the order is always determinate.

Props herbmillerjr for an initial patch.
Fixes #30480.

Note: See TracTickets for help on using tickets.