Make WordPress Core

Opened 12 years ago

Closed 9 years ago

Last modified 9 years ago

#23316 closed defect (bug) (fixed)

Top level admin sidebar menu items with conflicting positions bury one another

Reported by: beautomated's profile beautomated Owned by: chriscct7's profile chriscct7
Milestone: 4.4 Priority: normal
Severity: major Version: 3.5.1
Component: Plugins Keywords: has-patch commit needs-codex
Focuses: administration Cc:

Description

I have now seen two separate instances where a top level admin sidebar menu item wasn't showing up when another Plugin or Theme was activated. In the most recent case, using WP 3.5.1. my Plugin was creating a top level menu item with no position specified (blank, default). When I activated my client's Theme, our Plugin sidebar item disappeared and in place came the Theme Options item. I set our Plugin to use position 70, and it came back in place of the Users top level menu item.

In the earlier case, I had a custom post type that was requesting position 20. Whenever I activated Gravity Forms, my custom post type menu item was disappearing. This was with WordPress v3.5

According to the codex page: "WARNING: if two menu items use the same position attribute, one of the items may be overwritten so that only one item displays! Risk of conflict can be reduced by using decimal instead of integer values, e.g. 63.3 instead of 63 (Note: Use quotes in code, IE '63.3')."

This seems like a bug to me. Why should items be allowed to completely overwrite one another? Shouldn't they just fall in line, albeit randomly when two conflict? I can see tons of problems with Themes and Plugins killing one another's top level menu items, and the user not understanding what's going on when they loose something unexpectedly.

Attachments (2)

23316.patch (734 bytes) - added by chriscct7 9 years ago.
23316.2.patch (748 bytes) - added by chriscct7 9 years ago.
Cast to new position string

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
12 years ago

I guess #12718 would solve this.

#2 @rmccue
12 years ago

In the mean time, we could check isset( $menu[ $position] ) and increment $position until we have an empty space.

#3 follow-up: @nacin
12 years ago

Floats can't be used as array indicies, so that warning is plain wrong anyway.

#4 in reply to: ↑ 3 @rmccue
12 years ago

Replying to nacin:

Floats can't be used as array indicies, so that warning is plain wrong anyway.

Note the "Use quotes in code, IE '63.3'". It's using strings as the indices, then relying on type coercion in the sorting code.

#5 @desaiuditd
11 years ago

  • Keywords dev-feedback needs-refresh 2nd-opinion added

Just to add a point; Custom Post Type Menu Positions does not allow float / string positions.
Because of the followinf code snippet in wp-admin/menu.php (Line 108)

$ptype_menu_position = is_int( $ptype_obj->menu_position ) ? $ptype_obj->menu_position : ++$_wp_last_object_menu; // If we're to use $_wp_last_object_menu, increment it first.

It forces to have an integer value otherwise it will assign ext incremented position for the custom post type menu.

@chriscct7
9 years ago

#6 @chriscct7
9 years ago

  • Keywords has-patch added; needs-refresh 2nd-opinion removed

Patch: If the menu position is already in use, make the position it registers to a substring of the integer base conversion of the md5 of (the menu slug and menu title of the admin page being registered concatenated), multiplied by 0.00001 to always get a number < 1, and then added to the original menu position. This returns a nearly guaranteed unique number, that is repeatable to generate, and easy to do so

#7 @chriscct7
9 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner set to chriscct7
  • Status changed from new to assigned

@chriscct7
9 years ago

Cast to new position string

#8 @chriscct7
9 years ago

  • Component changed from Administration to Plugins
  • Focuses administration added
  • Keywords commit added; dev-feedback removed

#9 @chriscct7
9 years ago

Basic test code:

add_action( 'admin_menu', 'register_my_custom_menu_page' );

function register_my_custom_menu_page(){
        add_menu_page( 'custom menu title', 'custom menu', 'manage_options', 'custompage', 'my_custom_menu_page', '', 63 ); 
}

function my_custom_menu_page(){
        echo "Admin Page Test"; 
}

add_action( 'admin_menu', 'register_my_custom_menu_page2' );

function register_my_custom_menu_page2(){
        add_menu_page( 'custom menu title2', 'custom menu 2', 'manage_options', 'custompage2', 'my_custom_menu_page2', '', 63 ); 
}

function my_custom_menu_page2(){
        echo "Admin Page Test2";        
}

#10 @chriscct7
9 years ago

  • Keywords needs-codex added

Codex article needs to be updated to remove the guidance that plugins should use a decimal number for the position to avoid conflicts, as after this patch, plugins can register on the same priority without context. Possibility also a make article worthy, since many plugin authors will be happy this is fixed, and to let them know of the change.

#11 @wonderboymusic
9 years ago

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

In 35477:

Admin Menu: allow more than one menu item to be added at the same priority/position.

Props chriscct7.
Fixes #23316.

#12 follow-up: @DrewAPicture
9 years ago

Rather than perpetuating the Codex, let's future-proof the docs. We can do that by improving the inline docs and/or writing up some supplemental information for the code reference, then redirect the Codex page(s) for add_menu_page() and add_submenu_page().

Inline docs will carry over when the code reference is reparsed for the 4.4 release. Supplemental information can be pushed live anytime.

#13 in reply to: ↑ 12 ; follow-up: @chriscct7
9 years ago

Replying to DrewAPicture:

Rather than perpetuating the Codex, let's future-proof the docs. We can do that by improving the inline docs and/or writing up some supplemental information for the code reference, then redirect the Codex page(s) for add_menu_page() and add_submenu_page().

Inline docs will carry over when the code reference is reparsed for the 4.4 release. Supplemental information can be pushed live anytime.

The information is custom to the current codex, so the proposal is to just not change the current codex then, since that information wouldn't be auto-carried over?

#14 in reply to: ↑ 13 @DrewAPicture
9 years ago

Replying to chriscct7:

The information is custom to the current codex, so the proposal is to just not change the current codex then, since that information wouldn't be auto-carried over?

Correct. The problem is that since these particular Codex articles are already mid-migration (as are many many others), at some point we can't guarantee that anything new that gets added is going to be carried over. The best thing we can do is solidify the inline docs as the canonical source of information. This is why changelog entries for significant functionality changes are important, they preserve the history when read out of context.

If there's supplemental info – that is to say something we don't want to bloat the inline docs with – for the time being, submitting that as a "user contributed note" to the code reference article(s) is the best way to ensure it gets carried over.

Note: See TracTickets for help on using tickets.