WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 18 months ago

#14477 reopened enhancement

get_pages with child_of only works with uninterrupted hierarchies

Reported by: vividvisions Owned by: wonderboymusic
Milestone: Future Release Priority: high
Severity: normal Version: 3.0
Component: Query Keywords: has-patch
Focuses: Cc:
PR Number:

Description

I have a page X with several children and grandchildren. Some of the grandchildren have a certain meta key and value. I now want to fetch all pages under page X that have this meta key. After reading the documentation of get_pages, I expected this to work with a single call of get_pages.

But if I use get_pages like this:

get_pages('child_of=X&meta_key=A&meta_value=B');

it returns no pages at all (hierarchical=0 has no effect). It seems that's because child_of triggers a call of get_page_children which only returns uninterrupted hierarchies. Since the query returns only some grandchildren and no direct descendants, the function fails. IMHO, that's a bug.

Attachments (10)

14477.diff (1.1 KB) - added by wonderboymusic 6 years ago.
14477.2.diff (2.1 KB) - added by wonderboymusic 6 years ago.
14477.3.diff (2.0 KB) - added by wonderboymusic 6 years ago.
14477.4.diff (2.0 KB) - added by wonderboymusic 5 years ago.
14477-41-broken-ut.diff (2.7 KB) - added by dd32 5 years ago.
14477.5.diff (1.3 KB) - added by boonebgorges 5 years ago.
14477-41-broken-ut.2.diff (5.3 KB) - added by dd32 5 years ago.
14477-41-broken-ut.3.diff (6.2 KB) - added by dd32 5 years ago.
14477.6.diff (2.6 KB) - added by boonebgorges 5 years ago.
Revert to the way things were in 4.0
14477.7.diff (2.6 KB) - added by eclev91 2 years ago.

Download all attachments as: .zip

Change History (34)

#1 @nacin
9 years ago

  • Keywords needs-patch added; cms get_pages get_page_children removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

@wonderboymusic
6 years ago

#2 @wonderboymusic
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 3.7

Grandchildren can only be accessed if we know the full list of ancestors for the page being checked, 14477.diff addresses this. The list of pages is mapped earlier to get_post to turn them into WP_Post instances (before get_page_children() is called). With a proper WP_Post instance, we can check $page->post_parent == $page_id || in_array( $page_id, $page->ancestors )

All of these calls are highly-cacheable, but I don't know what everyone's feelings are on the performance of get_post_ancestors()

#3 @wonderboymusic
6 years ago

  • Keywords commit added

.2.diff has unit tests

#4 @nacin
6 years ago

  • Milestone changed from 3.7 to Future Release

Because this is distinctly an enhancement and 3.7 will have a very short beta/RC period, punting to 3.8. I am fine with this being committed at the start of the cycle if we can agree get_post_ancestors() is a sensible hit here. I don't actually know if it is. Should we only try checking ancestors if someone is filtering by meta key? Is it more likely than not that the ancestor posts are already in cache, thus avoiding any kind of ancestor queries? The answers to these questions are not difficult, but they're not obvious.

There was also a discussion in IRC with markjaquith about this, and it involved possibly adding a new parameter like descendant_of. Might not be worth it for the purposes of maintaining compatibility, but could be worth it for the purposes of performance. (Unless we agree that since this can only happen if someone is filtering by meta key, thus it doesn't actually matter as they're doing it to themselves.)

#5 @wonderboymusic
5 years ago

  • Milestone changed from Future Release to 4.1

Let's look at this again, patch is refreshed.

#6 @wonderboymusic
5 years ago

Walker_Page already does this - gonna drop it in.

#7 @wonderboymusic
5 years ago

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

oops, fixed in [30159].

#8 @boonebgorges
5 years ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[30159] breaks Tests_Post_Template::test_wp_dropdown_pages().

This appears to be a symptom of a broader problem. [30159] makes it so that page ancestors are fetched in all cases. But then page children are fetched as well with get_page_children(). This results in potential duplicates in the results of get_pages(). I haven't looked into it deeply, but it may be enough to do duplicate detection after fetching the results (though this may break pagination/number).

#9 @wonderboymusic
5 years ago

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

Whoops - meant to tag this one: [30246]

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


5 years ago

#11 @dd32
5 years ago

  • Priority changed from normal to high
  • Resolution fixed deleted
  • Status changed from closed to reopened

This change has broken get_pages() with child_of, it now returns duplicates, as I suspect it's pulling from the ancestors for siblings or something.

14477-41-broken-ut.diff contains a unit test that shows the expected output, and the output I'm seeing on 4.1.

This has a knock on effect to anyone using wp_list_pages() and child_of as suddenly they'll get pages showing up at each level duplicated.

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


