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

[justinbusa]

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.

<?php
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!

[dd32]

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;
}

[welcher]

Related #39776

[justinbusa]

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

[justinbusa]

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

[dd32]

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

[justinbusa]

Great! Thanks for checking it out.

[SergeyBiryukov]

#50534 was marked as a duplicate.

[SergeyBiryukov]

#51869 was marked as a duplicate.

[kirtan95]

@SergeyBiryukov @audrasjb This can be marked as fixed. I have handled this in #54798.

[audrasjb]

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

[Cybr]

Reverts changeset #52569. Fixes tickets #40927 and #54798.

[Cybr]

I found I linked to the changeset incorrectly, which should lead here: [52569].

In "40927.1.patch" I copied the handling of $menu to $submenu.
I removed all strict type checks and simplified the code.

The input still allows for float values, for they're widely used to prevent "first come first serve" issues, where menus go up and down sporadically. See plugin coverage: ​https://wpdirectory.net/search/01FVWACTXKES71SZ65JDBH3GEJ.

Since PHP converts all numerics to weakly typed integers ($a['1'] === $a[1]), we can safely cast the position to a string value.

No changes should be noticed by plugin developers. I suggest allowing int|float for $position in the PHPDocs; perhaps even the string-equivalent thereof, although I recognize that is difficult to convey.

Aside: I found that $menu is sorted only once, but add_submenu_page() keeps re-sorting the submenu with every item added. This isn't a big issue, but it's not the most efficient. In any case, I copied the sorting algorithm used for $menu and applied it to $submenu.

I see I uploaded a patch of the wrong iteration of my work -- 40927.1.patch will resolve that momentarily.

[Cybr]

Convert array index back to string from float after the duplicate is resolved.

[chaion07]

Thanks @justinbusa for reporting this. The core team reviewed this ticket recently during a ​bug scrub session. Based on the feedback we're updating the ticket with the following:

1. Added workflow needs-testing & dev-feedback
2. Suggested approach should be to include the sort taken from $menu and applied to $submenu

Props to @costdev

Cheers!

[costdev]

To clarify:

The testing and feedback should be for the approach 40927.2.patch uses to resolve the ticket's issue, as well as testing/giving feedback on the change in the $submenu sorting algorithm to confirm that it doesn't introduce a regression and improves efficiency as intended.

Additionally, as the change to the sorting algorithm isn't strictly part of this ticket, this change might be worth committing separately to make the history clearer.

https://core.trac.wordpress.org/ticket/40927

[peterwilsoncc]

I've linked a ​pull request based on 40927.2.patch​ to get the tests running.

The tests are currently failing, I suspect due to the following:

• PR is storing with the array key (string) $position + (a tiny randomish number) whereas the tests are testing for the key (int) $position.
• change in sorting algorithm, possibly.

---

For now, let's target the bug that passing different values can result in an override (64.64 replacing 64).

Avoiding collisions with a tiny randomish number can wait as it's documented that only one menu item can own a $position value.

It's strongly advised against but it is currently possible to manipulate the $menu global directly. For example, I could remove the comments menu with:

<?php
global $menu;
unset( $menu[25] );

It's not a good idea to do so but it would be nice not to break backward compatibility if other options are available.

https://core.trac.wordpress.org/ticket/40927

[peterwilsoncc]

@costdev @SergeyBiryukov @audrasjb I've created a much simpler ​pull request #2479 to allow for the numerous plugins passing the position as a float.

• I've removed the new _doing_it_wrong so it can be dealt with later/discussed
• null or a non-numeric string append new menu to final position
• reordered collision detection, made it a little faster with an order of operations change
• tests are passing, which is nice

Additionally, in the commit ​ceebb6a8c I've modified the code for add_submenu_page to be consistent with the add_menu_page changes on this ticket.

• allows floats or ints
• reduces cases in which the doing it wrong added in [46570] for #48249 can fire
• retains the completely different method submenus use for determining the position

Replaced by ​https://github.com/WordPress/wordpress-develop/pull/2479

[audrasjb]

Ok fine @peterwilsoncc, the new PR looks good to me 👍

[peterwilsoncc]

Replying to audrasjb:

Ok fine @peterwilsoncc, the new PR looks good to me 👍

Thanks JB, I'll commit with a See reference to figure out getting the doing it wrong call back in.

[peterwilsoncc]

In 53104:

Administration: Allow floats for menu positions.

Permit plugin authors to pass the menu position as a float in add_menu_page() and add_submenu_page(). This allows for a common practice within major plugins to avoid menu collisions by passing a float.

Follow up to [52569].

Props justinbusa, dd32, welcher, SergeyBiryukov, kirtan95, audrasjb, Cybr, chaion07, costdev, peterwilsoncc.
See #40927.

https://core.trac.wordpress.org/changeset/53104

https://core.trac.wordpress.org/ticket/40927

Follow up to add the _doing_it_wrong() call.

Modified the text in the submenu's version to avoid a similar but duplicate string.

For consistency, they both now accept floats per the discussion on the ticket.

cc @costdev for review

[peterwilsoncc]

In 53264:

Administration: Trigger a notice for incorrect add_menu_page() parameter.

Trigger a notice (via _doing_it_wrong()) for developers incorrectly setting the position when calling add_menu_page().

Modify a similar message in add_submenu_page() to combine near identical strings and match the error description to the conditions in which it is called.

Follow up to [52569], [53104].

Fixes #40927.

Merged in https://core.trac.wordpress.org/changeset/53264

Merged in https://core.trac.wordpress.org/changeset/53264

[david.binda]

Sorry for re-opening. But the logic for computing the $collision_avoider has changed in the latest patch.

base_convert( substr( md5( $menu_slug . $menu_title ), -4 ), 16, 10 ) * 0.00001;

vs. original:

substr( base_convert( md5( $menu_slug . $menu_title ), 16, 10 ), -5 ) * 0.00001;

and those give different results. I wonder, is this change intentional?

[peterwilsoncc]

Replying to david.binda:

I wonder, is this change intentional?

It is intentional to improve performance. The re-ordered code is pretty consistently 10 times faster than the old code. See ​https://3v4l.org/kaR8b#v7.4.29.

The results do differ but given it's a pseudorandom number seeded by the hash, I figured it was worth the performance improvement.

[costdev]

The results do differ but given it's a pseudorandom number seeded by the hash, I figured it was worth the performance improvement.

I agree, and propose that we close this ticket again as fixed. What do you think @davidbinda?

[costdev]

With 6.0 RC1 tomorrow, I'm closing this ticket as fixed.