WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 12 months ago

#24587 closed defect (bug) (wontfix)

Walker_Page and Walker_Nav_Menu not compatible

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

Description

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 3 years ago.
patch-24587-1
24587-tests-enforce-object.diff (1.5 KB) - added by peterwilsoncc 22 months ago.
24587-tests-enforce-object.2.diff (3.0 KB) - added by peterwilsoncc 22 months ago.
24587-tests-enforce-object.3.diff (3.8 KB) - added by peterwilsoncc 22 months ago.
24587.diff (8.5 KB) - added by peterwilsoncc 20 months ago.
24587.patch (1.6 KB) - added by deeptiboddapati 19 months ago.

Download all attachments as: .zip

Change History (28)

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


4 years ago

#3 @SergeyBiryukov
4 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
3 years ago

  • Version set to trunk

+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
3 years ago

patch-24587-1

#5 @DrewAPicture
3 years ago

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

#6 @johnbillion
3 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
3 years ago

  • Milestone changed from Future Release to 4.4

#8 @DrewAPicture
3 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
3 years ago

  • Milestone changed from 4.4 to Future Release

#10 @peterwilsoncc
22 months 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
22 months ago

#37791 was marked as a duplicate.

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


22 months ago

#13 @peterwilsoncc
22 months 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
22 months 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
22 months 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
22 months 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.


20 months ago

#18 @peterwilsoncc
20 months 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
19 months 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
19 months 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

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

#21 @SergeyBiryukov
12 months ago

#37136 was marked as a duplicate.

#22 in reply to: ↑ 20 @peterwilsoncc
12 months 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.

Note: See TracTickets for help on using tickets.