Opened 15 years ago
Last modified 4 years ago
#14477 reopened enhancement
get_pages with child_of only works with uninterrupted hierarchies
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | high |
Severity: | normal | Version: | 3.0 |
Component: | Query | Keywords: | has-patch |
Focuses: | Cc: |
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)
Change History (35)
#1
@
14 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
#2
@
12 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Future Release to 3.7
#4
@
11 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
@
10 years ago
- Milestone changed from Future Release to 4.1
Let's look at this again, patch is refreshed.
#8
@
10 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
@
10 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.
10 years ago
#11
@
10 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.
10 years ago
#13
@
10 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
@
10 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from reopened to closed
In 30636:
#15
@
10 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.
10 years ago
#17
@
10 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 asBB, AAA
, notAAA, 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
@
10 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.
#19
@
10 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.
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 intoWP_Post
instances (beforeget_page_children()
is called). With a properWP_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()