WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 months ago

#12718 reopened enhancement

Better structure for admin menu

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

Description (last modified by scribu)

Currently, the global $menu variable is one big linear array:

$menu = array(
    [2] => array('Dashboard', ...
    [4] => array('', 'read', 'separator1', ...),
    [5] => array('Posts', ...)
    ...
)

To allow plugins to add a menu item at the end of a group, we use a bunch of additional global variables that remember the last element in each group.

Also, we use arbitrary numeric indexes to specify the order of the items, instead of being able to position items relative to one another.

It's all very low level. Things would be a lot easier if we had an actual API for manipulating the menu items.

Attachments (13)

12718.diff (79.2 KB) - added by scribu 21 months ago.
12718.2.diff (80.9 KB) - added by scribu 21 months ago.
12718.3.diff (79.8 KB) - added by scribu 21 months ago.
plugins-using-global-menu.txt (27.8 KB) - added by nacin 21 months ago.
plugins-using-global-submenu.txt (33.6 KB) - added by nacin 21 months ago.
plugins-using-GLOBALS-menu.txt (2.0 KB) - added by nacin 21 months ago.
plugins-using-GLOBALS-submenu.txt (2.2 KB) - added by nacin 21 months ago.
plugins-using-custom_menu_order.txt (3.7 KB) - added by nacin 21 months ago.
plugins-using-global-menu-filtered.txt (3.8 KB) - added by azaozz 21 months ago.
12718.4.diff (80.0 KB) - added by scribu 21 months ago.
12718.5.diff (83.6 KB) - added by scribu 21 months ago.
12718.6.diff (83.6 KB) - added by scribu 21 months ago.
test_admin_menu.php (899 bytes) - added by scribu 19 months ago.

Download all attachments as: .zip

Change History (100)

comment:1 scribu4 years ago

  • Owner set to scribu
  • Status changed from new to accepted

In terms of backwards compatibility, only the $position parameter from add_menu_page() would be affected.

comment:2 nacin4 years ago

  • Component changed from General to Plugins

Whatever we do we would need this to be fully back compat.

comment:3 follow-up: scribu4 years ago

Plugins that specify a $position would just get their menus created at the bottom. I think that's an acceptable degree of backwards compatibility-ness.

Anyway, lots of time to think about this until the 3.1 cycle starts.

comment:4 in reply to: ↑ 3 markmcwilliams4 years ago

Replying to scribu:

Plugins that specify a $position would just get their menus created at the bottom. I think that's an acceptable degree of backwards compatibility-ness.

+1 Like the sound of that! :D

comment:5 cags4 years ago

The current system 'allows' for overriding of items, that is to say if you specify a $position that already exists it will simply overwrite it. I'm not sure if this was intentional or not but it seems like strange behaviour to me. I can see that you might wish to allow people to override items, but it just seems to easy to do by accident. I'd personally like to see this as an option, but by default shift items down out of the way. ($position_override=FALSE for example).

comment:6 follow-up: nacin4 years ago

Interesting approach by MikeSchinkel:
https://gist.github.com/792b7aa5b695d1092520
https://gist.github.com/65dd845e9637f621b73b

cags: It does allow override in some cases, but not in others. I think menu_position for register_post_type ensures no conflict, or maybe it ensures no conflict when it is then pushing them onto the $menu array.

comment:7 nacin4 years ago

Related #14666, removing menus and submenus.

comment:8 in reply to: ↑ 6 ; follow-up: mikeschinkel4 years ago

Replying to nacin:

cags: It does allow override in some cases, but not in others. I think menu_position for register_post_type ensures no conflict, or maybe it ensures no conflict when it is then pushing them onto the $menu array.

cags?

Also note that I wrote those to be 100% compatible with the current structure.

They could be added to core and used for 3 or 4 versions down the road and at the same time discourage all direct access.

By about v3.5 we could potentially modify the underlying structure to be more rational, deprecate the methods I have related to refresh, and just let the really old plugins break.

Potentially good transition strategy?

comment:9 in reply to: ↑ 8 ; follow-up: nacin4 years ago

Replying to mikeschinkel:

cags?

It was in reply to http://profiles.wordpress.org/cags - see comment above.

Also note that I wrote those to be 100% compatible with the current structure.

That's good. Excellent, in fact. I don't think we'll be looking too far ahead (you reference 3.5 for example) but severing plugins from direct access is indeed the next step. That said I doubt you'll see a refactoring of the underlying storage on a roadmap in the near future, as long as an abstraction layer does everything we need it to.

comment:10 in reply to: ↑ 9 mikeschinkel4 years ago

  • Cc mikeschinkel@… added

Replying to nacin:

Replying to mikeschinkel:

cags?

It was in reply to http://profiles.wordpress.org/cags - see comment above.

Arghh, those cryptic user names, always tripping me up! ;-)

