WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 9 months ago

#24035 closed defect (bug) (fixed)

in WP 3.5.1, filter wp_nav_menu_items is never called on empty menus

Reported by: mykle Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.5
Component: Menus Keywords: has-patch
Focuses: Cc:

Description

We upgraded from 3.4 to 3.5.1 and our main navigation menu vanished.

Our theme uses the "wp_nav_menu_items" filter to add menu items to an empty menu, depending on login context & other things.

I have verified that in 3.5.1, this filter is never called on empty menus. wp_nav_menu() called with the proper args to select our menu just returns null. we are specifying theme_location in the call.

A workaround/hack is to add a bogus menu item to the DB, via the admin interface, and then delete that bogus menu item in our filter.

I imagine this crept in with the new 3.5 behavior of not emitting HTML for an empty menu.

The bug is in wp-includes/nav-menu-template.php . The comment at line 174 claims we will fallback on empty menus only if no theme_location is specified, and we do test that on line 177, but on line 181 we test again and fallback on empty menus without checking theme_location.

This line (181) :

if ( !$menu || is_wp_error( $menu ) || empty( $menu_items ) )

... should be this line:

if ( !$menu || is_wp_error( $menu ))

Attachments (1)

24035.diff (880 bytes) - added by DrewAPicture 12 months ago.
Bail after menu items filters have run (against 3.5.2 tag)

Download all attachments as: .zip

Change History (15)

comment:1 SergeyBiryukov15 months ago

Introduced in [21868].

comment:2 SergeyBiryukov15 months ago

  • Version set to 3.5

comment:3 DrewAPicture15 months ago

  • Keywords close added
  • Severity changed from major to normal

This is intended behavior. If the menu is empty, don't display the markup.

The comment at line 174 claims we will fallback on empty menus only if no theme_location is specified, and we do test that on line 177, but on line 181 we test again and fallback on empty menus without checking theme_location.

We 'test again' after the fallback callback is fired, which I'd argue is what you should be using to bring menu items in to an empty menu. As noted in #21576, outputting empty markup for menus can wreak havoc on theme styling that relies on wp_nav_menu falling back to the callback.

Suggest close.

comment:4 follow-up: mykle15 months ago

When the theme location is specified, the fallback is not fired.

I would argue (politely!) that the menu filters are for changing the contents of a menu, and that adding contents to an empty menu is just as legitimate a use of those filters as any other. If that's true, then you can't really say a menu is empty until you've called its filters. That has been normal behavior up to 3.5 .

Furthermore: the doc specifies the purpose of the callback is for "if a menu doesn't exist". In this case the menu definitely exists, and simply has no contents until after the filters fire.

If the menu is still empty after its filters run, it should certainly not be output. But otherwise, this change silently deactivates a long-standing WP feature.

But maybe we're doing it wrong. What's a better approach to populating an empty menu via a plugin?

comment:5 in reply to: ↑ 4 DrewAPicture15 months ago

Replying to mykle:

I would argue (politely!) that the menu filters are for changing the contents of a menu, and that adding contents to an empty menu is just as legitimate a use of those filters as any other. If that's true, then you can't really say a menu is empty until you've called its filters. That has been normal behavior up to 3.5 .

I think that's a valid argument. On the other hand, reverting the change from [21868] doesn't maintain both what you're aiming for and hiding the markup for an empty menu.

I can see moving the filter earlier or bailing later in the function, but there may be wider back-compat implications to consider. Let's see what @nacin or @SergeyBiryukov have to say.

comment:6 mighty_mt13 months ago

  • Cc bikertom@… added

comment:7 DrewAPicture12 months ago

  • Keywords dev-feedback added

comment:8 nacin12 months ago

  • Keywords needs-patch added; close dev-feedback removed
  • Milestone changed from Awaiting Review to 3.5.3

We should aim to fire any pertinent filters prior to bailing. The goal of [21868] was to avoid markup, not improve performance.

comment:9 DrewAPicture12 months ago

  • Keywords has-patch added; needs-patch removed

24035.diff moves the return after the menu item filters.

DrewAPicture12 months ago

Bail after menu items filters have run (against 3.5.2 tag)

comment:10 nacin12 months ago

(against 3.5.2 tag)

In general, always make patches against trunk. Patches against a branch can be in addition to, but not in lieu of.

comment:11 nacin12 months ago

In 24634:

If a nav menu has no items, wait until after the wp_nav_menu_items filter before deciding whether to print nothing.

see [21868] for original commit. see #21576.
see #24035 for trunk.

comment:12 nacin12 months ago

  • Keywords fixed-major added

comment:13 andg9 months ago

The bug appears to be already fixed in trunk.

comment:14 nacin9 months ago

  • Keywords fixed-major removed
  • Milestone changed from 3.5.3 to 3.6
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.