WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 4 months ago

#17374 assigned defect (bug)

get_pages() with child_of forgets sort

Reported by: janfabry Owned by: DrewAPicture
Milestone: Priority: normal
Severity: normal Version: 3.1.2
Component: Posts, Post Types Keywords: dev-feedback needs-testing needs-refresh
Focuses: Cc:
PR Number:

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 6 years ago.
17374.patch

Download all attachments as: .zip

Change History (10)

#1 @nacin
6 years ago

  • Component changed from General to Post Types

#2 @jessepollak
6 years 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 6 years ago by jessepollak (previous) (diff)

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


6 years ago

#4 @nacin
6 years 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.

#5 follow-up: @jessepollak
6 years ago

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

#6 in reply to: ↑ 5 @nacin
6 years 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.

#7 @jessepollak
6 years 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.

@jessepollak
6 years ago

17374.patch

#8 @chriscct7
4 years ago

  • Keywords needs-testing needs-refresh added; has-patch removed

Some formatting issues and needs testing

#9 @DrewAPicture
4 years ago

  • Owner set to DrewAPicture
  • Status changed from new to assigned

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

Note: See TracTickets for help on using tickets.