Also note that I wrote those to be 100% compatible with the current structure.

That's good. Excellent, in fact. I don't think we'll be looking too far ahead (you reference 3.5 for example) but severing plugins from direct access is indeed the next step. That said I doubt you'll see a refactoring of the underlying storage on a roadmap in the near future, as long as an abstraction layer does everything we need it to.

I was just trying to think through all the angles. It's not a perfect API because, for example when you look at the object in the debugger you don't see the menus and because it has to be refreshed, but it's fully abstracted. I guess it would/should be possible to add some functions that will build an object structure for those who want to see if via print_r(), etc.

Anyway, love feedback on this or to contribute in any way. I'm itching to provide some material contribution to core finally vs. the one line patch in v3.0. :)

comment:11 scribu4 years ago

  • Owner scribu deleted
  • Status changed from accepted to assigned

comment:12 follow-up: scribu4 years ago

Mike, how would you remove a single menu item with your API?

comment:13 in reply to: ↑ 12 mikeschinkel4 years ago

Replying to scribu:

Mike, how would you remove a single menu item with your API?

What I presented was a proof of concept and not a fully fleshed-out example. That said, I just spent time and updated to it provide a delete_admin_menu_item() to encapsulate the object calls and thus make it easier for the average guy (and I found and fixed a few bugs. :)

Here's the gist/download link:

https://gist.github.com/792b7aa5b695d1092520

So let's say you wanted to delete the "Editor" items for both "Appearance" and "Plugins" menu sections. The first one requires you to set a priority of 102 because the Editor item is added using a priority of 101:

add_action('admin_menu', 'my_admin_menu',102);
function my_admin_menu() {
  delete_admin_menu_item('Appearance','Editor');
}

The second one requires a regular expression because the Plugin menu section title has a <span></span> that follows it to display the number of plugins that need to be updated so I use a simple RegEx to match:

add_action('admin_menu', 'my_admin_menu');
function my_admin_menu() {
  delete_admin_menu_item('Plugins .*','Editor');
}

Another I could do these is to use the slugs, i.e.

add_action('admin_menu', 'my_admin_menu',102);
function my_admin_menu() {
  delete_admin_menu_item('themes.php','theme-editor.php',array(
  	'find_section_by' => 'slug',
  	'find_item_by'    => 'slug',
  ));
  delete_admin_menu_item('plugins.php','plugin-editor.php',array(
  	'find_section_by' => 'slug',
  	'find_item_by'    => 'slug',
  ));
}

As I write that last example I think maybe it would be better if I modified it to "sniff" out the distinction between title, slug, and/or index; i.e. look first to see if is_numeric(), if not then look to match slug, if not look to match title. Your thoughts?

BTW, I'd be happy to evolve of change this in whatever direction would be needed to use it in core, if potential. Let me know.

-Mike

comment:14 follow-up: scribu4 years ago

As I write that last example I think maybe it would be better if I modified it to "sniff" out the distinction between title, slug, and/or index; i.e. look first to see if is_numeric(), if not then look to match slug, if not look to match title. Your thoughts?

I think we should just use slugs. Indexes are what we're trying to get away from and titles are translatable.

As such, you wouldn't need the third parameter for delete_admin_menu_item().

comment:15 in reply to: ↑ 14 mikeschinkel4 years ago

Replying to scribu:

I think we should just use slugs. Indexes are what we're trying to get away from and titles are translatable.

Good point about indexes. When writing code for my own use they made sense, but as a potential inclusion into core I agree they probably aren't needed.

As such, you wouldn't need the third parameter for delete_admin_menu_item().

Agree.

That said, I'd hate to loose the titles even though translatable.

I'd expect 85% of use for this would be people developing custom themes for a specific client so they only ever have to test with one site, and 15% for use in commercial themes plugins. For plugins I think the slugs make much better sense but for the average theme developer I know it took me a while to wrap my head around how the slugs were used so I think the ease of use of titles would have a huge "usability" benefit.

For the titles we can of course test against the translated version too.

So for those who barely know what they are doing they can use the easy way with titles and those who know what they are doing can use the more robust and performant way with slugs. This could easily be documented as 1.) the easy way for single sites or 2.) the robust way for commercial themes and plugins, and we don't need the 3rd parameter because we can do a case-sensitive comparison for slug first then for title.

If you agree I can give it a go later today and refactor to simplify the code. Also, do you see other functions to implement?

-Mike

comment:16 follow-up: scribu4 years ago

Ok, I guess also looking at the title makes sense.

Looking forward to the cleaned up patch.

comment:17 in reply to: ↑ 16 mikeschinkel4 years ago

Replying to scribu:

Ok, I guess also looking at the title makes sense.

