wp_list_pages, if it finds no pages to display, shows random child pages instead because of a bug in get_pages()

[bobsoap]

How to reproduce:

• About page is published
• additionally, there is a number of parent pages
  
• these each have a number of children
  
• when calling wp_list_pages(), the "exclude" attr excludes all parent pages, and display only the About page.

This works as long as there is at least 1 other page published that is not in the list of excluded IDs. In this example, as soon as the About page is set to "draft", wp_list_pages stops working correctly.

So... with no other pages besides the excluded ones published, we do this:

1) wp_list_pages('title_li=&depth=1&exclude=3,5,7');
=> wp_list_pages SHOULD return nothing, but instead it displays all child pages of the first parent page ID in the "exclude" attr (here: 3).

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).

It looked like random behavior at first but I've been able to identify the above pattern. I'm guessing it's a failing condition somewhere in the function.

[mdawaffe]

It looks like the problem is that get_pages() is returning the incorrect pages in all exclude situations, but the page Walker orphans the children (so doesn't display them) in most of those cases. In the ones you list above, though, the Walker does not orphan the children because there's no parent for it to start walking at.

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

As a workaround, you can set parent=0.

[jackreichert]

adds condition to walker class to prevent random child

[jackreichert]

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

If you notice the comment on line 219 says

When none of the elements is top level.
Assume the first one must be root of the sub elements.

This assumption should fine, except when the depth is limited to 1. That's when the above condition fails. The secondary condition in the patch fixes this.

[willmot]

Unit tests for get_pages() exclude and exclude_tree args

[willmot]

Unit tests for the base Walker class

[willmot]

Unit tests or the wp_list_pages exclude and exclude_tree args

[willmot]

Don't assume that the first element is root when there are no parent elements if depth=1

[willmot]

Been digging into this, it's not as simple as it first seems. First order of business, 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), 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 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 of3.

adds condition to walker class to prevent random child

class-wp-walker.php.diff breaks both depth=-1 and depth=0. 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.

[jackreichert]

class-wp-walker.php.diff breaks both depth=-1 and depth=0. class-wp-walker.php.1.diff changes the > to a !== so we only trigger the behaviour when depth=1.

Good catch!

[willmot]

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.

See #9153

[wonderboymusic]

15667.diff​ combines all the unit tests with the patch - 3 of the tests are failing... they need to pass soon for this to have a chance for 3.9

[nacin]

This isn't a big patch, but man are there a lot of tests needed here. Due to its serious depth within the API, this is not an easy change to make late in a cycle, and with the failing tests, let's get this straightened out and then pull it into 4.0.

[willmot]

Whilst I'm not sure the original change / patch is a good idea yet, it would be good to at least get the unit tests for the current functionality in, is there any reason the unit tests for get_pages, wp_list_pages & the Walker class can't be committed separately?

[wonderboymusic]

In 29347:

Add unit tests for Walker class.

Props willmot.
See #15667.

[wonderboymusic]

15667.2.diff​ is what remains

[wonderboymusic]

15667.2.diff​ is what remains

[Chair Hire London]

Is this now fixed in the latest version I am having a similar issue, wandering if upgrading to latest version will solve it for me.

[wonderboymusic]

Bump - 15667.3.diff​ is a fuzz-less refresh