Opened 2 years ago

Last modified 10 months ago

#16946 reviewing defect (bug)

Bug between CPT menu_position and add_object_page()

Reported by: momo360modena Owned by:
Priority: normal Milestone: Future Release
Component: Administration Version: 3.1
Severity: normal Keywords: needs-patch
Cc: amaury@…, nacin, mpvanwinkle77

Description

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

Attachments (5)

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

Download all attachments as: .zip

Change History (23)

Proof of bug

  • Cc amaury@… added

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

  • Cc nacin added
  • Milestone changed from Awaiting Review to 3.2
  • 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 2 years ago by mpvanwinkle77 (previous) (diff)

Patch to wp-admin/menu.php

  • 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

Yikes. My bad. Will resubmit.

Patch to wp-admin/menu.php

  • Keywords has-patch added

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 2 years ago by scribu (previous) (diff)

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.

blepoxp2 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.

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.

  • Keywords dev-feedback added

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

Therefore, I prefer menu.diff.

Comment, whitespace, and patched from wp root

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.

  • 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.

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.

Version 0, edited 19 months ago by layotte (next)
  • Owner scribu deleted

Related: #12718.

Note: See TracTickets for help on using tickets.