Looking forward to the cleaned up patch.

As promised, major positive changes. Your input helped me greatly to simplify it:

https://gist.github.com/792b7aa5b695d1092520

I removed the $args parameter and now lookups are done first via slug and if that fails then via Title. I did not (yet?) perform Title translations because I was thinking maybe we shouldn't; that way use of Titles would really only apply for specific sites.
I could possibly add a check for a constant something like 'FIND_ADMIN_MENUS_BY_TITLE' that must be set for Find-By-Title to work. That way they'd have to find the docs to learn how to use which could say to only use Titles for individual sites, not for distributed themes nor for distributed plugins.

One thing I did was recode it so that any properties or methods of the objects that might expose the internal structure became private. The idea is that the developer should not be able to use this API in a manner that would disallow us to later replace the underlying structure with something more rational (as you requested above) and not break the developers code. Because of this constraint I placed on the code architecture I had to write some code less than straightforward in one place but I think we'll greatly benefit down the road by this encapsulation.

Also, I used the terms "Menu Section" and "Menu Item" because AFAICT there hasn't been consistent naming and the names that have been used are unclear (at least to me.) If there are terms that others would prefer, I can change.

Here is some example use cases:

  // This example creates one menu in place of Dashboard called "My Menu", adds a few things, and removes all else.
  // This example assumes this might only be done for end users, not administrators

  $dashboard = rename_admin_menu_section('Dashboard','My Menu');
  delete_admin_menu_item($dashboard,'index.php');         // Dashboard
  delete_admin_menu_item($dashboard,'update-core.php');   // Updates
  
  $movies = "edit.php?post_type=movie";
  copy_admin_menu_item($dashboard,$movies);
  $movie_genre = 'edit-tags.php?taxonomy=movie-genre&post_type=movie';
  copy_admin_menu_item($dashboard,$movies,$movie_genre);
  rename_admin_menu_item($dashboard,$movie_genre,'Movie Genre');
  delete_admin_menu_item($movies);
  delete_admin_menu_item($movies,$movie_genre);
  delete_admin_menu_item($movies,'post-new.php?post_type=movie');
  delete_admin_menu_section($movies);
  
  $actors = "edit.php?post_type=actor";
  copy_admin_menu_item($dashboard,$actors);
  delete_admin_menu_item($actors);
  delete_admin_menu_item($actors,'post-new.php?post_type=actor');
  //delete_admin_menu_section($actors);
  
  rename_admin_menu_item($dashboard,'Pages','Other Pages');
  
  delete_admin_menu_section('edit.php');                  // Posts
  delete_admin_menu_section('upload.php');                // Media
  delete_admin_menu_section('link-manager.php');          // Links
  delete_admin_menu_section('edit-comments.php');         // Comments
  delete_admin_menu_section('edit.php?post_type=page');   // Pages
  delete_admin_menu_section('plugins.php');               // Plugins
  delete_admin_menu_section('themes.php');                // Appearance
  delete_admin_menu_section('users.php');                 // Users
  delete_admin_menu_section('tools.php');                 // Tools
  delete_admin_menu_section('options-general.php');       // Settings

This is definitely still an alpha; no hooks, no real code documentation, etc. but I wanted to get a working version up to get feedback. Let me know your thoughts.

-Mike

comment:18 follow-up: scribu4 years ago

The examples look great, but the code still needs to be compatible wth PHP4 for it to be included in WP 3.1.

comment:19 in reply to: ↑ 18 mikeschinkel4 years ago

Replying to scribu:

The examples look great, but the code still needs to be compatible wth PHP4 for it to be included in WP 3.1.

I haven't paid much attention to what isn't supported in PHP4; I can research but can you point out any specifics you see which our incompatible?

comment:20 follow-up: scribu4 years ago

These are the things that aren't supported in the antique version of PHP4:

  • static method calls: WP_AdminMenuSection::add($section,$args);
  • access modifiers for methods or properties:
    • private function add(...)
    • private $index;

comment:21 in reply to: ↑ 20 mikeschinkel4 years ago

Replying to scribu:

These are the things that aren't supported in the antique version of PHP4:

  • static method calls: WP_AdminMenuSection::add($section,$args);
  • access modifiers for methods or properties:
    • private function add(...)
    • private $index;

Figures. Much of the work I did was rearchitect so I could use private. Ah well.

New version up:

