WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 3 days ago

#45361 assigned defect (bug)

Integer menu slugs should not be supported

Reported by: desrosj Owned by: audrasjb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch commit
Focuses: Cc:

Description

Currently, menus can be registered with an integer slug. This can cause unintended issues that are difficult to debug.

Consider the following snippet:

register_nav_menus( array(
	'primary' => 'Primary',
	1         => 'First',
	2         => 'Second',
) );

The assumption is that the resulting list of menus would match what was passed. Instead, the result is this:

array(
    'primary' => 'Primary',
    0         => 'First',
    1         => 'Second',
)

This would cause wp_nav_menu( array( 'theme_location' => 1 ) ) to return the wrong menu.

This is happening because array_merge() is used to add new nav menu locations to the global $_wp_registered_nav_menus variable. array_merge() renumbers numeric indexes, incrementing keys starting from zero in the result array.

My proposal is to add a _doing_it_wrong() warning when registering a nav menu with a numeric index. This will at least inform developers of the potential issue, and would also encourage better practices of using a string slug for nav menus.

Casting the integer as a string could also be explored, that is likely to be complicated for backward compatibility.

Attachments (3)

45361.diff (666 bytes) - added by audrasjb 6 months ago.
Doing it wrong: Menu location slug should not be a int.
45361.2.diff (672 bytes) - added by audrasjb 2 months ago.
Error wording changes and break after the first occurrence
45361.3.diff (672 bytes) - added by audrasjb 3 days ago.
Refresh patch for milestone 5.3

Download all attachments as: .zip

Change History (12)

#1 @desrosj
8 months ago

For context, I encountered this while working on #45018.

#2 @audrasjb
6 months ago

  • Owner set to audrasjb
  • Status changed from new to assigned

Great idea Jonathan, thanks\\
I'll try some experimentation on my side to see what is the best solution.

#3 @audrasjb
6 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 5.2

Hi,

45361.diff adds a _doing_it_wrong() warning message:

"Menu location slug (key) for menu "%s" should not be a int."

%s is the value of the $locations array, not the key, since numeric indexes can be different from what developers used.

@audrasjb
6 months ago

Doing it wrong: Menu location slug should not be a int.

#4 @audrasjb
5 months ago

  • Keywords reporter-feedback added

#5 @desrosj
4 months ago

  • Keywords needs-refresh added; reporter-feedback removed

Thanks for working on this, @audrasjb!

Some feedback:

  • The patch causes two unit test methods to fail. These should be able to be corrected with the @expectedIncorrectUsage tag.
  • I am a little hesitant to introduce a notice for every single menu with an integer slug. This could very quickly cause an error log to become large in size. Instead, how about the message is less specific (don't include the key), maybe "Strings should always be used for menu location slugs.", and let's only call _doing_it_wrong() for the first instance that is encountered (use a break;).

#6 @audrasjb
4 months ago

  • Milestone changed from 5.2 to 5.3

Moving to 5.3, the deadline is too short for 5.2.

@audrasjb
2 months ago

Error wording changes and break after the first occurrence

#7 @audrasjb
2 months ago

  • Keywords needs-refresh removed

#8 @audrasjb
3 days ago

  • Keywords needs-refresh added

Needs refresh since the ticket is now milestoned to 5.3.

@audrasjb
3 days ago

Refresh patch for milestone 5.3

#9 @audrasjb
3 days ago

  • Keywords commit added; needs-refresh removed

Patch refreshed.
The path has been reviewed so I'm adding commit keyword.

Note: See TracTickets for help on using tickets.