Menu args and 'container' => 'false'

[ChiefAlchemist]

See Note 1 and Note 2 as added to the Codex.

​http://codex.wordpress.org/Function_Reference/wp_nav_menu#Removing_the_Navigation_Container

In short, just having 'container' in the args list will suppress the container. And 'container' => true replaces the container's div tag with a 1.

[shedonist]

Tested this and verified that passing

'container'= true

does results in a "1" being used as the element name. For example:

<1 class="menu-main-container">

Since the default container value is 'div', it makes sense to default to 'div' when 'true' is passed in.

[shedonist]

This patch has the full directory path

[DrewAPicture]

This feels a little bit like developer error, as the argument is not meant to be used in a truly boolean fashion. In fact, the passing of false isn't even a documented feature (in the inline docs).

I think rather that coding this defensively, we should realistically update the inline docs for the $container argument to reflect that it can accept either a string or false.

Passing anything else could rightly produce unexpected results, as you've illustrated.

[ChiefAlchemist]

1) This says the container is removable, yes?

​http://codex.wordpress.org/Function_Reference/wp_nav_menu#Removing_the_Navigation_Container

2) If false is legit most would presume true would be as well. As in there could be conditions where you'd want 'container' => $bool_flag - where $bool_flag is set programatically.

3) Moi? I was always trained to presume the worst, including "developer error" (even when it doesn't make sense.) That is, in this case, the function should validate / catch the "error" (just in case) and not presume the dev will be inhuman (i.e., perfect.)

Documented or not, the result is not desired and/or the function could be more robust. Again, maybe it's just me, but to me leaving it as-is doesn't make sense. Up to you :)

[wonderboymusic]

In 34630:

Nav Menus: in wp_nav_menu(), $container is already bound to a list of allowed tags. PHP, being its whimsical self, while return true if someone sets $container to true via in_array( true, [ 'div', 'nav' ] ). Check that $container is a string before the in_array() check. 'true' does not pass.

Props shedonist for the original patch.
Fixes #32464.

[shedonist]

Scott, I see that your fix addresses the problem in a different way which does resolve the issue. However your fix results in 'container' = true not returning a container at all whereas my fix results in the default "div" container being returned. Wouldn't it be better to return a container since the true argument implies one was desired? Or is it considered better to make the function conform to the function documentation?

I'm not trying to be argumentative. I'm just genuinely curious as this is my first foray into contributing to core and I'd like to better understand the preferred methodology. Thanks!

[Otto42]

It seems that [34630] breaks the theme previewer.

Example: ​http://wp-themes.com/storefront/

Shows div class=menu instead of ul class=menu, which breaks the CSS.

Note that this currently affects 2/3 of the top 12 themes.

[wonderboymusic]

Nope, #11095.

[shedonist]

@Wonderboymusic - still curious about the wisdom of returning no container for container = true (your fix) vs returning the default div element (my fix). Can you comment?

[tellyworth]

Since [34630], the behaviour of 'container' => '' has changed.

Previously it would remove the container. Now it results in a tag with an empty element:

				< class="menu clear-fix">

Note that the empty string param is explicitly given in the docs as an example ​http://codex.wordpress.org/Function_Reference/wp_nav_menu#Removing_the_Navigation_Container:

<?php wp_nav_menu( array( 'container' => '' ) ); ?>

I'm not entirely sure what the intention is in this case, but this seems like a regression, and the behaviour doesn't match the docs.

[dd32]

Based on above comments

[wonderboymusic]

In 35736:

Add a unit test for wp_nav_menu() with container => ''

See #32464.

[ocean90]

Replying to tellyworth:

Since [34630], the behaviour of 'container' => '' has changed.

Related: [34950]

[obenland]

I can't reproduce it either. Everything looks swell for core, I'll continue to investigate on .com.