WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 3 years 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: needs-unit-tests has-patch
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 (8)

class-wp-walker.php.diff (508 bytes) - added by jackreichert 4 years ago.
adds condition to walker class to prevent random child
15667.get_pages-unit-tests.diff (2.4 KB) - added by willmot 4 years ago.
Unit tests for get_pages() exclude and exclude_tree args
15667.walker-unit-tests.diff (6.0 KB) - added by willmot 4 years ago.
Unit tests for the base Walker class
15667.wp_list_pages-unit-tests.diff (1.4 KB) - added by willmot 4 years ago.
Unit tests or the wp_list_pages exclude and exclude_tree args
class-wp-walker.php.1.diff (510 bytes) - added by willmot 4 years 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 years ago.
15667.2.diff (4.3 KB) - added by wonderboymusic 4 years ago.
15667.3.diff (4.3 KB) - added by wonderboymusic 3 years ago.

Download all attachments as: .zip

Change History (30)

#1 @mdawaffe
7 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.

#2 @nacin
4 years ago

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

#3 @nacin
4 years ago

  • Severity changed from critical to minor

@jackreichert
4 years ago

adds condition to walker class to prevent random child

#4 @jackreichert
4 years 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.

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


4 years ago

#6 @DrewAPicture
4 years ago

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

#7 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 3.9

@willmot
4 years ago

Unit tests for get_pages() exclude and exclude_tree args

@willmot
4 years ago

Unit tests for the base Walker class

@willmot
4 years ago

Unit tests or the wp_list_pages exclude and exclude_tree args

@willmot
4 years ago

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

#8 @willmot
4 years 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.

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


4 years ago

#10 @jackreichert
4 years 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!

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


4 years ago

#12 @willmot
4 years 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

@wonderboymusic
4 years ago

#13 @wonderboymusic
4 years 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

#14 @nacin
4 years 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.

#15 @samuelsidler
4 years ago

  • Keywords 4.0-early added

#16 @willmot
4 years ago

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?

#17 @wonderboymusic
4 years ago

In 29347:

Add unit tests for Walker class.

Props willmot.
See #15667.

#20 @Chair Hire London
3 years ago

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.

#21 @wonderboymusic
3 years ago

Bump - 15667.3.diff is a fuzz-less refresh

#22 @wonderboymusic
3 years ago

  • Keywords good-first-bug 4.0-early removed
Note: See TracTickets for help on using tickets.