https://gist.github.com/792b7aa5b695d1092520

  • Attempts to make PHP 4.x compatible by removing static and private functions and vars (though I can't test against v4.0)
  • Added hookname property to both section and item
  • Renamed property with list of items to be "items" instead of "submenus"

comment:22 mikeschinkel4 years ago

BTW, it would be really nice if we could figure out a way to transition to using named identifiers instead of file/slug for identifiers. For example, this is not a really great identifier to have to deal with:

edit-tags.php?taxonomy=movie-genre&post_type=movie

One issue (of many) if the ambiguity regarding the need to HTML encode/decode the ampersands as well the general length and complexity of this. It would be soooo much better to be able to simply call this the`"movie-genre-tags"' menu item or similar.

comment:23 follow-up: scribu4 years ago

Ah, another "wonderful" thing about PHP4: objects are not passed by reference, but are cloned. That's why we have add_action_ref_array().

I think we should either:

  • add a big warning in the docs, signaling that this doesn't work properly in PHP4
  • punt to WP 3.2 and only commit #14666 in 3.1

Also, I used the terms "Menu Section" and "Menu Item" because AFAICT there hasn't been consistent naming and the names that have been used are unclear (at least to me.) If there are terms that others would prefer, I can change.

We do have a naming convention:

menu: a top-level item
submenu: an item inside a menu

Also, we generally use 'delete' when it's related to user data, like posts and comments. We use 'remove' for everything else: remove_action(), remove_theme_support() etc.

So, delete_admin_menu_section() would become remove_admin_menu() and delete_admin_menu_item() would be remove_admin_submenu().

comment:24 in reply to: ↑ 23 ; follow-up: mikeschinkel4 years ago

Replying to scribu:

Ah, another "wonderful" thing about PHP4: objects are not passed by reference, but are cloned. That's why we have add_action_ref_array().

Well that's a bit of a bummer. I'm starting to understand why people have been ranting about the switch for so long now.

If that's the case I say we add the privates back in and even the static method unless you see the latter as an issue?

I think we should either:

  • add a big warning in the docs, signaling that this doesn't work properly in PHP4
  • punt to WP 3.2 and only commit #14666 in 3.1

I'm good for either. What does the core team think about adding PHP5-specific code right now?

Also, we generally use 'delete' when it's related to user data, like posts and comments. We use 'remove' for everything else: remove_action(), remove_theme_support() etc.

Sounds good. I'll make those changes.

Also, I used the terms "Menu Section" and "Menu Item" because AFAICT there hasn't been consistent naming and the names that have been used are unclear (at least to me.) If there are terms that others would prefer, I can change.

We do have a naming convention:

menu: a top-level item
submenu: an item inside a menu

Ugh. Those terms were some of what caused me difficulty when I first tried to understand the menus, and hence why I used different terms. The "submenu" term is particularly problematic for me because AFAICT they are options, not (sub)menus. And even "menu" is not so good because isn't the entire thing a menu?

Proposal: Evolve these terms and make a break from the past? After all, we are talking about deprecating the old architecture, why not deprecate the terms too? Here are some potentials:

  • menu section, menu item
  • menu section, menu option


And there might be others?

I'll wait to make the "delete_*" -> "remove_*" changes until we decide on this.

Also, can I lobby for deprecating the external use of the existing functions and moving to these or something else that is $args based rather than 6 or 7 parameters? They include:

  • add_menu_page()
  • add_object_page()
  • add_utility_page()
  • add_submenu_page()
  • add_management_page()
  • add_options_page()
  • add_theme_page()
  • add_plugins_page()
  • add_users_page()
  • add_dashboard_page()
  • add_posts_page()
  • add_media_page()
  • add_links_page()
  • add_pages_page()
  • add_comments_page()
  • add_plugins_page()

The new functions could call these with an "undocumented" additional parameter that, if not passed, would trigger a deprecated warning when WP_DEBUG is set. Doing this could really streamline this otherwise very obtuse API. Thoughts?

-Mike

comment:25 in reply to: ↑ 24 ; follow-up: scribu4 years ago

Replying to mikeschinkel:

If that's the case I say we add the privates back in and even the static method unless you see the latter as an issue?

No can do, since that would cause syntax errors. So, I think we should go with #14666 for now.

comment:26 in reply to: ↑ 25 mikeschinkel4 years ago

Replying to scribu:

No can do, since that would cause syntax errors. So, I think we should go with #14666 for now.

As an alternate, or a stop gap?

comment:27 follow-up: scribu4 years ago

I don't know. We'll discuss it today at the dev meeting.

comment:28 in reply to: ↑ 27 mikeschinkel4 years ago

Replying to scribu:

I don't know. We'll discuss it today at the dev meeting.

I'm really motivated to get some of this included in core (even if that means v3.2.) I just want to feel like I've contributed real tangible value to core.

comment:29 10sexyapples4 years ago

Just tested Mike's latest github version on a site I'm currently building.
So great that now I can't live without it. Implemented.
Hugely liberating to an obsessive compulsive like myself to quickly be able to lose the unused fluff and reorganize in such a way that best benefits my clients. It would be great to see this implemented in 3.1 with an optional fail for PHP4.
+1

comment:30 mikeschinkel4 years ago

Made the change of function names from delete_*() to remove_*() as per @scribu:

https://gist.github.com/792b7aa5b695d1092520

comment:31 jjs_174 years ago

I just from Mike's class and I will be using it on every WordPress CMS install from here on out. I hope it's implemented into the core as soon as possible because the admin menu is extremely important for new WP users and the current set-up is very difficult to customize.

I think "menu section" and "menu item" should be used to describe $menu & $subMenu as it better describes what is shown to users in the admin area.

john

comment:32 follow-up: westi4 years ago

I'm happy that you find using github and gists good for collaboration but could you put snapshot patches on tickets in future so it is easy to follow the development in one place :-)

comment:33 in reply to: ↑ 32 mikeschinkel4 years ago

Replying to westi:

I'm happy that you find using github and gists good for collaboration but could you put snapshot patches on tickets in future so it is easy to follow the development in one place :-)

I'm still trying to get to the point where I understand how to efficiently create patches. Will move to that when as soon as I can.

comment:34 scribu4 years ago

  • Milestone changed from Awaiting Triage to 3.1

comment:35 scribu4 years ago

  • Milestone 3.1 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Sorry, wrong ticket. Closing, as per post-dev-meeting IRC discussion today.

comment:36 scribu4 years ago

  • Resolution changed from wontfix to fixed

(In [15753]) Introduce remove_menu_page() and remove_submenu_page(). Fixes #12718

comment:37 follow-up: johnjamesjacoby4 years ago

Why not dogfood the existing nav_menu cpt and make a new, internal one for the admin menus? Full and complete manipulation if you want it, and completely hidden from view if you don't.

comment:38 follow-up: scribu4 years ago

[15753] was supposed to reference #14666. Sorry, again.

comment:39 in reply to: ↑ 37 mikeschinkel4 years ago

Replying to johnjamesjacoby:

Why not dogfood the existing nav_menu cpt and make a new, internal one for the admin menus? Full and complete manipulation if you want it, and completely hidden from view if you don't.

After working with it a lot, I don't think the current nav_menu system is anywhere near mature enough to extend to another context. It should be given time to mature before tackling.

comment:40 in reply to: ↑ 38 mikeschinkel4 years ago

Replying to scribu:

[15753] was supposed to reference #14666. Sorry, again.

One thing my code implicitly addressed that we didn't cover in IRC (my bad) was moving to an$args based solution for what is currently add_menu_page() and add_submenu_page().

The fact that there 6 or 7 parameters that are hard to remember and must be called in the correct order makes the current API less approachable than if it were to use args instead.

Can we discuss that on this ticket before closure?

comment:41 scribu4 years ago

The original goal of this ticket was to not have to bother with separators.

So, I think a new ticket should be opened for passing an array of arguments, similar to #13937.

comment:42 jltallon3 years ago

  • Cc jltallon added

( related: #11517, #16342)

comment:43 voyagerfan57613 years ago

  • Cc WordPress@… added

comment:44 F J Kaiser3 years ago

  • Cc 24-7@… added

comment:46 scribu21 months ago

  • Milestone set to Awaiting Review
  • Resolution fixed deleted
  • Status changed from closed to reopened

This was not fixed.

I'm currently in the middle of refactoring all the admin menu related code. I'm not so sure anymore that another level of grouping is a good idea.

comment:47 scribu21 months ago

I'll be uploading a patch to this ticket soon. In the mean time: https://github.com/scribu/WordPress/compare/admin_menus

comment:48 scribu21 months ago

The reason I'm hesitating about getting rid of separators is that it adds another layer that you need to navigate through:

$admin_menu->get( 'dashboard' )->append( array( ... ) );

becomes

$admin_menu->get( 'object_group' )->get( 'dashboard' )->append( array( ... ) );

etc.

scribu21 months ago

comment:49 scribu21 months ago

  • Cc duck_ added
  • Keywords has-patch dev-feedback added
  • Milestone changed from Awaiting Review to 3.5

I think the patch is stable enough to warrant a first round of reviews.

You can get a feel for the current API by looking through the changes in wp-admin/menu.php as well as in wp-admin/includes/plugin.php

All the existing menu-related functions, such as add_menu_page(), remove_submenu_page() etc. work.

The meaning of the 'menu_position' arg has been changed. Previously, it was an arbitrary numeric index, which could overwrite other menu items - see #16946

Now, you pass to it a menu id. For example, here's how you would register a CPT that you want to show up above the Pages menu:

register_post_type( 'my-cpt', array(
  ...
  'menu_position' => 'pages'
) );

I also thought of deprecating 'menu_position' outright and replacing it with 'menu_before'.

Also, the reason it inserts menus before a given menu id and not after is because it's more intuitive when you have multiple post types:

register_post_type( 'cpt-1', array(
  ...
  'menu_position' => 'pages'
) );

register_post_type( 'cpt-2', array(
  ...
  'menu_position' => 'pages'
) );

The resulting order will be CPT 1, CPT 2, Pages. This is also consistent with the behavior of the revamped $menu_position parameter of add_menu_page().

Some unanswered questions:

  1. Currently submenu items don't have ids (they default to the slugs). Should they?
  1. There was an old 'custom_menu_order' filter, again numeric index based. Should we introduce a replacement? My impression is that with the improved API, it's not really needed anymore.

comment:50 scribu21 months ago

It seems I ignored the 'show_in_menu' arg from register_post_type(). Will fix it up soon.

scribu21 months ago

comment:51 martythornley21 months ago

  • Cc marty@… added

comment:52 scribu21 months ago

  • Description modified (diff)
  • Summary changed from Better structure for admin $menu to Better structure for admin menu

scribu21 months ago

comment:53 ryan21 months ago

I think custom_menu_order could go away. Few ever used it.

comment:54 scribu21 months ago

Closed #21315 as duplicate.

comment:55 follow-up: nacin21 months ago

After talking with ryan a bit on this, and doing some research, I'd like to bring up some backwards compatibility concerns, and possible solutions.

The most obvious way to be backwards compatible is to make both $menu and $submenu instances of ArrayObject, and watch any changes to them. This, however, is fraught with issues because ArrayObjects are not real arrays, and thus cannot go through array_* functions in PHP without issuing E_WARNINGS.

But, if we're going to end up breaking only few plugins — and the worst that happens is something they hacked in isn't quite what they wanted it to be, and the issue is a warning rather than a fatal error, then I can be okay with sticking to an ArrayObject watcher.

Even then, we found a number of very interesting hacks against the $menu global that could be caught with ArrayObject — iterating through it, then setting/unsetting things — but that class is going to be a doozy. I guess that's where I come in? :-)

Let's say we don't move on this until 3.6. I kind of want to drop-in a change to make $menu and $submenu simple ArrayObjects that mirror real array functionality of iteration and access, and then see what kind of bug reports we get during a beta period, before reverting back to standard arrays for 3.5. That could clean up quite a bit of custom code now.

So, attached are five different search results of the plugins directory: looking for global $menu (330 results), global $submenu (400 results), $GLOBALS['menu'] (20), $GLOBALS['submenu'] (20), and custom_menu_order (20). At some point, we should go through all of these — yes, all — and make an inventory of what kind of modifications people are making. If they are only doing things covered by ArrayIterator and ArrayAccess, great. If they are using array_* functions, then we need to make a note.

Ryan has also searched WP.com (which includes the VIP repo), and hasn't found too many bad situations, but nonetheless, there is plenty of code that a new API here could affect.

comment:56 in reply to: ↑ 55 mikeschinkel21 months ago

Replying to nacin:

After talking with ryan a bit on this, and doing some research, I'd like to bring up some backwards compatibility concerns, and possible solutions.

The most obvious way to be backwards compatible is to make both $menu and $submenu instances of ArrayObject, and watch any changes to them....

A big +1 on this, and really glad to see the attention on the admin menu system.

comment:57 scribu21 months ago

Marked #21408 as dup.

comment:58 Mamaduka21 months ago

  • Cc georgemamadashvili@… added

comment:59 azaozz21 months ago

This is OT here but perhaps we could implement some means of contacting the plugin authors directly when there's something in core that affects their plugin(s). Creating, refactoring or updating an API shouldn't be held back and made dependent on plugins that "do something wrong" with it...

Filtered the plugins that use global $menu by plugin name rather than file. There are 237 plugins in total. This will be reduced further by removing plugins by same authors.

So, as @nacin suggested, we could commit a new menu API but instead of limiting it (a lot) to keep it back-compat, we can email about 200 or so plugin authors and ask them to update their plugins.

comment:60 soficgr21 months ago

+1 really good work! I still look...

it's possible add last seperator if not exist at wp-admin/includes/menu.php

function _add_admin_menu_classes () {
....
	// Remove the last menu item if it is a normal separator.
	$last = end( $items );
	if ( 'wp-menu-separator' == $last->class ) {
		$admin_menu->remove( $last->id );
		array_pop( $items );
	}
	// Add last separator if not exist
	if('separator-last' != $last->class){
		$admin_menu->append( array(
			'id' => 'separator-last',
		'class' => 'wp-menu-separator',
		));
	}

I give the code because if $admin_menu have duplicated saparators at end you lost all saparators.

	$admin_menu->append( array(
		'id' => 'separator-4',
		'class' => 'wp-menu-separator',
	));
	$admin_menu->append( array(
		'id' => 'separator-last',
		'class' => 'wp-menu-separator',
	));	

also i think it's wasteful code if 1st check for "Remove any duplicated separators"
and after "Remove the last menu item if it is a separator" maybe can join together


The new admin menu class is really beautiful :D

comment:61 scribu21 months ago

I guess instead of actually removing the separators, we could just not render them.

scribu21 months ago

comment:62 scribu21 months ago

12718.4.diff:

  • rebase against current trunk (I hope I got the tabindex stuff right)
  • call _add_admin_menu_classes().

In general, I think we could merge all of includes/menu.php into wp-admin/menu-header.php or keep only functions in includes/menu.php and live code in menu-header.php.

scribu21 months ago

comment:63 follow-up: scribu21 months ago

12718.5.diff:

  • move function definitions from class-wp-admin-menu.php and includes/menu.php to includes/menu-functions.php
  • move private methods from WP_Admin_Menu to includes/functions.php
  • move high-level logic from wp-admin/menu.php into includes/menu.php

Oh, and needless to say, I like azaozz' proposal of asking plugin authors to start using the new API instead of doing crazy hacks to make the code 100% backwards-compatible.

comment:64 follow-up: scribu21 months ago

And there's a clear migration path for plugins:

global $admin_menu, $menu;

if ( $admin_menu ) {
  // use new $admin_menu API
} else {
  // hack on the old $menu global
}

comment:65 in reply to: ↑ 64 mikeschinkel21 months ago

Replying to azaozz:

This is OT here but perhaps we could implement some means of contacting the plugin authors directly when there's something in core that affects their plugin(s). Creating, refactoring or updating an API shouldn't be held back and made dependent on plugins that "do something wrong" with it...

I agree with this, with caveats...

Filtered the plugins that use global $menu by plugin name rather than file. There are 237 plugins in total. This will be reduced further by removing plugins by same authors.

So, as @nacin suggested, we could commit a new menu API but instead of limiting it (a lot) to keep it back-compat, we can email about 200 or so plugin authors and ask them to update their plugins.

If you consider the class of users most likely to make changes to the admin menu outside of that admin menu APIs, it's most likely not plugin authors because by nature most plugin authors want their plugins to continue to look and feel like WordPress. And end-user bloggers are typically not anal enough to spend their money on getting custom admin menus.

Instead I think it's highly likely that the people who have done the most direct manipulation of $menu and $submenu are custom site builders whose clients have requested the admin menus to be simplified in ways that the WordPress admin menu API does not currently support. I know our main client has had us do major surgery on the admin menu system and I've had many developers comment about similar needs on some of my WordPress Answers posts, so I expect there are many people who will indeed be in the same situation.

So if you break backward compatibility you are likely to create a lot of havoc for all those consultants who were in a rock-and-a-hard place who could not have achieved what their clients asked for any other way than to directly modify $menu and $submenu. Some of those consultants will likely be happy they get to bill their client's more through no fault of their own but many will be caught off guard and have to eat the cost of updates. And then there are the business end-users who if their site breaks will probably blame WordPress for it, giving ammunition to others in their organizations who want use a different CMS for their next website.

All that is to say that WordPress shouldn't break old implementations that use $menu and $submenu but instead draw a line in the sand between the "old way" and the "new way", as I think @scribu is suggesting here:

Replying to scribu:

And there's a clear migration path for plugins:

global $admin_menu, $menu;

if ( $admin_menu ) {
  // use new $admin_menu API
} else {
  // hack on the old $menu global
}

Which raises some concerns for various use-cases (I mention plugins but same goes for themes):

  1. New Installation of WordPress: Easy, use the new admin menu system.
  2. Install Old Plugin on Site using Newer Menus: User attempts to install a plugin that uses $menu and $submenu and is not using a plugin that uses the new approach: offer to downgrade the user but give them a warning and recommend they instead contact the plugin developer to upgrade.
  3. Upgrade Site using Older Menus to New WordPress Version: If there are no plugins using old menu system switch to the new one. If there are plugins using the old system tell the user and give them a choice to deactive and go with new system or keep plugins and use old system.
  4. Install Newer Plugin on Site using Older Menus: Tell the user they can't have both and ask which plugins they prefer to keep and suggest they contact the plugin author(s) to update.

As for detection, I'm sure at plugin install time the plugin installer can detect access to $admin_menu if implemented as an object (which appears to be the case), and the plugin installer could attempt to use a version of $menu and $submenu based on ArrayAccess() to test for usage, possibly using wp_remote_get() to load WordPress in a special way to see what happens. It wouldn't catch all situations because some plugins may selectively add menu pages and submenu pages based on $pagenow or some other criteria, but I expect it'll catch the vast majority. The other rare situations can just crash as occasionally happens with custom code.

In addition it workd be helpful to set up a pair of indicators: a get_option() that if it not set tells WordPress uses the new admin menu system and if set then specifies which to use, as well as a constant that could use code in /wp-config.php to force which admin menu system to use.

As an aside, my concern for backward compatibility is a concern for others. We will HAPPILY switch to a new admin menu system ASAP for all our client usage as soon as one is available; the current admin menu system is by far the biggest bane of our existence related to outstanding WordPress legacy architecture issues.

comment:66 in reply to: ↑ 63 ; follow-up: mikeschinkel21 months ago

Replying to scribu:

12718.5.diff:

  • move function definitions from class-wp-admin-menu.php and includes/menu.php to includes/menu-functions.php
  • move private methods from WP_Admin_Menu to includes/functions.php
  • move high-level logic from wp-admin/menu.php into includes/menu.php

Also, can I plead for deprecating the admin menu functions that take between 7 and 9 parameters and instead replace with ones that pass an array of name/value pair arguments? Specifically functions like these any maybe a few more?

  • add_menu_page()
  • add_submenu_page()
  • add_comments_page()
  • add_utility_page()
  • add_object_page()

And I'll be happy to help with any of the coding, I just need direction from those who need the help because "too many cooks spoil the broth." Unless I know what kind of help and how it'd just be a waste time to develop something that would ultimately conflict with what others are doing. For example, if deprecating those functions is blessed I'll be happy to do the work of deprecating them and creating new ones.

Version 1, edited 21 months ago by mikeschinkel (previous) (next) (diff)

comment:68 in reply to: ↑ 66 scribu21 months ago

Replying to mikeschinkel:

Also, can I plead for deprecating the admin menu functions that take between 7 and 9 parameters and instead replace with ones that pass an array of name/value pair arguments? Specifically functions like these and maybe a few more?,

  • add_menu_page()
  • add_submenu_page()
  • add_comments_page()
  • add_utility_page()
  • add_object_page()

Those functions do more than generate admin menus. They register the actual pages, so replacing them is outside the scope of this ticket, though definitely a very good candidate for refactoring as well: #15067

Last edited 21 months ago by scribu (previous) (diff)

comment:69 scribu21 months ago

More bugs with the current index system: #21445

comment:70 whiteshadow21 months ago

  • Cc whiteshadow added

scribu21 months ago

comment:71 scribu21 months ago

I tried running BuddyPress, but there's some capability error when accessing wp-admin/network/admin.php?page=bp-wizard. Need to fix that.

comment:72 scribu20 months ago

  • Milestone changed from 3.5 to Future Release

Since the core team is rightfully focusing on Media in 3.5, I'm not going to work on this any more for a while.

comment:73 DrewAPicture19 months ago

  • Cc xoodrew@… added

scribu19 months ago

comment:75 soficgr18 months ago

Notes: plugin like this admin-menu-editor will have a huge problem.

Feedback: I have test the code for 3 months(custom wp build) is very nice and workable. I have not found a problem. Is really good and flexible.

comment:76 stephenh198818 months ago

  • Cc contact@… added

comment:77 helenyhou18 months ago

#16342 was marked as a duplicate.

comment:78 wycks18 months ago

  • Cc bob.ellison@… added

+ 1 for a revisit , I have used the globals for this specific reason, and would be happy to delete that code in favor of something better. I agree with azaozz but I think some care should be taken here.

Maybe it can be tied into #21583 , screen options suffer from the same problems.(Ryan's comment, I could not find a specific trac for it )

Last edited 18 months ago by wycks (previous) (diff)

comment:79 nacin16 months ago

#22895 highlights why the admin menu code needs more than just an API rewrite. Important functions like user_can_access_admin_page() still rely on grotesque globals like $_wp_submenu_nopriv.

Mind you, this admin menu code isn't just building a menu, it also controls page rendering, and importantly, privilege checking to determine access. But, this lower-level code is completely unmaintainable spaghetti, and should be a target in an overhaul. We should really aim to start fresh. That's especially since the current API proposal might get modified/adapted based on what we learn from the lower-level bits.

When reviewing the patch, I was surprised there is nothing involving WP_Screen. Or perhaps this is not unexpected considering this currently just deals with a better structure for the menu itself. But any rewrite that goes further down the rabbit hole should probably bring WP_Screen into the mix.

comment:81 whiteshadow11 months ago

  • Cc whiteshadow removed

comment:82 whiteshadow11 months ago

  • Cc whiteshadow@… added

comment:83 swissspidy6 months ago

  • Cc hello@… added

comment:84 soficgr5 months ago

does not work separator cap :(

comment:85 mboynes3 months ago

  • Cc mboynes@… added

comment:86 nacin2 months ago

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

comment:87 nacin2 months ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

Note: See TracTickets for help on using tickets.