Make WordPress Core

Opened 13 years ago

Closed 7 years ago

#16946 closed defect (bug) (duplicate)

Bug between CPT menu_position and add_object_page()

Reported by: momo360modena's profile momo360modena Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: needs-testing has-patch dev-feedback
Focuses: Cc:

Description

I write a demo plugin for describe the bug. See attachment.

Attachments (6)

my_bug.php (1011 bytes) - added by momo360modena 13 years ago.
Proof of bug
menu.php (13.1 KB) - added by mpvanwinkle77 13 years ago.
Patch to wp-admin/menu.php
menu.diff (981 bytes) - added by mpvanwinkle77 13 years ago.
Patch to wp-admin/menu.php
16946.diff (2.1 KB) - added by blepoxp 13 years ago.
16946.patch (1018 bytes) - added by johnjamesjacoby 13 years ago.
Comment, whitespace, and patched from wp root
16946.2.patch (1008 bytes) - added by Mte90 8 years ago.
patch refreshed

Download all attachments as: .zip

Change History (28)

@momo360modena
13 years ago

Proof of bug

#1 @momo360modena
13 years ago

  • Cc amaury@… added

#2 @scribu
13 years ago

Reproduced. The problem is that the last position isn't incremented, when using register_post_type().

#3 @scribu
13 years ago

  • Cc nacin added

#4 @scribu
13 years ago

  • Milestone changed from Awaiting Review to 3.2

#5 @mpvanwinkle77
13 years ago

  • Cc mpvanwinkle77 added
  • Keywords has-patch added; needs-patch removed
  • Owner set to scribu
  • Status changed from new to reviewing

Ok so this was my first patch submission so be gentle if i'm going about it all wrong. Cheers!

PS The first attachment still had a debugging echo statement in it. Cleaned and re-uploaded.

Last edited 13 years ago by mpvanwinkle77 (previous) (diff)

@mpvanwinkle77
13 years ago

Patch to wp-admin/menu.php

#6 @scribu
13 years ago

  • Keywords has-patch removed

Hi mpvanwinkle77,

A patch is a diff file, not an entire file.

You will find tutorials for creating diffs here:

http://core.trac.wordpress.org/#HowtoSubmitPatches

#7 @mpvanwinkle77
13 years ago

Yikes. My bad. Will resubmit.

@mpvanwinkle77
13 years ago

Patch to wp-admin/menu.php

#8 @mpvanwinkle77
13 years ago

  • Keywords has-patch added

#9 @scribu
13 years ago

Cool. One more thing about diffs: please make them against the root wp dir, not against wp-admin etc.

Also, we love white-space: http://codex.wordpress.org/WordPress_Coding_Standards#Space_Usage

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

#10 @blepoxp
13 years ago

I was apparently working on this at the same time mpvanwinkle77 was. Submitting my patch too - a little different approach:

It could really use some extra testing though. I had to refactor it several times myself as I continued to find unintended consequences with my first attempts at patching. I'd love confirmation that the current patch doesn't cause problems elsewhere.

It does two things: it prevents dynamically written $menu array keys from overwriting hardcoded keys provided by core. It also prevents keys previously added by custom post types and other add_menu_item calls from being overwritten. To accomplish this, I created a new function. Wasn't sure of the best name and place to define this function so let me know if you think it fits better elsewhere.

@blepoxp
13 years ago

#11 @scribu
13 years ago

I'm not sure using get_unique_menu_key() in add_menu_page() is a good idea.

Some plugins intentionally overwrite core menus, for various reasons. Disallowing that from add_menu_page() will just force them to manipulate $menu directly.

#12 @blepoxp
13 years ago

This is true. I wonder if we should provide extra param to add_menu_page to force an overwrite. I could see people overwriting without realizing it. That might get into backwards compat issues though.

#13 @scribu
13 years ago

  • Keywords dev-feedback added

I have no idea and I think we should concentrate on the bug at hand.

Therefore, I prefer menu.diff.

@johnjamesjacoby
13 years ago

Comment, whitespace, and patched from wp root

#14 @ryan
13 years ago

I tried this patch with my_bug.php and another plugin that does:

register_post_type('thing-y', array('label' => __('Thingies'), 'public' => true, 'taxonomies' => array('post_tag') ) );

And the Thingies menu ended up above the Dashboard menu. I haven't investigated further.

#15 @westi
13 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from 3.2 to Future Release

This has missed the boat for 3.2

Moving to Future Release for a refreshed patch to address the issues ryan raises.

#16 @layotte
12 years ago

The last patch added does not seem to fix this for me. I just ran into this problem with another developer's plugin overwriting my plugin's menu position. His plugin is available here: http://wordpress.org/extend/plugins/twitter-plugin/ mine is here: http://wordpress.org/extend/plugins/leenkme/

In his most recent version, he is calling:

add_menu_page(__('BWS Plugins'), __('BWS Plugins'), 'manage_options', 'bws_plugins', 'bws_add_menu_render', WP_CONTENT_URL."/plugins/twitter-plugin/images/px.png", 101); 

Which for some reason is over witing my plugin, which does not use the position argument.

For what it's worth, blepoxp's patch works fine for me. Of course, I've emailed the developer and told him to remove the position argument. I'm sure it affects more than just my plugin.

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

#17 @scribu
12 years ago

  • Owner scribu deleted

#18 @scribu
12 years ago

Related: #12718.

#19 @chriscct7
9 years ago

  • Keywords needs-testing added

@Mte90
8 years ago

patch refreshed

#20 @Mte90
8 years ago

  • Keywords has-patch added; needs-patch removed

I refreshed the patch based on 4.5.

#21 @Mte90
7 years ago

  • Keywords dev-feedback added

I can refresh but if there is no interest for this ticket I prefer to work on other tickets.

#22 @DrewAPicture
7 years ago

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

Let's centralize the discussion #12718.

Note: See TracTickets for help on using tickets.