Make WordPress Core

Opened 7 years ago

Closed 2 years ago

#40927 closed defect (bug) (fixed)

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

Reported by: justinbusa's profile justinbusa Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.8
Component: Administration Keywords: has-patch dev-feedback has-unit-tests
Focuses: administration Cc:

Description

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!

Attachments (7)

plugin.diff (508 bytes) - added by justinbusa 7 years ago.
40927.diff (1.3 KB) - added by dd32 7 years ago.
40927.2.diff (513 bytes) - added by justinbusa 7 years ago.
40927.3.diff (539 bytes) - added by justinbusa 7 years ago.
40927.patch (3.1 KB) - added by Cybr 2 years ago.
Reverts changeset #52569. Fixes tickets #40927 and #54798.
40927.1.patch (3.1 KB) - added by Cybr 2 years ago.
40927.2.patch (3.4 KB) - added by Cybr 2 years ago.
Convert array index back to string from float after the duplicate is resolved.

Download all attachments as: .zip

Change History (49)

@justinbusa
7 years ago

#1 @justinbusa
7 years ago

  • Keywords has-patch dev-feedback added

@dd32
7 years ago

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

Related #39776

#4 @justinbusa
7 years ago

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

@justinbusa
7 years ago

#5 @justinbusa
7 years ago

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

@justinbusa
7 years ago

#6 follow-up: @dd32
7 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
7 years ago

Great! Thanks for checking it out.

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


7 years ago

#9 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to Future Release

#10 @SergeyBiryukov
4 years ago

#50534 was marked as a duplicate.

#11 @SergeyBiryukov
4 years ago

#51869 was marked as a duplicate.

#12 @SergeyBiryukov
4 years ago

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

#13 @SergeyBiryukov
4 years ago

  • Keywords needs-unit-tests added

#14 @lukecarbis
3 years ago

  • Milestone changed from 5.7 to Future Release

#15 @kirtan95
2 years ago

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

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

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

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


2 years ago

@Cybr
2 years ago

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

#18 @Cybr
2 years ago

  • Keywords has-patch added

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.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

@Cybr
2 years ago

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


2 years ago

#20 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.0

@Cybr
2 years ago

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

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


2 years ago

#22 @chaion07
2 years ago

  • Keywords dev-feedback needs-testing added

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!

#23 @costdev
2 years ago

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.

#25 @peterwilsoncc
2 years ago

  • Keywords needs-refresh needs-unit-tests added

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.

This ticket was mentioned in PR #2479 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#26

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

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


2 years ago

#28 @peterwilsoncc
2 years ago

@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

#30 follow-up: @audrasjb
2 years ago

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

#31 in reply to: ↑ 30 @peterwilsoncc
2 years ago

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.

#32 @peterwilsoncc
2 years ago

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.

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


2 years ago

This ticket was mentioned in PR #2619 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#35

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

#36 @peterwilsoncc
2 years ago

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

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.

#39 follow-up: @david.binda
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

#40 in reply to: ↑ 39 @peterwilsoncc
2 years ago

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.

#41 @costdev
2 years ago

  • Keywords close added

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?

#42 @costdev
2 years ago

  • Keywords needs-testing close removed
  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.