WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 7 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 (18)

#1 @PeteMall
5 years ago

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

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

Version 0, edited 5 years ago by PeteMall (next)

#2 @westi
5 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
5 years ago

Patch to alleviate the menu item collision described

#3 @vegasgeek
5 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

@jorbin
5 years ago

updated whitespace

#4 @jorbin
5 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
5 years ago

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

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

  • Cc 2nd-opinion removed

#8 @nacin
5 years ago

  • Keywords close removed

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

#9 @toddhuish
5 years ago

  • Cc todd@… added

#10 @aaroncampbell
5 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
5 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
5 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
5 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
2 years ago

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

#15 @nacin
2 years ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

#16 @DrewAPicture
7 months ago

#32731 was marked as a duplicate.

Note: See TracTickets for help on using tickets.