WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 19 months ago

#15595 reopened defect (bug)

add_menu_page position conflict

Reported by: vegasgeek Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Plugins Keywords: has-patch
Focuses: administration Cc:

Description

If add_menu_page() gets called more than once using the same value for $position, only one menu item is added while the other is overwritten.

Attachments (2)

15595.patch (670 bytes) - added by toddhuish 5 years ago.
Patch to alleviate the menu item collision described
15595.2.patch (728 bytes) - added by jorbin 5 years ago.
updated whitespace

Download all attachments as: .zip

Change History (17)

comment:1 @PeteMall5 years ago

  • Cc 2nd-opinion added
  • Milestone changed from Awaiting Review to Future Release

This is not a regression from 3.0 so punting for now.

Last edited 5 years ago by PeteMall (previous) (diff)

comment:2 @westi5 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

That is expected behaviour.

You asked it to do that.

@toddhuish5 years ago

Patch to alleviate the menu item collision described

comment:3 @vegasgeek5 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

@jorbin5 years ago

updated whitespace

comment:4 @jorbin5 years ago

  • Component changed from General to Plugins
  • Keywords has-patch added
  • Milestone set to Future Release

This looks good to me and alleviates menu collision.

comment:5 @scribu5 years ago

Maybe we should focus on refactoring the API so that we don't have $position at all. See #16342

comment:6 @scribu5 years ago

  • Keywords close added

And I agree with westi. If you want the position to auto-increment, use one of the wrappers, such as add_object_page() etc.

comment:7 @scribu5 years ago

  • Cc 2nd-opinion removed

comment:8 @nacin5 years ago

  • Keywords close removed

Both Mark and I disagree. I'll post my thoughts later.

comment:9 @toddhuish5 years ago

  • Cc todd@… added

comment:10 @aaroncampbell4 years ago

I just thought I'd drop my thoughts here as I'm fighting this yet again. I know that currently the $position parameter is treated as an absolute demand, but in reality I think the most intuitive way for this to work would be to check for a conflict and if one exists to shift everything up to make room for the new item.

I just wanted to add a menu item near the top, but position 3 was taken, position 4 was the separator I wanted to be above, and position 5 was taken as well. I ended up having to write some really ugly code to do what I wanted that grabbed the menu global, took the keys from it, reverse sorted them and filtered out anything < 4, ran through them all doing $menu[$key+1] = $menu[$key]; unset($menu[$key]), had to ksort($menu), and then inserted my menu item into position 4. It would have been a lot easier if I could have used array_splice, but that resets the keys.

Maybe we could have add_menu_page() do something similar and then increment all the $_wp_last_*_menu vars that are >= the position we inserted into?

It's all rather messy honestly. I wish that there weren't all these empty spaces so we could simply array_splice in a new menu item and track the various sections using the $_wp_last_*_menu vars.

comment:11 follow-up: @vegasgeek4 years ago

Believe me, I'm with you and would like to see this sorted out. But, if you need a quick workaround, instead of using position 3, use position 3.283764827483 or something similar. Unlikely it will run in to a conflict.

comment:12 @markjaquith4 years ago

If you want to remove a menu item, you should remove it explicitly, not plop something down on top of it. Most collisions are going to be accidental and the replacement behavior is confusing. We should avoid collisions.

comment:13 in reply to: ↑ 11 @aaroncampbell4 years ago

Replying to vegasgeek:

Believe me, I'm with you and would like to see this sorted out. But, if you need a quick workaround, instead of using position 3, use position 3.283764827483 or something similar. Unlikely it will run in to a conflict.

Thanks. I had tried that and it didn't work. Explanation:

// Works as mentioned
add_menu_page('','',  8, '', '', null, '3.1');

// Ends up in position 3
add_menu_page('','',  8, '', '', null, 3.1);

The reason is that numeric indices in an array are forced to int (truncated, not rounded) but non-numeric indices can be anything.

comment:14 @nacin19 months ago

  • Component changed from Plugins to Admin APIs
  • Focuses administration added

comment:15 @nacin19 months ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

Note: See TracTickets for help on using tickets.