#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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (46)
#1
@
13 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
#4
@
13 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!!
#7
@
13 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.
#8
@
13 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.
#9
@
13 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.
#10
@
13 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?
#12
@
13 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.
#14
@
13 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.
#15
@
13 years ago
- Priority changed from normal to high
- 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.
#16
@
13 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?
#17
@
13 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.
#19
follow-up:
↓ 20
@
13 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?
#20
in reply to:
↑ 19
@
13 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.
#21
follow-up:
↓ 22
@
13 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?
#22
in reply to:
↑ 21
@
13 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?
#23
@
13 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.
#25
follow-up:
↓ 26
@
13 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 ); } }
#26
in reply to:
↑ 25
@
13 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.
#27
@
13 years ago
OK, sounds good. Didn't see your "second though" above. I will test with the latest patch.
#29
follow-up:
↓ 30
@
13 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.
#30
in reply to:
↑ 29
@
13 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.
#32
@
13 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.
@
13 years ago
This is a patch to class-wp-admin-bar.php@19519 changing get_nodes to public from protected
#35
@
13 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:
- Pre-bind: This is where we use
add_node()
and friends. Nodes are represented as a flat list.
- 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.
- 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()
andget_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.
One solution would be to make remove_menu() return the args. Example usage:
See 19371.diff