As of 3.3beta2 WP_Admin_Bar no longer provides a way to access attributes of existing menu items

[willshouse]

Up until around 3.3beta1 items in the WP_Admin_Bar Object could be accessed using this type of syntax, for example to change the CSS class of one of the existing menu items:

$wp_admin_bar->menu->{'wp-logo'}['meta']['class'] = 'new-class';

When running print_r($wp_admin_bar) the output looked something like this:

WP_Admin_Bar Object
(
	[menu] => stdClass Object
		(
			[my-account] => Array
				(

However, around version 3.3beta2 the above syntax for changing a menu item's CSS class no longer works, and the output from print_r($wp_admin_bar) reveals a different structure for that object:

WP_Admin_Bar Object
(
	[nodes:WP_Admin_Bar:private] => Array
		(
			[my-account] => stdClass Object
				(
					[id] => my-account

				)

I realize that Wordpress may not want me fiddling with the menus this way, and if there was a more standardized way to do this I would love to use it, but as far as I know there are only two functions that are available to modify the admin bar, add_menu_item and remove_menu_item, and these do not give the flexibility to do things like changing the attributes of existing menu items.

To confirm, I looked at wp-includes/class-wp-admin-bar.php it is clear that Wordpress has changed the way they define the variables.

Old Class

class WP_Admin_Bar {
	var $menu;
	var $proto = 'http://';
	var $user;

New Class

class WP_Admin_Bar {
	private $nodes = array();
	private $root = array();

	public $proto = 'http://';
	public $user;

So basically the problem is that there is no longer any way to access attributes for existing menu items in the WP admin bar unless I make changes to the WP core files myself.

BTW I submitted this to Stack Exchange for suggestions ( ​http://stackoverflow.com/questions/8286798/ ) and the response was that the private variables should be protected variables. At least that would give me something to work with.

[scribu]

One solution would be to make remove_menu() return the args. Example usage:

add_action( 'admin_bar_menu', function( $admin_bar ) {
	$node = $admin_bar->remove_menu( 'user-info' );

	$node['meta']['class'] = 'foo';

	$admin_bar->add_menu( $node );
}, 99 );

See 19371.diff​

[willshouse]

Not a bad solution. I will give this a try. Thank you Mr. Scribu.

[willshouse]

Ok, this works very well! Please roll this into the 3.3 RC! A much better way to interact with the admin bar!!

[willshouse]

need to check for empty when removing in case the menu id does not exist

[scribu]

Related: #19380

[nacin]

My current thoughts:

• get_node() should return the node, or false on failure.

• remove_node() should return get_node() in addition to unsetting.

• The stuff in 'meta' should be promoted to first-level properties.

• The late binding code should be moved to a separate protected method, rather than being in render.

• I'm torn on private versus protected. Was leaning toward private for the properties, but protected for the render_* methods. Could go with both going to protected.

[willshouse]

Your note about the stuff in 'meta' being promoted to first-level properties reminded me that it looks like 'children' is also no longer being utilized.

[nacin]

It is utilized, but for the late bindings. We should attempt to add that later as well, rather than including it in the node.

[ocean90]

Cross post from #19136:

It would be nice to be able to write a custom render method again. Both methods are declared now as private. Can we change it to protected or public again?

[nacin]

19371.2.diff​ is a full reshuffling. It limits the nodes property to a select few methods, and allows surrounding methods to be overridden. Late bindings are abstracted, and there is some attempt to make a render() method overridden in 3.2 to continue to work, given __get(). (Untested.)

It would be nice if _bind(), when called, did a reset, and re-bound everything. Then there would be no need for _bound.

get_node() and get_nodes() are both added.

I don't see much of this making it into 3.3, but there are some good bits we could steal.

End goal: Keep it flexible for plugins without putting our own feet in cement.

[willshouse]

that is looking quite good. thank you for your work on this.

[nacin]

Refreshed after [19484]

[nacin]

This is going to take too long to get right.

I suggest:

public function recursive_render() calls _render_item()

public function render() calls _bind() and _render()

protected function _bind() binds the items.

protected function _render() calls final protected functions _render_item() and _render_group()

We leave the namespaces render_item() and render_group() open. Ideally, these will become overridable in the future.

If you were already overriding render() and recursive_render(), things will work fine. If you were completely replacing WP_Admin_Bar (subclassing is not a requirement, for better or worse), things will work fine.

We could choose to not finalize _render_item() and _render_group(). Some code in _render_group() will need to be moved to _bind().

Finally, one thing the existing patches do not touch, is making sure that $this->menu returns back compat code. This should not be exceedingly difficult.

[nacin]

19371.4.diff​:

• Anything that deals with the nodes property is private or final. _set_node(), _unset_node(), get_node(), and get_nodes(). This way, at least our internal storage is protected from plugins.

• add_group() is a new wrapper, so it gets a final.

• _bind() is finalized too. If you want to mess with this, override render() and do the whole thing yourself.

• _render(), _render_item(), _render_group() are all protected but overridable.

• recursive_render() is deprecated. It is back compat and simply wraps _render_item().

One more pass is necessary:

• Ideally, koopersmith can move some of the group binding stuff from _render_group() to _bind().

• I don't think there's enough to ensure that $this->menu is back compat. Goal should be that it works in a read-only way.

[nacin]

Talking with koopersmith, I'm having a hard time coming up with a reason for _render(), _render_item(), and _render_group() to *not* be final. If someone really wanted to re-do the rendering, they should just override render() and be done with it. Thoughts?

[nacin]

Talked with ocean90, he's fine with finalizing the rendering functions. Additional flexibility in the API (such as how 'html' now works) makes most modifications unnecessary; and render() is still overridable.

We're not going to maintain compat for $menu. Suggest we make it array() and plugins will have to deal. We can throw a _deprecated_argument() and push them to use the new methods.

[nacin]

Marking for final review.

[willshouse]

I'm new to participating in trac (sorry). Should I be applying all of the patches sequentially or should I be applying just the latest one?

[nacin]

Replying to willshouse:

I'm new to participating in trac (sorry). Should I be applying all of the patches sequentially or should I be applying just the latest one?

Just the latest one. Patches are almost always going to be based off of trunk. Incremental patches are too burdensome.

[willshouse]

Ok, if that's the case I'm not seeing where scribu's changes are being included in the latest - 19371.5.diff. For example if I take the current version in the trunk and apply the 19371.5.diff patch I'm still seeing that the remove_menu function does not return the node that is being removed:

public function remove_menu( $id ) {
	$this->remove_node( $id );
}

Is that a mistake that needs to be added into the latest diff or am I missing another way of editing the existing menu attributes?

[nacin]

Replying to willshouse:

Ok, if that's the case I'm not seeing where scribu's changes are being included in the latest - 19371.5.diff. For example if I take the current version in the trunk and apply the 19371.5.diff patch I'm still seeing that the remove_menu function does not return the node that is being removed:

public function remove_menu( $id ) {
	$this->remove_node( $id );
}

There's a return missing. Easy fix.

Is that a mistake that needs to be added into the latest diff or am I missing another way of editing the existing menu attributes?

[nacin]

Second thought: Since we have get_node(), I'm going to avoid remove_node() returning anything. At least until/unless these become chainable.

[nacin]

In [19501]:

Finalize the WP_Admin_Bar architecture for 3.3.

• Introduce a get_node() method for plugins.
• Deprecate $wp_admin_bar->menu. Plugins will need to use get_node(), remove_node(), add_node() to make modifications. This finalizes a backwards incompatible change made earlier in the cycle.
• Allow add_node() to take a node object (which could come from get_node(), then be modified).
• Ensure that our underlying storage (the nodes property) is private to core. Introduce _set_node, _unset_node, _get_nodes, get_nodes as the only ways to interface with this.
• Protect and finalize _render_item, and _render_group. render() remains public and technically overridable, though I would discourage this of plugin authors.
• Deprecate recursive_render(). Use render() or _render_item().

More about the internals:

• Late-binds a node's 'children' array.
• Eliminates the root property, leverages a 'root' node.
• Splits render() into _bind() and _render(), both protected and finalized.

Fixes #19371.

[willshouse]

We are also missing the other part of scribu's contribution (and my !empty part)

public function remove_node( $id ) {
	if(!empty($this->nodes[ $id ])){
		$node = $this->nodes[ $id ]; 
		unset( $this->nodes[ $id ] ); 
		return get_object_vars( $node ); 
	}
}

[nacin]

Replying to willshouse:

We are also missing the other part of scribu's contribution (and my !empty part)

I addressed this above:

Second thought: Since we have get_node(), I'm going to avoid remove_node() returning anything.

It's not a typical construct for us to return something from a removal function. While it's clever, I think it is more clear to rely on get_node(), now that we have it.

[willshouse]

OK, sounds good. Didn't see your "second though" above. I will test with the latest patch.

[ocean90]

willshouse, you doesn't need to apply a patch, just use RC1. It's all in.

[willshouse]

I think we should possibly change "get_nodes" to be a public function. If we don't change it to a public function, what is the proper method for removing all child items from a node?

For example, if I wanted to remove all child items from the "wp-logo" menu, I could do this:

remove_action( 'admin_bar_menu', 'wp_admin_bar_wp_menu', 10 );

And then add back in the top level menu item:

$wp_admin_bar->add_menu( array(
	'id'    => 'wp-logo',
	'title' => '<span class="ab-wp-logo"></span>',
	'href'  => admin_url( 'about.php' ),
) );

However that causes an error to be thrown:

Warning: Invalid argument supplied for foreach()
in class-wp-admin-bar.php on line 252

So can we either change "get_nodes" to a public function or can someone let me know the best way to remove all child items (and child "groups") ?

ps: I think it would be OK if get_nodes was a public function. Since it is just pulling data it won't affect the internal storage.

This way, at least our internal storage is protected from plugins.

[nacin]

Replying to willshouse:

I think we should possibly change "get_nodes" to be a public function. If we don't change it to a public function, what is the proper method for removing all child items from a node?

If I had to guess, orphaning them. But that's not a very good answer, I know. koopersmith, thoughts?

So can we either change "get_nodes" to a public function or can someone let me know the best way to remove all child items (and child "groups") ?

ps: I think it would be OK if get_nodes was a public function. Since it is just pulling data it won't affect the internal storage.

I could get behind that. It should be noted, though, that get_nodes() generally returns a flat list of nodes, but that it can also return a mixture of a tree and a flat list once bound. Once _bind() is called, $this->nodes gets poisoned pretty well. If we can make it so get_nodes() always returns a flat list of nodes that can later be bound into a tree, I think we're in good shape.

For compat reasons, we may need to additionally unset( $argschildren? ) in add_node(), and make _bind() able to run more than once to allow for a re-binding of all nodes.

[willshouse]

It should be noted, though, that get_nodes() generally returns a flat list of nodes

What you mean when you say that it generally returns a flat list of nodes? This is the output I'm looking at:

add_action( 'admin_bar_menu', function( $admin_bar ) {
	$z = $admin_bar->get_nodes();
	print_r($z);
},11);

Array
(
   [root] => stdClass Object
   (
    [id] => root
    [title] => 
    [parent] => 
    [href] => 
    [group] => 
    [meta] => Array
     (
     )

   )

   [wp-logo] => stdClass Object
   (
    [id] => wp-logo
    [title] => <span class="ab-icon"></span>
    [parent] => 
    [href] => http://nightly.dev/wp-admin/about.php
    [group] => 
    [meta] => Array
     (
      [title] => About WordPress
     )

   )
... (items removed for brevity)...
)

I do like your idea of a "flat" list but I think an array or object might be better as that would show the parent / child relationship. I have no problem with the print_r type of array object that is above, but a simpler nested array of id's would also be fine.

If I had to guess, orphaning them.

The only problem is that if you want to remove child nodes, and you orphan them by removing their parent menu item, and then add back a parent menu item with the same ID, the child nodes still show up. So possibly we do need to look at doing the unset( $argschildren? ), and maybe also unsetting the group argument too?

Mostly I would be very happy if "get nodes" gets changed to a public function before 3.3 is released.

[willshouse]

This is a patch to class-wp-admin-bar.php@19519 changing get_nodes to public from protected

[koopersmith]

Okay! Simply making get_nodes() public or protected is not a reasonable solution without a few other changes — it leaves too many doors open.

There are three stages within WP_Admin_Bar:

1. Pre-bind: This is where we use add_node() and friends. Nodes are represented as a flat list.

2. Binding: Here, we transform the list of nodes into the tree that we'll use to render the toolbar. This should be contained entirely within the internal _bind() method.

3. Post-bind: Now we render the tree, stemming from the root node.

The ​patch on #19416 fixes a handful of things:

• #19416 (of course).

• get_nodes() is now public.

• add_node() is now the canonical way to change a node's value. get_node() and get_nodes() return clones of each node.

• The binding logic to create containers moved from _render_group() to _bind(). This is critical and makes rendering much simpler.

• The various node-manipulation functions short-circuit after _bind() is called. The $root node (returned by _bind()) becomes the only way of manipulating the tree.

This hardens the three stages, and will make working with the new admin bar API considerably less buggy.

[willshouse]

Have tested and it is working very well. Much less buggy now, thank you for the hard work.

[ryan]

In [19558]:

Admin bar API improvements. Props koopersmith. fixes #19416 #19371

[hellofromTonya]

In 55580:

Code Modernization: Fix dynamic properties in WP_Admin_Bar.

To fix the dynamic properties, the following changes are included:

• Removes WP_Admin_Bar::__get().
• Declares menu as a property on the class, deprecates it, and initializes it to an empty array.
• Removes the unused 'proto' dynamic property.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

Why remove the WP_Admin_Bar::__get() magic method?

tl;dr
The magic method is no longer needed.

The magic method only handled the menu and proto dynamic properties. Introducing a full set of magic methods is overkill for this class. Instead of having to maintain magic methods, this changeset instead directly addresses the 2 properties (see below).

Why declare the menu property on the class?

tl;dr
To simplify the code while maintaining backwards compatibility for extenders who are using this deprecated property.

The menu property was introduced during the 3.1.0 development cycle as a declared property [15671]. Its purpose was to internally hold the menu structure.

During the WP 3.3.0 development cycle, it was replaced by a new private property called nodes (see [19120]).

But breakage reports from extenders caused it to be restored. [19501] added the __get() magic method, i.e. for handling it as a dynamic property, and deprecated it.

We're not going to maintain compat for $menu. Suggest we make it array() and plugins will have to deal. We can throw a _deprecated_argument() and push them to use the new methods.

~ Source: see #19371 comment 17

​A search of the wp.org plugins and themes repository shows that a few plugins are still using this deprecated property. To maintain backwards compatibility, menu is moved back to the class as a declared property, set to an empty array (as it's been since 3.3.0), and deprecated in the property's description.

Why remove the proto dynamic property?

tl;dr

• It was not intended to be released in 3.1.
• There are no usages of it in Core or in the WP.org's plugin or theme directories.
• It should be safe to remove.

This property was first introduced in the WP 3.1.0 development cycle to replace the PROTO constant (see [16038]) for protocol handling for the admin bar's hyperlinks. [16077] replaced the property's usages with URL functions such as get_admin_url() and admin_url(). But it missed removing the property, which was no longer needed or used.

It was relocated to the __get() magic method as a dynamic property when the menu property was restored (see [19501]).

A search of WP.org's plugins and themes repositories shows no usages of the property. Core hasn't used it since the removed in [16038] before 3.1 final release. It should be safe to remove it, but committing very early in the 6.3 alpha cycle to give time for reports of usages, if there are any.

References:

• A ​live open public working session where these changes were discussed and agreed to.
• ​PHP RFC: Deprecate dynamic properties.

Follow-up to [19501], [19120], [16308], [16038], [15671].

Props antonvlasenko, hellofromTonya, jrf, markjaquith, desrosj, ironprogrammer, peterwilsoncc, SergeyBiryukov.
See #56876, #56034.