Make WordPress Core

Opened 5 years ago

Last modified 10 days ago

#40927 reviewing defect (bug)

Passing a float as the position in add_menu_page can override other menu items

Reported by: justinbusa Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Administration Keywords:
Focuses: administration Cc:


If you pass a float value for the $position argument of add_menu_page, PHP will convert it to an integer which overrides any other menu items with that position.

Per the PHP docs...

Floats are also cast to integers, which means that the fractional part will be truncated. E.g. the key 8.7 will actually be stored under 8.

You can see this happening with the following snippet. The Test 2 menu item will show but Test 1 will not.

add_action( 'admin_menu', function() {
        add_menu_page( 'Test 1', 'Test 1', 'manage_options', 'test-1', '', '', 64 );
        add_menu_page( 'Test 2', 'Test 2', 'manage_options', 'test-2', '', '', 64.99 );
} );

However, if you pass 64.99 in as a string instead of a float, Test 1 will show.

Patch attached, thanks!

Attachments (4)

plugin.diff (508 bytes) - added by justinbusa 5 years ago.
40927.diff (1.3 KB) - added by dd32 5 years ago.
40927.2.diff (513 bytes) - added by justinbusa 5 years ago.
40927.3.diff (539 bytes) - added by justinbusa 5 years ago.

Download all attachments as: .zip

Change History (20)

5 years ago

#1 @justinbusa
5 years ago

  • Keywords has-patch dev-feedback added

5 years ago

#2 @dd32
5 years ago

  • Component changed from Menus to Administration
  • Keywords dev-feedback removed

Previously: #23316

40927.diff is a bit more WordPress-like version of plugin.diff.
I do wonder if we should just do something like this:

if ( ! is_int( $position ) && ! is_string( $position ) ) {
   $position = (string) $position;

#3 @welcher
5 years ago

Related #39776

#4 @justinbusa
5 years ago

@dd32 I agree as $position should always be either an int or a string. New patch attached.

5 years ago

#5 @justinbusa
5 years ago

Actually, I think that's going to break the if ( null === $position ) statement below it. New patch attached.

5 years ago

#6 follow-up: @dd32
5 years ago

Thanks for catching that! 40927.3.diff looks good to me.

I'll loop back onto this later to test it properly, and see if there's any unit testing opportunity here (not an easy one I don't think).

#7 in reply to: ↑ 6 @justinbusa
5 years ago

Great! Thanks for checking it out.

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

5 years ago

#9 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to Future Release

#10 @SergeyBiryukov
19 months ago

#50534 was marked as a duplicate.

#11 @SergeyBiryukov
14 months ago

#51869 was marked as a duplicate.

#12 @SergeyBiryukov
14 months ago

  • Milestone changed from Future Release to 5.7
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#13 @SergeyBiryukov
14 months ago

  • Keywords needs-unit-tests added

#14 @lukecarbis
11 months ago

  • Milestone changed from 5.7 to Future Release

#15 @kirtan95
10 days ago

@SergeyBiryukov @audrasjb This can be marked as fixed. I have handled this in https://core.trac.wordpress.org/ticket/54798

#16 @audrasjb
10 days ago

  • Keywords has-patch needs-unit-tests removed

Right. But then I'd keep this ticket open to also take care of add_submenu_page.

Note: See TracTickets for help on using tickets.