Customizer Menus: Debug Notices

[johnjamesjacoby]

Since r32806, I'm seeing this in my customizer:

Notice: Undefined property: stdClass::$fallback_cb in /srv/www/wordpress-develop/src/wp-includes/nav-menu-template.php on line 311

Using a pretty standard approach to menus:

wp_nav_menu( array(
	'theme_location' => 'primary',
	'container'      => false,
	'menu_class'     => 'nav navbar-nav',
	'walker'         => new Bootstrap_Nav_Menu(),
	'fallback_cb'    => '' // @todo
) );

This worked without a debug notice previous to r32806.

[johnjamesjacoby]

The cause appears to be:

WP_Customize_Nav_Menus::filter_wp_nav_menu_args();

It's unsetting both the fallback_cb and walker arguments via a filter to wp_nav_menu_args.

Currently in wp_nav_menu(), all object variables are assumed to be at least set. I see two potential fixes:

• Change the unset()'s in WP_Customize_Nav_Menus::filter_wp_nav_menu_args(); to empty strings so they remain set
• Wrap all object variable checks in wp_nav_menu() in isset() or ! empty() to avoid notices

I think for sure the second of the above two fixes should happen anyways, since (as usual) we cannot trust any object to ever have any variables set in any scope thanks to filters and nothing being immutable. I think the first of these two fixes should sufficiently address the original intentions.

Tangent: we're starting to use 1000 as if it were some kind of WP_MAX_FILTER_PRIORITY, particularly in overrides done for the Customizer (though 9999 is used in some other places.) Should we actually define these priorities somewhere?

[celloexpressions]

@westonruter: thoughts here?

[adamsilverstein]

Add explicit check for isset( $args->fallback_cb ) before using

[adamsilverstein]

In attachment​:

• Add an explicit check for isset( $args->fallback_cb ), fixing a PHP notice in the customizer.

@johnjamesjacoby - I added the isset check and verified that this corrects the notice you were seeing (using your test snippet). I didn't want to change the unsset part - concerned that may affect things elsewhere. Also, I didn’t address the unset walker argument (or any other potentially uncheck variables).

[SergeyBiryukov]

Replying to johnjamesjacoby:

Currently in wp_nav_menu(), all object variables are assumed to be at least set. I see two potential fixes:

• Change the unset()'s in WP_Customize_Nav_Menus::filter_wp_nav_menu_args(); to empty strings so they remain set
• Wrap all object variable checks in wp_nav_menu() in isset() or ! empty() to avoid notices

I think I'd prefer the first option in this ticket's scope.

[westonruter]

I agree. Instead of doing unset it should assign the properties to empty strings, which should be in line with the default values.

[adamsilverstein]

Ok, thanks for the feedback.

I will expand the patch!

[adamsilverstein]

Better reset for ! can_partial_refresh

[adamsilverstein]

In 32781.2.diff​:

• When can_partial_refresh is false, reset 'walker' and 'fallback_cb' args to their default blank string values, instead of unsetting them, avoiding PHP notices.
• Add an explicit check for isset( $args->fallback_cb ), fixing a PHP notice in the customizer, because we really should check that variables exist before using them, always.

[westonruter]

We've also been noticing this sometimes:

PHP Notice: Undefined index: echo in /srv/www/wp/wp-includes/class-wp-customize-nav-menus.php on line 756

So 32781.3.diff​ adds empty checks on the array keys to prevent undefined index notices.

[SergeyBiryukov]

In 33035:

Customizer: Avoid PHP notices after [32806].

props adamsilverstein, westonruter.
fixes #32781.

[SergeyBiryukov]

In 33043:

Update test_filter_wp_nav_menu_args() after [33035].

see #32781.

[westonruter]

The fix here introduces an unexpected side-effect: any supplied fallback_cb or walker gets clobbered in the preview, even though partial refresh is disabled. This is wrong. We disable partial refresh for the very reason that a non-serializable fallback_cb or walker gets defined in the first place, so these should remain in effect in the preview when a full page refresh is then done.

See 32781.4.diff​.

[westonruter]

Patch was tested in #32894 (comment), although not specifically the problem with notices.

[westonruter]

In 33131:

Customizer: Prevent loss of walker and fallback_cb args for wp_nav_menu.

These args only need to be cleared out when exported to JavaScript, when they are not JSON-serializable. So the filter now clears these when gathering args for exporting to JS, but otherwise now leaves the original values to be passed through to wp_nav_menu().

Fixes #32781.