WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 months ago

#12718 reopened enhancement

Better structure for admin menu — at Version 52

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.

Change History (55)

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

Note: See TracTickets for help on using tickets.