WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 3 years ago

#15595 closed defect (bug) (duplicate)

add_menu_page position conflict

Reported by: vegasgeek Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Plugins Keywords: has-patch dev-feedback
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 10 years ago.
Patch to alleviate the menu item collision described
15595.2.patch (728 bytes) - added by jorbin 10 years ago.
updated whitespace

Download all attachments as: .zip

Change History (20)

#1 @PeteMall
10 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 10 years ago by PeteMall (previous) (diff)

#2 @westi
10 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.

@toddhuish
10 years ago

Patch to alleviate the menu item collision described

#3 @vegasgeek
10 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

@jorbin
10 years ago

updated whitespace

#4 @jorbin
10 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.

#5 @scribu
10 years ago

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

#6 @scribu
10 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.

#7 @scribu
10 years ago

  • Cc 2nd-opinion removed

#8 @nacin
10 years ago

  • Keywords close removed

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

#9 @toddhuish
10 years ago

  • Cc todd@… added

#10 @aaroncampbell
10 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.

#11 follow-up: @vegasgeek
10 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.

#12 @markjaquith
10 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.

#13 in reply to: ↑ 11 @aaroncampbell
10 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.

#14 @nacin
7 years ago

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

#15 @nacin
7 years ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

#16 @DrewAPicture
5 years ago

#32731 was marked as a duplicate.

#17 follow-up: @swissspidy
4 years ago

  • Keywords dev-feedback added

Any objections to closing this as a duplicate of #12718 so we can join efforts there?

#18 in reply to: ↑ 17 @DrewAPicture
3 years ago

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

Replying to swissspidy:

Any objections to closing this as a duplicate of #12718 so we can join efforts there?

Let's do that.

Note: See TracTickets for help on using tickets.