[wp-trac] [WordPress Trac] #28801: Walker::walk makes an incorrect assumption if $top_level_elements is empty.

[WordPress Trac]

#28801: Walker::walk makes an incorrect assumption if $top_level_elements is empty.
----------------------------+-----------------------------
 Reporter:  rob-castlegate  |      Owner:
     Type:  defect (bug)    |     Status:  new
 Priority:  normal          |  Milestone:  Awaiting Review
Component:  General         |    Version:  trunk
 Severity:  normal          |   Keywords:
  Focuses:                  |
----------------------------+-----------------------------
 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

--
Ticket URL: <https://core.trac.wordpress.org/ticket/28801>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform