WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 weeks ago

#17374 new defect (bug)

get_pages() with child_of forgets sort

Reported by: janfabry Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1.2
Component: Posts, Post Types Keywords: has-patch dev-feedback
Focuses: Cc:

Description

If you call get_pages() with both the child_of and sort_column, the sorting is not applied.

child_of makes it select all pages (sorted) and later applies a subselect via get_page_children(). This subselect can mess up the sort order.

An example was reported on http://wordpress.stackexchange.com/questions/16921/get-pages-not-ordering-as-it-should

Related: #12821

Attachments (1)

17374.patch (3.0 KB) - added by jessepollak 8 weeks ago.
17374.patch

Download all attachments as: .zip

Change History (8)

comment:1 nacin3 months ago

  • Component changed from General to Post Types

comment:2 jessepollak8 weeks ago

  • Keywords needs-patch added

Thanks for the bug report janfabry, sorry you never got a response!

I just confirmed that this bug is still around in trunk at revision 27076.

To recreate:

  1. Create a new page Page 1.
  2. Give Page 1 two subpages Page 1a and Page 1b where Page 1b is created after Page 1a.
  3. Give Page 1a a sub page Page 1aa.
  4. Run
$pages = get_pages(array('child_of' => PAGE_1_ID, 
    'sort_column' => 'post_date', 
    'sort_order' => 'DESC'
));

Expected:

Get Page 1a, Page 1b, Page 1aa in that order (because that's the order they were created in).

Actual

The sub pages are ordered Page 1a, Page 1aa, Page 1b.

Why

This happens because, as janfabry points out, the sort happens when the all of the pages are gotten. To get the children, we go through all the sorted pages and select the children and array_merge them into the page list. If a sub-page has a sub-page, this gets added right behind the sub-page, potentially messing up the ordering.

There are two possible ways to patch this:

  1. When we add each child, we add it in the correct place (more performant, a little trickier).
  2. After we add all the children we sort the list again (less performant, more tricky).

I'd be happy to write up a patch with either of these options.

Thanks again janfabry!

Last edited 8 weeks ago by jessepollak (previous) (diff)

comment:3 ircbot8 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by jessepollak. View the logs.

comment:4 nacin8 weeks ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

This would also strongly benefit from a unit test. It would go in here: https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/post/getPages.php.

comment:5 follow-up: jessepollak8 weeks ago

I'll write this patch and unit test later today, any preference on whether I do the sort during merge or post merge?

comment:6 in reply to: ↑ 5 nacin8 weeks ago

Replying to jessepollak:

I'll write this patch and unit test later today, any preference on whether I do the sort during merge or post merge?

I don't; I really have no idea what they'll look like.

comment:7 jessepollak8 weeks ago

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

OK, so it turned out that neither of my ideas were good ones. Both involved reimplementing parts of the MySQL search, which seemed like Bad Idea.

Instead, I decided that I'd rewrite the get_page_children. In the current version, we do a top down recursive search to find all the children of child_of. The problem with this is that we get all the children at the same time, so they all get added to the new list at the same time — this messes up the sort that we had previously.

In this patch, I reverse this so we do a bottom up search with a while loop to check whether a page has an ancestor with the ID child_of. If it does, we add it to the new list of pages to return. This maintains the sorted order because we're iterating through in sorted order and adding one page at a time. To make this search faster, we add all the pages to a dictionary so we can do O(1) lookup when traversing up the tree.

I've also written a unit test that verifies the failed functionality and confirms the patch. I can write more, if we want.

jessepollak8 weeks ago

17374.patch

Note: See TracTickets for help on using tickets.