[wp-trac] [WordPress Trac] #15667: wp_list_pages, if it finds no pages to display, shows random child pages instead because of a bug in get_pages()

[WordPress Trac]

#15667: wp_list_pages, if it finds no pages to display, shows random child pages
instead because of a bug in get_pages()
-------------------------------------------------+-------------------------
 Reporter:  bobsoap                              |       Owner:
     Type:  defect (bug)                         |  jackreichert
 Priority:  normal                               |      Status:  assigned
Component:  Query                                |   Milestone:  3.9
 Severity:  minor                                |     Version:  3.0.2
 Keywords:  good-first-bug needs-unit-tests      |  Resolution:
  has-patch 2nd-opinion                          |     Focuses:
-------------------------------------------------+-------------------------
Changes (by willmot):

 * keywords:  good-first-bug needs-unit-tests has-patch => good-first-bug
     needs-unit-tests has-patch 2nd-opinion


Comment:

 Been digging into this, it's not as simple as it first seems. First order
 of business,
 [https://core.trac.wordpress.org/attachment/ticket/15667/15667
 .wp_list_pages-unit-tests.diff 15667.wp_list_pages-unit-tests.diff]
 includes a couple of unit tests to prove this issue.

 > The root of the problem is get_pages(), since the Walker's behavior when
 fed bad data like this should probably be left undefined.

 I actually don't think that's the case, `get_pages()` will return the
 children of any excluded parent pages as long as `hierarchical` is set to
 `false` (as it is in `wp_list_pages`),
 [https://core.trac.wordpress.org/attachment/ticket/15667/15667.get_pages-
 unit-tests.diff 15667.get_pages-unit-tests.diff] adds some unit tests to
 prove that. I think that behaviour makes sense and isn't something that
 should be changed.

 > What's causing this is a condition in the walk method in class-wp-
 walker.php.

 I'm also not convinced we need to fix this in the Walker, although I do
 think that separate issue merits some thought.

 Currently the `Walker` attempts to handle the situation where it has been
 passed a list of elements with no top level elements by just taking the
 first element and treating that as root. This seems like a pretty strange
 assumption. It would seem more logical to either always omit elements
 which have a missing parent or to treat all elements with missing parents
 as being top level. Obviously doing the former would resolve this issue
 for `wp_list_pages` but might not actually be the desired behaviour for
 the `Walkers`. Currently if there is both a valid top-level element and an
 orphaned child then that is treated as top level when `depth=0` but is
 omitted when `depth=1`.

 > This assumption should fine, except when the depth is limited to 1

 I don't necessarily agree, `depth=1` should probably mean the first level
 of the elements that are passed in, not just elements which have parent
 set to `0`.

 I've added some unit tests for the base Walker class in
 [https://core.trac.wordpress.org/attachment/ticket/15667/15667.walker-
 unit-tests.diff 15667.walker-unit-tests.diff] which assert the current
 functionality.

 > Now we now add the "exclude_tree" attr just for fun:
 > 2) wp_list_pages('title_li=&depth=1&exclude=3,5,7&exclude_tree=3,5,7');
 > => should again return nothing, but instead, it displays the first-ever
 published child page globally (here: a child page of 5).

 Actually, `exclude_tree` works as expected, it's just that it only accepts
 a single `(int)` not a comma separated list so in your example above you
 were only actually excluding children of`3`.

 > adds condition to walker class to prevent random child

 [https://core.trac.wordpress.org/attachment/ticket/15667/class-wp-
 walker.php.diff class-wp-walker.php.diff] breaks both `depth=-1` and
 `depth=0`. [https://core.trac.wordpress.org/attachment/ticket/15667/class-
 wp-walker.php.1.diff class-wp-walker.php.1.diff] changes the `>` to a
 `!==` so we only trigger the behaviour when `depth=1`.

 Phew!

 There's a couple of ways to approach fixing this:

 1. In `wp_list_pages` stop passing the orphaned children that we receive
 from `get_pages` to the `Page_Walker`.
 2. Change the behaviour of the `Walker` so that it skips orphaned child
 elements, either always or when `depth=1`.
 3. Allow a comma separated list of id's to be passed to `exclude_tree` in
 `get_pages` and then either recommend that over `exclude` or even
 specifically map `exclude` to `exclude_tree` in `wp_list_pages`.
 4. Use a Nav Menu ;-)

 Would be good to get a second opinion on the above, happy to add more unit
 tests and / or produce a patch for any of the above.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/15667#comment:8>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform