Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#54798 closed defect (bug) (fixed)

Check for int in menu priority

Reported by: kirtan95's profile kirtan95 Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Similar to #48249, this ticket focuses on ensuring that devs don't accidentally pass non-integer values in the position argument.

A similar issue was also reported in #40927, where passing a float argument in add_menu_page causes to override menu item.

For backward compatibility, I'd like to suggest informing users with _doing_it_wrong if a non-integer value is passed.

Optionally, we can also typecast float to string to ensure in case someone has passed float value, at least it doesn't override menus.


Attachments (3)

54798.diff (3.0 KB) - added by kirtan95 2 years ago.
54798.1.diff (3.0 KB) - added by kirtan95 2 years ago.
Fixed a last minute change in unit test 😅
54798-follow-up-fix-php81-deprecation.patch (1014 bytes) - added by jrf 22 months ago.
Fix newly introduced PHP 8.1 incompatibility

Download all attachments as: .zip

Change History (21)

#1 @kirtan95
2 years ago

  • Summary changed from Check for int in menu priority implementation to Check for int in menu priority

This ticket was mentioned in PR #2143 on WordPress/wordpress-develop by kirtangajjar.


2 years ago
#2

  • Keywords has-patch has-unit-tests added

It will throw error with _doing_it_wrong if the typeof position is not
int.
Additionally, if the type of position is float, it will conver it to
string instead of typecasting it down to int which can cause override.

Fixes #54798, #40927

Trac Link - https://core.trac.wordpress.org/ticket/54798#comment:1

Trac ticket:

#3 @audrasjb
2 years ago

  • Milestone changed from Awaiting Review to 6.0
  • Owner set to audrasjb
  • Status changed from assigned to reviewing
  • Version trunk deleted

@kirtan95
2 years ago

@kirtan95
2 years ago

Fixed a last minute change in unit test 😅

#4 @SergeyBiryukov
2 years ago

  • Description modified (diff)

#5 @audrasjb
2 years ago

  • Keywords commit added

Hello and welcome back to Trac @kirtan95 !

This change looks good to me.

Self-assigning for commit consideration.

#6 @audrasjb
2 years ago

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

In 52569:

Administration: Ensure an integer is used for menu priority in add_menu_page().

This change adds a verification of the $position parameter in add_menu_page() to ensure an integer is used. If not, the function informs developers of the wrong parameter type via a _doing_it_wrong message. This brings consistency with a similar check used in add_submenu_page().

This change also typecasts any floating number to string to ensure that in case a float value was passed, at least it doesn't override existing menus.

Follow-up to [46570].

Props kirtan95.
Fixes #54798. See #48249.

#8 @audrasjb
2 years ago

In 52570:

Tests: Trac ticket number correction after changeset [52569].

Follow-up to [52569].

See #54798.

#9 @audrasjb
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Looking back on the code committed in [52569], it looks like there is a missing indentation in the translators comment. Also, /* translators: %s: add_submenu_page() */ should probably be replaced with /* translators: %s: add_menu_page() */.

Reopening to address those issues.

#10 @audrasjb
2 years ago

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

In 52571:

Administration: Fix an erroneous translators comment after changeset [52569].

This change also fixes the indentation of the translators comment.

Fixes #54798.

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


2 years ago

#12 @SergeyBiryukov
2 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening, as there are some concerns with the current implementation that does not allow for float values, which are widely used by plugins to prevent "first come first serve" issues where menus go up and down sporadically.

See comment:18:ticket:40927.

#13 @kirtan95
2 years ago

@SergeyBiryukov So is it fine to add type check for float values? It will at least prevent users from passing float values as strings and plugin authors will be easily able to migrate their code in case they're passing float as a string.

If passing a float value is so widely used, perhaps we can even change the function docstring of both add_menu_page and add_submenu_page to reflect that they accept float values.

The motive of this PR was to add stricter type checks that would help developers spot bugs early during development, so I think the above two suggestions will help achieve this goal.

#14 @peterwilsoncc
2 years ago

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

I think this can be closed with the commits on #40927

add_menu_page() position:

  • can be either a float or an integer in line with common plugin practices
  • the default position, null, adds the menu as the last item
  • a _doing_it_wrong() notice is triggered if the position is not numberic
  • float types are cast to string to ensure PHP doesn't internally cast the number to an integer
  • the position is sorted as a human would expect it, eg: 1, "1.043", "2", 3.

#15 @jrf
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Commit [52569] introduced an new incompatibility with PHP 8.1 Implicit conversion from float 1.5 to int loses precision....

This needs a follow-up to fix that.

Last edited 22 months ago by jrf (previous) (diff)

@jrf
22 months ago

Fix newly introduced PHP 8.1 incompatibility

#16 follow-up: @peterwilsoncc
22 months ago

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

@jrf Could you please open a follow up ticket.

This has commits on a closed milestone. Reopening and moving to a new milestone will create confusion during bisects and similar investigation of code history.

#17 in reply to: ↑ 16 @jrf
22 months ago

Replying to peterwilsoncc:

@jrf Could you please open a follow up ticket.

I will move the patch to #55656 which is already open for PHP 8.x patches.

As a general word of warning though: the fact that PHP 8.1 build does not currently pass, does not mean that it is acceptable to introduce new incompatibilities... Please always check the build manually for newly introduced errors.

#18 @SergeyBiryukov
22 months ago

In 53555:

Code Modernization: Use the integer portion of an item position in add_submenu_page().

This fixes an Implicit conversion from float to int loses precision PHP 8.1 deprecation notice when adding a new admin menu item with a float value passed as the $position parameter.

This change is covered by existing unit tests and addresses 8 errors when running the test suite on PHP 8.1.

References:

Follow-up to [52569], [53104].

Props jrf.
See #55656, #54798.

Note: See TracTickets for help on using tickets.