Integer menu slugs should not be supported

[desrosj]

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.

[desrosj]

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

[audrasjb]

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

[audrasjb]

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]

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

[desrosj]

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;).

[audrasjb]

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

[audrasjb]

Error wording changes and break after the first occurrence

[audrasjb]

Needs refresh since the ticket is now milestoned to 5.3.

[audrasjb]

Refresh patch for milestone 5.3

[audrasjb]

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

[audrasjb]

Fix issue with sprintf function in the previous patch

[audrasjb]

Refreshed the patch to remove a useless sprintf function.

We just need to fix unit-tests. See comment 5: https://core.trac.wordpress.org/ticket/45361#comment:5

[welcher]

Adds unit tests

[welcher]

@audrasjb @desrosj I've added the changes to the unit tests.

[audrasjb]

Thanks @welcher !

The patch still applies cleanly. I think we are good to go.
Adding commit keyword.

@desrosj @welcher do you think we need a dev-note? I'm not sure the change is very important, but I'd say yes, for the sake of backward compatibility :)

[welcher]

Replying to audrasjb:

@desrosj @welcher do you think we need a dev-note? I'm not sure the change is very important, but I'd say yes, for the sake of backward compatibility :)

I don't think we can go wrong with too much documentation :)

[audrasjb]

Alright :)
I drafted a dev note: ​https://docs.google.com/document/d/1jl1-FH3XK-5EsAyI37cHT5n-Jrbu2S54Yg9mNNJANWE/edit?usp=sharing just waiting for the ticket to be committed.

[desrosj]

This is looking good. 45361.6.diff​ avoids creating a variable and massages the message a little to be more clear.

@audrasjb @welcher If you're ok with the change, this can get in.

[welcher]

+1 from me

[audrasjb]

Perfect 👌

[desrosj]

In 46102:

Menus: Nav menu locations should not be integers.

When nav menu location slugs are integers, some hard to debug results can occur. register_nav_menus() utilizes array_merge() which renumbers numeric indexes, starting from 0. Because of this, numeric menu locations will almost always be changed.

This change introduces a _doing_it_wrong() notice to inform developers that nav menu locations should always be strings.

Props audrasjb, desrosj, welcher.
Fixes #45361.

[desrosj]

In 46104:

Correct the version number specified in [46102].

See #45361.

[jrf]

FYI: ​https://github.com/WordPress/WordPress-Coding-Standards/issues/1798

[desrosj]

For future reference, the dev note is here: ​https://make.wordpress.org/core/2019/09/18/integer-menu-slugs-are-no-longer-supported-from-wordpress-5-3/