WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#32464 closed defect (bug) (fixed)

Menu args and 'container' => 'false'

Reported by: ChiefAlchemist Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.2
Component: Menus Keywords: has-patch
Focuses: docs Cc:

Description

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.

Attachments (3)

32464.patch (689 bytes) - added by shedonist 4 years ago.
32464.2.patch (737 bytes) - added by shedonist 4 years ago.
This patch has the full directory path
32464.3.patch (1.0 KB) - added by DrewAPicture 4 years ago.

Download all attachments as: .zip

Change History (18)

#1 @shedonist
4 years ago

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
4 years ago

#2 @shedonist
4 years ago

  • Keywords has-patch 2nd-opinion added

@shedonist
4 years ago

This patch has the full directory path

#3 @DrewAPicture
4 years ago

  • Focuses docs added
  • Milestone changed from Awaiting Review to 4.4

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.

#4 @ChiefAlchemist
4 years ago

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

#5 @wonderboymusic
4 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

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.

#6 @shedonist
4 years ago

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!

#7 @Otto42
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Last edited 4 years ago by Otto42 (previous) (diff)

#8 @wonderboymusic
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Nope, #11095.

#9 @shedonist
4 years ago

@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?

#10 follow-up: @tellyworth
4 years ago

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.

#11 @dd32
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Based on above comments

#12 @wonderboymusic
4 years ago

In 35736:

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

See #32464.

#13 in reply to: ↑ 10 @ocean90
4 years ago

Replying to tellyworth:

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

Related: [34950]

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


4 years ago

#15 @obenland
4 years ago

  • Keywords 2nd-opinion removed
  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.