WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 weeks ago

#15667 assigned defect (bug)

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

Reported by: bobsoap Owned by: jackreichert
Milestone: Future Release Priority: normal
Severity: minor Version: 3.0.2
Component: Query Keywords: good-first-bug needs-unit-tests has-patch 4.0-early
Focuses: Cc:

Description

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.

Attachments (6)

class-wp-walker.php.diff (508 bytes) - added by jackreichert 2 months ago.
adds condition to walker class to prevent random child
15667.get_pages-unit-tests.diff (2.4 KB) - added by willmot 7 weeks ago.
Unit tests for get_pages() exclude and exclude_tree args
15667.walker-unit-tests.diff (6.0 KB) - added by willmot 7 weeks ago.
Unit tests for the base Walker class
15667.wp_list_pages-unit-tests.diff (1.4 KB) - added by willmot 7 weeks ago.
Unit tests or the wp_list_pages exclude and exclude_tree args
class-wp-walker.php.1.diff (510 bytes) - added by willmot 7 weeks ago.
Don't assume that the first element is root when there are no parent elements if depth=1
15667.diff (10.3 KB) - added by wonderboymusic 4 weeks ago.

Download all attachments as: .zip

Change History (21)

comment:1 mdawaffe3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from wp_list_pages, if it finds no pages to display, shows random child pages instead to wp_list_pages, if it finds no pages to display, shows random child pages instead because of a bug in get_pages()

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.

comment:2 nacin3 months ago

  • Component changed from General to Query
  • Keywords good-first-bug needs-unit-tests added; wp_list_pages removed

comment:3 nacin3 months ago

  • Severity changed from critical to minor

jackreichert2 months ago

adds condition to walker class to prevent random child

comment:4 jackreichert2 months ago

  • Keywords has-patch added; needs-patch removed

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.

comment:5 ircbot2 months ago

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

comment:6 DrewAPicture2 months ago

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

comment:7 SergeyBiryukov2 months ago

  • Milestone changed from Future Release to 3.9

willmot7 weeks ago

Unit tests for get_pages() exclude and exclude_tree args

willmot7 weeks ago

Unit tests for the base Walker class

willmot7 weeks ago

Unit tests or the wp_list_pages exclude and exclude_tree args

willmot7 weeks ago

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

comment:8 willmot7 weeks ago

  • Keywords 2nd-opinion added

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.

comment:9 ircbot7 weeks ago

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

comment:10 jackreichert7 weeks ago

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!

comment:11 ircbot6 weeks ago

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

comment:12 willmot6 weeks ago

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

wonderboymusic4 weeks ago

comment:13 wonderboymusic4 weeks ago

  • Keywords 2nd-opinion removed

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

comment:14 nacin3 weeks ago

  • Milestone changed from 3.9 to Future Release

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.

comment:15 samuelsidler3 weeks ago

  • Keywords 4.0-early added
Note: See TracTickets for help on using tickets.