Make WordPress Core

Opened 2 years ago

Closed 17 months ago

Last modified 16 months ago

#45361 closed defect (bug) (fixed)

Integer menu slugs should not be supported

Reported by: desrosj Owned by: desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:


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:

    '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 (6)

45361.diff (666 bytes) - added by audrasjb 2 years ago.
Doing it wrong: Menu location slug should not be a int.
45361.2.diff (672 bytes) - added by audrasjb 21 months ago.
Error wording changes and break after the first occurrence
45361.3.diff (672 bytes) - added by audrasjb 19 months ago.
Refresh patch for milestone 5.3
45361.4.diff (606 bytes) - added by audrasjb 18 months ago.
Fix issue with sprintf function in the previous patch
45361.5.diff (1.4 KB) - added by welcher 17 months ago.
Adds unit tests
45361.6.diff (1.2 KB) - added by desrosj 17 months ago.

Download all attachments as: .zip

Change History (30)

#1 @desrosj
2 years ago

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

#2 @audrasjb
2 years 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
2 years ago

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


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.

2 years ago

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

#4 @audrasjb
2 years ago

  • Keywords reporter-feedback added

#5 @desrosj
23 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
22 months ago

  • Milestone changed from 5.2 to 5.3

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

21 months ago

Error wording changes and break after the first occurrence

#7 @audrasjb
21 months ago

  • Keywords needs-refresh removed

#8 @audrasjb
19 months ago

  • Keywords needs-refresh added

Needs refresh since the ticket is now milestoned to 5.3.

19 months ago

Refresh patch for milestone 5.3

#9 @audrasjb
19 months ago

  • Keywords commit added; needs-refresh removed

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

18 months ago

Fix issue with sprintf function in the previous patch

#10 @audrasjb
18 months ago

  • Keywords needs-unit-tests added; commit removed

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

17 months ago

Adds unit tests

#11 @welcher
17 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

#12 follow-up: @audrasjb
17 months ago

  • Keywords commit added

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

#13 in reply to: ↑ 12 @welcher
17 months ago

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

#14 @audrasjb
17 months ago

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.

#15 @audrasjb
17 months ago

  • Keywords needs-dev-note added

#16 @desrosj
17 months ago

  • Owner changed from audrasjb to desrosj
  • Status changed from assigned to reviewing

17 months ago

#17 @desrosj
17 months ago

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.

#18 @welcher
17 months ago

+1 from me

#19 @audrasjb
17 months ago

Perfect 👌

#20 @desrosj
17 months ago

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

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.

#21 @desrosj
17 months ago

In 46104:

Correct the version number specified in [46102].

See #45361.

#23 @audrasjb
16 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.