Secondary items should be a flag for the admin bar

[nacin]

Follow-up to #18197, 18197#comment:64, [19120], and an IRC discussion.

The current setup is clever, but doesn't scale and isn't flexible (for plugins, especially).

[nacin]

Also fixes 18197#comment:88.

[nacin]

Also test with 18197#comment:122.

[nacin]

A wpdevel post coming out of #18880 depends on this ticket.

[koopersmith]

Patch builds secondary menus into the core admin bar class. Late binding allows items to be registered to either the primary or secondary menu through the use of the boolean secondary parameter (in add_menu/add_node).

[koopersmith]

In [19230]:

Add secondary flag to admin bar. fixes #19136.

[nacin]

In [19236]:

Promote secondary admin bar items to primary if there are no primary items at time of render. see #19136, #19221.

[koopersmith]

There are a bunch of issues with the secondary flag. The main problem is that in the case of a secondary menu, setting a flag on each item is far worse than having one common parent.

A good example use case is BuddyPress — BP adds a top level admin bar menu, and provides plugins with a variable to implement their own submenus. If BP wanted to change its menu to become a secondary component of another menu, it would have to update every submenu item with the secondary flag (instead of just changing the parent). The larger issue here is plugins: every single plugin that added a BuddyPress admin submenu would have to update their items with the secondary flag.

The ideal solution is using groups, which would allow us to split submenus into sections. In the case of the BuddyPress menu, it could be converted to a group and be easily styled. No plugins would have to be updated, as the parent ID would still work.

[azaozz]

I was with the impression that the secondary flag would be only for second level menus, i.e. all "top > submenu.secondary" will behave exactly like "top > submenu" except they will be outputted at the bottom and have slightly different styling. All children of submenu.secondary would inherit the styling.

Not sure another UL was needed there at all. Seems to just make things complicated and more fragile.

So the secondary flag would be valid only on the first level of submenus "top > submenu", and ignored on any sub-submenus, etc. So if a plugin decides to move it's menu and adds the flag, all children would inherit the styling, no additional flags needed.

[johnjamesjacoby]

Replying to azaozz:

I was with the impression that the secondary flag would be only for second level menus, i.e. all "top > submenu.secondary" will behave exactly like "top > submenu" except they will be outputted at the bottom and have slightly different styling. All children of submenu.secondary would inherit the styling.

This is still how it works with groups3.diff except "levels" are "groups" and there is no upper limit to the number of groupings. Previous to this patch, there were only two "types" of menu stylings, and no way to reliably group them by ID. See below:

Not sure another UL was needed there at all. Seems to just make things complicated and more fragile.

If it's the UL I think you're seeing, it is needed to maintain the styling and parentage associated with groups.

So the secondary flag would be valid only on the first level of submenus "top > submenu", and ignored on any sub-submenus, etc. So if a plugin decides to move it's menu and adds the flag, all children would inherit the styling, no additional flags needed.

The "secondary flag" is a poor way to architect this, and actually perpetuates the problem you mention here. It limits any ability to reliably move groups of items in the future, and is a work-around approach similar to the rabbit-hole of the current main admin-nav.

Implementing "true" groups rather than mimic them (numerically or otherwise) allows us to designate physical menu groups with valid and unique identifiers to *know* that a group of items always contain its children regardless of where it might ever be relocated in the future, up to and including anything that a plugin might choose to add anywhere in the binding process.

---

After review, this patch is actually considerably smaller than it looks, and touches very few guts of the core API itself.

[koopersmith]

In [19429]:

Admin Bar: Secondary is so passé. Groups are the new black. fixes #19136.

[ocean90]

It would be nice to be able to write a custom render method, but both methods are declared as private. Could we change it to protected or public again?

[nacin]

Replying to ocean90:

It would be nice to be able to write a custom render method, but both methods are declared as private. Could we change it to protected or public again?

Carry this into #19371.