5 years ago

@boonebgorges
5 years ago

#13 @boonebgorges
5 years ago

  • Keywords has-patch 2nd-opinion added

14477.5.diff rolls back [30246] and implements by suggestions from https://core.trac.wordpress.org/ticket/14477#comment:8, which is to do some simple deduplication before returning the results from get_page_children(). This fixes all unit tests, including dd32's 14477-41-broken-ut.diff.

There may be a more efficient way to dedupe than what I've proposed here. Suggestions welcome.

#14 @wonderboymusic
5 years ago

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

In 30636:

Ensure uniqueness when returning page lists in get_page_children(). Fixes failing unit tests.

Props boonebgorges.
Reverts [30246].
Fixes #14477.

#15 @dd32
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

14477-41-broken-ut.2.diff shows yet-another change in behaviour, which has a knock-on effect.

get_page_children() is now returning in alphabetical order, rather than hierarchical order.
When used in conjunction with wp_list_pages() and child_of > 0, and a sub-page with a page_title which is ordered before the child_of, it causes wp_list_pages() to not be able to list the page hierarchy correctly.

IMHO, this actually shows a bug in the Walker_Page. The Walker_Page, well, really Walker::walk calculates $top_level_elements as being post_parent = 0, and if none are found, then post_parent = first post_parent found, this only works because it relies upon get_pages() (which relies on get_page_children()) to return in a hierarchical order.

get_pages() by default returns posts in Alphabetical order, the catch is that this is respected amongst the hierarchy, so BB => AAA should be returned as BB, AAA, not AAA, BB.

The patch contains more explanation and examples..

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


5 years ago

#17 @dd32
5 years ago

get_pages() by default returns posts in Alphabetical order, the catch is that this is respected amongst the hierarchy, so BB => AAA should be returned as BB, AAA, not AAA, BB.

For completeness, and to say that the bug doesn't lie entirely within the Walker_Page, 14477-41-broken-ut.3.diff also shows that the hierarchical parameter (which is false in wp_list_pages(), go figure) is now broken.

#18 @boonebgorges
5 years ago

  • Keywords revert added; has-patch removed

Le groan. Thanks for looking into this, dd32.

The change in order is due to the $post->ancestors trick introduced by wonderboymusic in [30159]. The problem there is that get_post_ancestors() returns a flat list. For any item in that list, we have no way of knowing where in the tree it lives: whether it's a direct parent (post_parent), a grandparent, or further up the tree. When looping through the $pages passed to get_page_children(), then, the in_array( $page_id, $page->ancestors ) is just too coarse.

I don't think it's possible to salvage this. For 4.1, I recommend reverting the changes in get_pages() and get_page_children() back to what they were in 4.0. For 4.2 and beyond, we can look into building a properly tree recurser - something like get_page_children() (maybe get_page_descendants()?), but which *would* perform SQL queries to get each level of child, so that we'd get an exhaustive and unique descendant tree, which would solve the original bug reported in this ticket.

@boonebgorges
5 years ago

Revert to the way things were in 4.0

#19 @dd32
5 years ago

Thanks for looking into this, dd32.

I'm not going to lie, I spent lots of hours trying to work out why wp_list_pages() was breaking :)

I don't think it's possible to salvage this. For 4.1, I recommend reverting the changes in get_pages() and get_page_children() back to what they were in 4.0.

Yep, I wasn't going to suggest that straight up as I was hoping someone would go "Oh, we just have to do x!", but it's quite likely that's not possible, and a revert is in order.

For 4.2 and beyond, we can look into building a properly tree recurser - something like get_page_children() (maybe get_page_descendants()?), but which *would* perform SQL queries to get each level of child,

Sounds like a good idea. Pages, especially when there's lots of them (with multiple depths) has some serious performance issues, so looking at writing something like that could be a big benefit.

#20 @wonderboymusic
5 years ago

In 30735:

Give up on making uninterrupted hierarchies work in get_page_children() for now, reverts [30159], [30246], [30636].

Props boonebgorges.
See #14477.

#21 @wonderboymusic
5 years ago

  • Milestone changed from 4.1 to Future Release

#22 @wonderboymusic
5 years ago

  • Keywords needs-patch added; revert 2nd-opinion removed

#23 @eclev91
2 years ago

Looking at this. Currently breaking the test_get_pages_cache test because, as @boonebgorges mentioned, it'll probably have to run SQL queries to find children. I'll commit the patch for review.

@eclev91
2 years ago

#24 @sebastien@…
18 months ago

  • Keywords has-patch added; needs-patch removed
Note: See TracTickets for help on using tickets.