Walker_Page and Walker_Nav_Menu not compatible

[geardev]

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.

[SergeyBiryukov]

Related: #18232

[SergeyBiryukov]

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.

[aaronholbrook]

+1.

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.

[aaronholbrook]

patch-24587-1

[johnbillion]

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.

[DrewAPicture]

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?

[peterwilsoncc]

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.

[peterwilsoncc]

#37791 was marked as a duplicate.

[peterwilsoncc]

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.

[peterwilsoncc]

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.

[peterwilsoncc]

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.

[johnbillion]

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

[peterwilsoncc]

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.

[peterwilsoncc]

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.

[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.

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

abstract

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 :)

[SergeyBiryukov]

#37136 was marked as a duplicate.

[peterwilsoncc]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

#50261 was marked as a duplicate.

[szepe.viktor]

This issue comes up in static analysis.

I help it by using these value objects

• ​https://github.com/szepeviktor/phpstan-wordpress/blob/master/example/class-wp-nav-menu-args.php
• ​https://github.com/szepeviktor/phpstan-wordpress/blob/master/example/class-wp-nav-menu-item.php