Make WordPress Core

Opened 11 years ago

Closed 7 years ago

Last modified 4 years ago

#24587 closed defect (bug) (wontfix)

Walker_Page and Walker_Nav_Menu not compatible

Reported by: geardev's profile geardev Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: needs-patch needs-unit-tests close
Focuses: Cc:


Assume the following scenario: A developer decides to use a nav menu but wants to use a custom walker. He copy-pastes all the code in the Walker_Nav_Menu class and creates his own walker with it. But he doesn't assign a menu in the backend. Now this happens:

  • wp_nav_menu is called
  • The menu can't be found, therefore wp_page_menu is used
  • The Walker_Nav_Menu, that was passed to wp_nav_menu, is used to display the elements

But there are two problems.

The $args variable is converted to an object in wp-includes/nav-menu-template.php on line 145 by wp_nav_menu. Problem is, the wp_page_menu keeps the array instead and passes it to the nav menu walker, which needs the object. This will result in a few errors:

  • Notice: Trying to get property of non-object in wp-includes/nav-menu-template.php on line 88
  • Notice: Trying to get property of non-object in wp-includes/nav-menu-template.php on line 90
  • Notice: Trying to get property of non-object in wp-includes/nav-menu-template.php on line 90
  • Notice: Trying to get property of non-object in wp-includes/nav-menu-template.php on line 92

If you use this code snippet in an empty theme:

<?php wp_nav_menu( array(
        'theme_location' => 'non-existing-menu',
        'walker'         => new Walker_Nav_Menu
) );

Additionally, the WP_Post object is different. For a nav menu, the walker uses $item->title, which doesn't exist in the page walker. Therefore all <li>'s are empty.

One should either standardize the Walker system, or implement checks in the default Walker_Nav_Menu to provide a solid starting point for other developers.

Attachments (6)

patch-24587-1.patch (388 bytes) - added by aaronholbrook 9 years ago.
24587-tests-enforce-object.diff (1.5 KB) - added by peterwilsoncc 8 years ago.
24587-tests-enforce-object.2.diff (3.0 KB) - added by peterwilsoncc 8 years ago.
24587-tests-enforce-object.3.diff (3.8 KB) - added by peterwilsoncc 8 years ago.
24587.diff (8.5 KB) - added by peterwilsoncc 7 years ago.
24587.patch (1.6 KB) - added by deeptiboddapati 7 years ago.

Download all attachments as: .zip

Change History (31)

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

10 years ago

#3 @SergeyBiryukov
10 years ago

The type of $args in Walker_Nav_Menu::start_el() docs was changed from object to array in [25643]. However, it's treated as an object there, not as an array.

#4 @aaronholbrook
9 years ago

  • Version set to trunk


Just ran into this issue and was thoroughly confused as to why object property notation is being used on what's supposed to be an array.

Either there should be a transformation to an object of the $args array, or it should access via array indexes instead of object properties.

Very clear and well written bug report @geardev, thanks for submitting.

9 years ago


#5 @DrewAPicture
9 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 3.0

#6 @johnbillion
9 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch removed
  • Owner set to johnbillion
  • Status changed from new to accepted

I just ran across this bug. What a mess.

patch-24587-1.patch isn't an ideal solution because $args should always be an array (according to the docblocks and its default value in methods throughout the Walker class). However, changing it to an array will break things that are filtering nav_menu_css_class, nav_menu_item_id, nav_menu_link_attributes, or walker_nav_menu_start_el and expecting an object. Ugh.

#7 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.4

#8 @DrewAPicture
8 years ago

Hasn't been a lot of movement here in the last 7 weeks. @johnbillion: Would you rather still try to fix this in 4.4 or punt?

#9 @johnbillion
8 years ago

  • Milestone changed from 4.4 to Future Release

#10 @peterwilsoncc
8 years ago

What a mess!

Is converting wp_parse_args() to return an object implementing ArrayAccess viable as a backward compatible solution? A generic utility class for the purpose would be the most useful solution.

Massive con: If I recall correctly, is_array() returns false for ArrayAccess objects.

#11 @peterwilsoncc
8 years ago

#37791 was marked as a duplicate.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.

8 years ago

#13 @peterwilsoncc
8 years ago

In 24587-tests-enforce-object.2.diff, I've created some tests to enforce an $args object on filters within Walker_Nav_Menu.

It's easy to see an oversight breaking backward compatibility here so I'd like to enforce it. Will commit after sitting on these tests for a few days. Further tests will be needed for the eventual solution.

