get_pages() with child_of forgets sort

[janfabry]

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

[jessepollak]

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!

[nacin]

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.

[jessepollak]

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

[nacin]

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.

[jessepollak]

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.

[jessepollak]

17374.patch

[chriscct7]

Some formatting issues and needs testing

[DrewAPicture]

I've been doing some work on get_pages() elsewhere, I'll take a look.