WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 2 months ago

Last modified 5 weeks 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:
PR Number:

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

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

Download all attachments as: .zip

Change History (30)

#1 @desrosj
12 months ago

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

#2 @audrasjb
10 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
9 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
9 months ago

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

#4 @audrasjb
9 months ago

  • Keywords reporter-feedback added

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

  • Milestone changed from 5.2 to 5.3

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

@audrasjb
6 months ago

Error wording changes and break after the first occurrence

#7 @audrasjb
6 months ago

  • Keywords needs-refresh removed

#8 @audrasjb
4 months ago

  • Keywords needs-refresh added

Needs refresh since the ticket is now milestoned to 5.3.

@audrasjb
4 months ago

Refresh patch for milestone 5.3

#9 @audrasjb
4 months ago

  • Keywords commit added; needs-refresh removed

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

@audrasjb
3 months ago

Fix issue with sprintf function in the previous patch

#10 @audrasjb
3 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

@welcher
2 months ago

Adds unit tests

#11 @welcher
2 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
2 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
2 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
2 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
2 months ago

  • Keywords needs-dev-note added

#16 @desrosj
2 months ago

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

@desrosj
2 months ago

#17 @desrosj
2 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
2 months ago

+1 from me

#19 @audrasjb
2 months ago

Perfect 👌

#20 @desrosj
2 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
2 months ago

In 46104:

Correct the version number specified in [46102].

See #45361.

#23 @audrasjb
8 weeks ago

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