WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 months ago

#16856 closed enhancement (duplicate)

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: Priority: normal
Severity: normal Version: 3.1
Component: Plugins Keywords: has-patch needs-testing needs-refresh dev-feedback
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 7 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 7 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 7 years ago.
is_numeric() check
16856.2.diff (1.8 KB) - added by nathanrice 7 years ago.
Adds is_numeric() check to wp-admin/menu.php and wp-admin/includes/plugin.php

Download all attachments as: .zip

Change History (24)

@nathanrice
7 years ago

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

@nathanrice
7 years ago

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

#1 @kawauso
7 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.

@kawauso
7 years ago

is_numeric() check

#2 @nathanrice
7 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.

@nathanrice
7 years ago

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

#3 @scribu
7 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.

#4 @nathanrice
7 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.

#5 @nathanrice
7 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.

#6 follow-up: @scribu
7 years ago

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

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

#7 in reply to: ↑ 6 @nathanrice
7 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.

#8 @scribu
7 years ago

That would be #16342, I think.

#9 @scribu
7 years ago

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

#10 @ryan
7 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.

#11 @scribu
6 years ago

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

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

#12 @WraithKenny
6 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.

#13 @briandichiara
5 years 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?

#14 @sc0ttkclark
5 years 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.

#15 @sc0ttkclark
5 years ago

  • Cc lol@… added

#16 @jeremyfelt
4 years ago

  • Component changed from Administration to Menus
  • Focuses administration added

#17 @SergeyBiryukov
4 years ago

  • Component changed from Menus to Plugins

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

#18 @chriscct7
2 years ago

  • Keywords needs-refresh dev-feedback added

#19 @smerriman
11 months ago

Opened 6 years old but I'd like to add my voice to this one too. Recently in https://core.trac.wordpress.org/ticket/23316, it was permitted to add multiple menu items to the same menu_position without causing issues. This meant the previous workaround of needing to use decimals was no longer required.

However, this doesn't apply to custom post types. Decimals are not permitted, and if I attempt to use the same menu_position for every one, the position gets incremented at each step.

This means it is impossible for me to register 5 custom post types and place them between Pages and Comments in the admin menu. When I assign a menu position of 20 to each, all of the increments bump the 5th one to position 26, which appears after comments.

Perhaps the same solution could be applied of small decimal increments, but at the least the fix above for allowing decimals would be a handy workaround.

#20 @DrewAPicture
5 months ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Let's centralize the discussion around menu_position collisions to #12718.

Note: See TracTickets for help on using tickets.