Walker::walk makes an incorrect assumption if $top_level_elements is empty.

[rob-castlegate]

A colleague of mine was generating a sidebar sub-navigation for one of his projects. The subnavigation contained second-level and third-level navigation elements.

The problem my colleague was having was that occasionally third-level elements would not be nested underneath their parent element (also in the list of elements) on some pages.

My colleague was calling wp_list_pages with an array of page IDs that he wanted to render in the sub-navigation, wp_list_pages then turned the list of page IDs into a list of Page objects, and it sorted the page objects by their 'menu_order' attribute; the third-level navigational elements all had their 'menu_order' set to 0, whereas the second-level navigational elements all had 'menu_order' set to something more than 0 - causing the third-level elements to be the first elements in the list.

wp_list_pages later made a call to Walker::walk, passing along that list of pages. Here is a relevant code snippet from Walker::walk:

/*
		 * When none of the elements is top level.
		 * Assume the first one must be root of the sub elements.
		 */
		if ( empty($top_level_elements) ) {

			$first = array_slice( $elements, 0, 1 );
			$root = $first[0];

			$top_level_elements = array();
			$children_elements  = array();
			foreach ( $elements as $e) {
				if ( $root->$parent_field == $e->$parent_field )
					$top_level_elements[] = $e;
				else
					$children_elements[ $e->$parent_field ][] = $e;
			}
		}

The bug is this code's assumption that the first item in $elements is a suitable root-element for the entire list (sentence emboldened for anybody not wanting to read the wall of text). wp_list_pages ordered our list by 'menu_order' which put our 3rd-level elements at the top of the list - causing a 3rd-level element to be treated as the navigation's root.

I wrote up a quick fix for this (I'm not sure if it's the best fix, I'm not overly experienced in Wordpress), and for our project we'll use wp_list_pages with a custom walker class that implements my fix.

Here is the patch of my fix:

Index: public_html/wp-includes/class-wp-walker.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- public_html/wp-includes/class-wp-walker.php	(date 1404915904000)
+++ public_html/wp-includes/class-wp-walker.php	(revision )
@@ -217,12 +217,34 @@
 
 		/*
 		 * When none of the elements is top level.
-		 * Assume the first one must be root of the sub elements.
+		 * ~~Assume the first one must be root of the sub elements.~~ Disregard - RJ CGIT 2014-07-09
+		 *
+		 * ----------
+		 *
+		 * Modified by Rob Jackson, Castlegate IT; 2014-07-09:
+		 *  Do not assume the first element is root, instead loop through the elements
+		 *  until we find one whose parent is _not_ in the list of elements.  If that fails,
+		 *  just fall back to the default behaviour of using the first element.
 		 */
 		if ( empty($top_level_elements) ) {
+            $root = false;
+            $element_ids = array_map(function($element){ return $element->ID; }, $elements);
 
+            foreach($elements as $element)
+            {
+                if (!in_array($element->post_parent, $element_ids))
+                {
+                    $root = $element;
+                    break;
+                }
+            }
+            unset($element);
+
+            if ($root === false)
+            {
-			$first = array_slice( $elements, 0, 1 );
-			$root = $first[0];
+                $first = array_slice( $elements, 0, 1 );
+                $root = $first[0];
+            }
 
 			$top_level_elements = array();
 			$children_elements  = array();

Kind regards,
Rob

[tellyworth]

Appears to apply to 3.8, perhaps earlier.

[ross_ritchey]

Handle the case when there are no top elements and there is an element with a non-existant parent

[ross_ritchey]

Added a patch that handles this a bit differently. Instead of adding a new foreach I added a third conditional to the original foreach.

To do this I had to move the $root declaration out of the fallback if, which adds a small amount of performance overhead when there are top level elements.

Honestly not sure right now if this is better or worse than having a third foreach.