WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 2 years ago

#16946 closed defect (bug) (duplicate)

Bug between CPT menu_position and add_object_page()

Reported by: 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 8 years ago.
Proof of bug
menu.php (13.1 KB) - added by mpvanwinkle77 8 years ago.
Patch to wp-admin/menu.php
menu.diff (981 bytes) - added by mpvanwinkle77 8 years ago.
Patch to wp-admin/menu.php
16946.diff (2.1 KB) - added by blepoxp 8 years ago.
16946.patch (1018 bytes) - added by johnjamesjacoby 8 years ago.
Comment, whitespace, and patched from wp root
16946.2.patch (1008 bytes) - added by Mte90 4 years ago.
patch refreshed

Download all attachments as: .zip

Change History (28)

@momo360modena
8 years ago

Proof of bug

#1 @momo360modena
8 years ago

  • Cc amaury@… added

#2 @scribu
8 years ago

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

#3 @scribu
8 years ago

  • Cc nacin added

#4 @scribu
8 years ago

  • Milestone changed from Awaiting Review to 3.2

#5 @mpvanwinkle77
8 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 8 years ago by mpvanwinkle77 (previous) (diff)

@mpvanwinkle77
8 years ago

Patch to wp-admin/menu.php

#6 @scribu
8 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
8 years ago

Yikes. My bad. Will resubmit.

@mpvanwinkle77
8 years ago

Patch to wp-admin/menu.php

#8 @mpvanwinkle77
8 years ago

  • Keywords has-patch added

#9 @scribu
8 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 8 years ago by scribu (previous) (diff)

#10 @blepoxp
8 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
8 years ago

#11 @scribu
8 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
8 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
8 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
8 years ago

Comment, whitespace, and patched from wp root

#14 @ryan
8 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
8 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
8 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 7 years ago by scribu (previous) (diff)

#17 @scribu
7 years ago

  • Owner scribu deleted

#18 @scribu
7 years ago

Related: #12718.

#19 @chriscct7
4 years ago

  • Keywords needs-testing added

@Mte90
4 years ago

patch refreshed

#20 @Mte90
4 years ago

  • Keywords has-patch added; needs-patch removed

I refreshed the patch based on 4.5.

#21 @Mte90
3 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
2 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.