#14 @peterwilsoncc
8 years ago

In 38400:

Unit tests: Enforce $args object in wp_nav_menu() & Walker_Nav_Menu.

WordPress always* passes $args to filters as an array of arugments. A exception is made in wp_nav_menu() and the associated walker where these are passed to filters as an object, this has been the case for seven years (since [13368]).

These new tests enforce the use of an object in these filters to ensure backward compatibility is maintained.

See #24587.

#15 @peterwilsoncc
8 years ago

  • Milestone changed from Future Release to 4.7
  • Owner changed from johnbillion to peterwilsoncc
  • Status changed from accepted to assigned

I'll have a go at this over the coming weeks. The wholesale change of wp_parse_args() I mentioned in comment:10 is not the solution, I'll try a targeted approach along those lines for nav/pages.

#16 @johnbillion
8 years ago

In 38559:

Menus: Correct the docblocks for Walker_Nav_Menu, wp_nav_menu(), and walk_nav_menu_tree().

This corrects the parameter type for the $args and $item parameters passed throughout these functions, class methods, and hooks.

See #24587
See #35206
See #37770

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.

8 years ago

7 years ago

#18 @peterwilsoncc
7 years ago

  • Milestone changed from 4.7 to Future Release

Bouncing from 4.7.

I've worked up 24587.diff with a helper class and some unit tests.

However, ArrayAccess breaks most PHP array functions (including array_merge used for parsing arguments) so it's way to late to get an adequate solution.

This is a *-early ticket to give consideration to breaking BC and for developers to test their code.

#19 @peterwilsoncc
7 years ago

  • Keywords close added

I think this is a wontfix.

There is no fix for the $args mismatch without implementing ArrayAccess, which prevents the use of the array_merge() function. It's possible to check for objects implementing ArrayAccess in wp_parse_args() but this risks breaking plugins/themes using array functions directly.

Additionally there are other incompatibilities between the Nav and Page walkers so fixing the arguments object/array conflict only solves half the problem.

#20 follow-up: @deeptiboddapati
7 years ago

To me, this is a design flaw more than a bug. We're dealing with tight coupling between menu item making methods and menu assemblers. If the item makers do it wrong, then menu makers error out.

The crux of the problem concerns three pieces of jargon:
Abstract Classes- implement only the invariant steps of the algorithm. Later versions of PHP added the


keyword and made real abstract classes. These have abstract methods that subclasses must implement.

Concrete classes- implement the abstract methods to carry out subclass-specific steps or variant steps of the algorithm.

'hook operations' or 'customization hooks'- provide default behavior that subclasses can optionally extend and usually do nothing by default.

The parent WP_Nav_Walker class is pretending to be an abstract class when it isn't. If it was actually Abstract it would just do the steps that never vary, and let child classes define the abstract methods for anything that is variable.

Although the start_el, end_el, end_lvl and start_lvl methods are called 'abstract' in the documentation they are actually 'hook operations' or 'customization hooks'.

The issue is that child classes are acting like Wp_nav_walker is abstract although it's still handling variable aspects like the menu items and the arguments array.

For BC we can't make the walker class actually abstract. But a good solution is providing more customization hooks for the child classes to define what input they are expecting.

This is what the patch I'm uploading adds.

If no factory methods are re-defined in child classes then it's shouldn't change anything. But if a particular child class is expecting something different like the nav_walker_class is, then it can define the factory methods to suit.

I worked on this patch during WCUS 2016 contributor day :)

#21 @SergeyBiryukov
7 years ago

#37136 was marked as a duplicate.

#22 in reply to: ↑ 20 @peterwilsoncc
7 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Replying to deeptiboddapati:

To me, this is a design flaw more than a bug. We're dealing with tight coupling between menu item making methods and menu assemblers. If the item makers do it wrong, then menu makers error out.

I agree. Unfortunately would require a change in arguments to resolve, along side a fix for the object/array issue. It's a backcompat nightmare without enough gain to break it.

As there haven't been any objections in the last few months, I am closing as wontfix.

#23 @SergeyBiryukov
4 years ago

In 47189:

Docs: Correct type for $item and $args parameters in Walker_Nav_Menu_Checklist and Walker_Nav_Menu_Edit to match the parent Walker_Nav_Menu` class.

Follow-up to [38559], [45537].

See #24587, #48303.

#24 @SergeyBiryukov
4 years ago

#50261 was marked as a duplicate.

Note: See TracTickets for help on using tickets.