WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#19371 closed defect (bug) (fixed)

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

Reported by: willshouse Owned by: koopersmith
Milestone: 3.3 Priority: high
Severity: minor Version: 3.3
Component: Toolbar Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (7)

19371.diff (574 bytes) - added by scribu 4 years ago.
19371b.diff (475 bytes) - added by willshouse 4 years ago.
need to check for empty when removing in case the menu id does not exist
19371.2.diff (8.1 KB) - added by nacin 4 years ago.
19371.3.diff (8.3 KB) - added by nacin 4 years ago.
Refreshed after [19484]
19371.4.diff (8.2 KB) - added by nacin 4 years ago.
19371.5.diff (8.3 KB) - added by nacin 4 years ago.
patch.6.diff (300 bytes) - added by willshouse 4 years ago.
This is a patch to class-wp-admin-bar.php@19519 changing get_nodes to public from protected

Download all attachments as: .zip

Change History (45)

@scribu4 years ago

comment:1 @scribu4 years ago

  • Component changed from General to Admin Bar
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.3
  • Severity changed from normal to minor

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

comment:2 @scribu4 years ago

  • Version set to 3.3

comment:3 @willshouse4 years ago

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

comment:4 @willshouse4 years ago

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

@willshouse4 years ago

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

comment:5 @scribu4 years ago

Related: #19380

comment:6 @nacin4 years ago

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

comment:7 @nacin4 years ago

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.

comment:8 @willshouse4 years ago

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.

comment:9 @nacin4 years ago

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

comment:10 @ocean904 years ago

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?

comment:11 @nacin4 years ago

  • Keywords dev-feedback added

@nacin4 years ago

comment:12 @nacin4 years ago

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.

comment:13 @willshouse4 years ago

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

@nacin4 years ago

Refreshed after [19484]

comment:14 @nacin4 years ago

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.

@nacin4 years ago

comment:15 @nacin4 years ago

  • Priority changed from normal to high

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.

comment:16 @nacin4 years ago

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?

comment:17 @nacin4 years ago

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.

@nacin4 years ago

comment:18 @nacin4 years ago

  • Keywords commit added; dev-feedback removed

Marking for final review.

comment:19 follow-up: @willshouse4 years ago

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?

comment:20 in reply to: ↑ 19 @nacin4 years ago

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.

comment:21 follow-up: @willshouse4 years ago

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?

comment:22 in reply to: ↑ 21 @nacin4 years ago

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?

comment:23 @nacin4 years ago

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

comment:24 @nacin4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

comment:25 follow-up: @willshouse4 years ago

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 ); 
	}
}

comment:26 in reply to: ↑ 25 @nacin4 years ago

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.

comment:27 @willshouse4 years ago

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

comment:28 @ocean904 years ago

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

comment:29 follow-up: @willshouse4 years ago

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.

comment:30 in reply to: ↑ 29 @nacin4 years ago

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.

comment:31 @nacin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:32 @willshouse4 years ago

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.

comment:33 @koopersmith4 years ago

  • Keywords has-patch commit removed

@willshouse4 years ago

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

comment:34 @nacin4 years ago

  • Owner changed from nacin to koopersmith
  • Status changed from reopened to assigned

comment:35 @koopersmith4 years ago

  • Keywords has-patch added
  • Status changed from assigned to accepted

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.
  1. 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.
  1. Post-bind: Now we render the tree, stemming from the root node.

The patch on #19416 fixes a handful of things:

  • 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.

comment:36 @nacin4 years ago

  • Keywords commit added

comment:37 @willshouse4 years ago

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

comment:38 @ryan4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [19558]:

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

Note: See TracTickets for help on using tickets.