WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#16856 new enhancement

When registering custom post type, menu_position isn't honored if it's a number passed as a string

Reported by: nathanrice Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Plugins Keywords: has-patch needs-testing
Focuses: administration Cc:

Description

When registering a custom post type, if you use '50' instead of just 50, is_int() will fail and the position will be set to ++$_wp_last_object_menu. Seems safer to use isset() instead, since position doesn't technically need to be an integer. ($menu gets sorted before being output, and '50' is the same as 50 in that case)

This has the added benefit of allowing MANY more spaces in the $menu variable. For instance, if you passed '50.555' as menu_position, there's very little chance that this will conflict with another theme/plugin register a custom menu or custom post type, and will successfully go between 50 and 51 in the $menu array.

There are currently no checks on $position when using add_menu_page() ... but the checks on register_post_type() currently make this an invalid universal solution.

Personally, I don't see a downside to letting menu_position be a string. Anything other than a number (whole or decimal) will just go to the bottom of the $menu array, anyway, after the sort.

I'm submitting two patches ... one is type independent, and the other casts menu_position as (int) after the isset() check.

Attachments (4)

menu.php.noint.diff (837 bytes) - added by nathanrice 3 years ago.
Use isset() instead of is_int() when setting the $menu position for custom post types
menu.php.int.diff (837 bytes) - added by nathanrice 3 years ago.
Use isset() instead of is_int() when setting the $menu position for custom post types. Cast position as (int).
16856.diff (795 bytes) - added by kawauso 3 years ago.
is_numeric() check
16856.2.diff (1.8 KB) - added by nathanrice 3 years ago.
Adds is_numeric() check to wp-admin/menu.php and wp-admin/includes/plugin.php

Download all attachments as: .zip

Change History (21)

nathanrice3 years ago

Use isset() instead of is_int() when setting the $menu position for custom post types

nathanrice3 years ago

Use isset() instead of is_int() when setting the $menu position for custom post types. Cast position as (int).

comment:1 kawauso3 years ago

Both patches are the same?

Also, is_numeric() would preserve the validation aspect (string values for a position sound bad to me) while not requiring an integer.

kawauso3 years ago

is_numeric() check

comment:2 nathanrice3 years ago

  • Keywords needs-testing added

@kawauso
I like that idea too. I'm not as averse to the idea of text strings as you are, but this seems like it would make everyone happy.

It's probably a good idea to add this check on add_menu_page() too.

nathanrice3 years ago

Adds is_numeric() check to wp-admin/menu.php and wp-admin/includes/plugin.php

comment:3 scribu3 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.1.1

16856.2.diff looks good.

Allowing strings as menu positions is an interesting idea, but it kind of breaks the paradigm.

comment:4 nathanrice3 years ago

@scribu
Thanks!

The current menu position paradigm is weak, IMO, and I think other devs agree. Giving 15 positions between Comments and Appearance for CPTs and custom menus means there is bound to be a conflict at some point. The 15 position limit itself isn't the weakness, it's the fact that there's a limit at all.

Ideally, menu_position would accept a section as a parameter. Main, Content, Settings, for example. And you could register a new section too, if you wanted, that could go at the bottom, or between two other sections.

register_menu_section( 'my-section', array( 'main', 'content' ) );

You get the idea. Register 'my-section' between 'main' and 'content', adding the menu separaters automatically.

Then, top level menus (custom menus, CPTs, etc.) get added to a section (default or custom) based on the order they're registered. Let themers and plugin writers fight over who's menu gets added first/last, but at least they all show up.

comment:5 nathanrice3 years ago

Oh, and backward compatibility shouldn't be an issue, since the numeric positions should easily correspond to a menu section. The real weakness is allowing people to position their menus within sections. I understand that a CPT doesn't necessarily belong below Comments, for instance. Probably not an insurmountable problem, but one worth noting.

comment:6 follow-up: scribu3 years ago

Yes, problem already noted: #12718 (perfect example of a derailed ticket, otoh)

Last edited 3 years ago by scribu (previous) (diff)

comment:7 in reply to: ↑ 6 nathanrice3 years ago

Replying to scribu:

(perfect example of a derailed ticket, otoh)

No kidding. Is there a particular ticket that is currently active for refactoring the admin menu system? I would potentially be interested in contributing.

comment:8 scribu3 years ago

That would be #16342, I think.

comment:9 scribu3 years ago

  • Milestone changed from 3.1.1 to Future Release
  • Type changed from defect (bug) to enhancement

comment:10 ryan3 years ago

Changing menu code in a dot release is rather scary. This seems like an enhancement rather than a fix for a regression. Let's take care of it later. Punted per bug scrub.

comment:11 scribu3 years ago

#18622 has another conflict example (closed as duplicate).

Last edited 3 years ago by scribu (previous) (diff)

comment:12 WraithKenny3 years ago

I've found a possibly related bug to this. Found a theme that registers 3 public custom post types (only one of which has a numeric menu position) and a plugin uses add_object_page (without a numeric position). The plugin's menu overwrites one of the post types, but not the one with the numeric menu position. I had to solve this for a client today.

Conceivably, this means that a plugin registering a CPT without a position, and another using add_object_menu can be caused to conflict by an unrelated 3rd plugin using a CPT with a numeric position. Strange huh?

Disclaimer: I didn't dive into the core code, or deeply test this theory. I might after work.

comment:13 briandichiara13 months ago

I think it would be great if this was updated into core. Last updated 18 months ago. +1 tested and working! Is there time to get this into 3.6 before code freeze?

comment:14 sc0ttkclark13 months ago

This one is really bothering me, really wish it could handle floats like 20.1 etc.. which would be covered by is_numeric() vs a strict is_int() check.

Can someone who could bless this for 3.7 explain what it would take for this ticket to move forward? Updated patch is one thing, but I wouldn't want to update it and then it not move forward.

comment:15 sc0ttkclark13 months ago

  • Cc lol@… added

comment:16 jeremyfelt3 months ago

  • Component changed from Administration to Menus
  • Focuses administration added

comment:17 SergeyBiryukov3 months ago

  • Component changed from Menus to Plugins

Admin menu API now belongs to Plugins, according to the new component structure.

Note: See TracTickets for help on using tickets.