WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 21 months ago

#16946 reviewing defect (bug)

Bug between CPT menu_position and add_object_page()

Reported by: momo360modena Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: needs-patch
Focuses: Cc:

Description

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

Attachments (5)

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

Download all attachments as: .zip

Change History (23)

momo360modena3 years ago

Proof of bug

comment:1 momo360modena3 years ago

  • Cc amaury@… added

comment:2 scribu3 years ago

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

comment:3 scribu3 years ago

  • Cc nacin added

comment:4 scribu3 years ago

  • Milestone changed from Awaiting Review to 3.2

comment:5 mpvanwinkle773 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 3 years ago by mpvanwinkle77 (previous) (diff)

mpvanwinkle773 years ago

Patch to wp-admin/menu.php

comment:6 scribu3 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

comment:7 mpvanwinkle773 years ago

Yikes. My bad. Will resubmit.

mpvanwinkle773 years ago

Patch to wp-admin/menu.php

comment:8 mpvanwinkle773 years ago

  • Keywords has-patch added

comment:9 scribu3 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 3 years ago by scribu (previous) (diff)

comment:10 blepoxp3 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.

blepoxp3 years ago

comment:11 scribu3 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.

comment:12 blepoxp3 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.

comment:13 scribu3 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.

johnjamesjacoby3 years ago

Comment, whitespace, and patched from wp root

comment:14 ryan3 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.

comment:15 westi3 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.

comment:16 layotte2 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 22 months ago by scribu (previous) (diff)

comment:17 scribu22 months ago

  • Owner scribu deleted

comment:18 scribu21 months ago

Related: #12718.

Note: See TracTickets for help on using tickets.