Docs: Improve docs to include recommendations for changing existing admin bar menu items

[michaelwp85]

Scope of this ticket

Improve docs to add recommendations for customizing / changing existing items

This is necessary as [58748] changeset moved the user menu to a higher priority level (of 9991) to achieve:

so nodes added with higher priorities will still be shown visually before the user and recovery menu items, as they were prior to [58215].

When hooking into 'admin_bar_menu' with a priority level less than 9991 and attempting to get the 'my-account' node, null is returned. While the preferred approach is to hook into 'wp_before_admin_bar_render', the documentation of the 'admin_bar_menu' action is misleading.

This ticket aims to provide clear recommendations to help guide developers.

Original report that drove this change

Getting admin bar node "my-account" returns null after updating to 6.6.1

<?php
add_action('admin_bar_menu', function (WP_Admin_Bar $adminBar): WP_Admin_Bar {
  var_dump($adminBar->get_node('my-account'));
}

6.6.0:

$adminBar->get_node('my-account'))

{#3866 ▼
  +"id": "my-account"
  +"title": "Howdy, <span class="display-name">__ACCOUNT__</span><img alt='' src='https://secure.gravatar.com/avatar/....?s=26&#038;d=mm&#038;r=g' ▶"
  +"parent": "top-secondary"
  +"href": "https://core.test/wp/wp-admin/profile.php"
  +"group": false
  +"meta": array:3 [▶]
}

$adminBar

WP_Admin_Bar {#3349 ▼
  -nodes: array:4 [▼
    "user-actions" => {#3362 ▶}
    "user-info" => {#3546 ▶}
    "logout" => {#3545 ▶}
    "my-account" => {#3893 ▶}
  ]
  -bound: false
  +user: {#3348 ▶}
  +menu: []
}

6.6.1:

$adminBar->get_node('my-account'))

null

$adminBar

WP_Admin_Bar {#3352 ▼
  -nodes: array:3 [▼
    "user-actions" => {#3365 ▶}
    "user-info" => {#3549 ▶}
    "logout" => {#3548 ▶}
  ]
  -bound: false
  +user: {#3351 ▶}
  +menu: []
}

Trac ticket: https://core.trac.wordpress.org/ticket/61738

[narenin]

Hi,

I have tried to replicate this and found that this is happening because of the changes introduced in https://core.trac.wordpress.org/ticket/61615

In that we have changed the priorities which is causing this issue.

[michaelwp85]

Replying to narenin:

Hi,

I have tried to replicate this and found that this is happening because of the changes introduced in https://core.trac.wordpress.org/ticket/61615

In that we have changed the priorities which is causing this issue.

Hi Narenin,

I haven't participated in bug reporting/fixing yet (this is my first report).
Seeing this change I can't understand from a semver perspective how this could have been added as a patch previously. This seems like a breaking change to me which should only have gone into a major release. Looking at your patch, you reverted the priorities to the old values?

Am I correct or would this be something that could be changed in a minor/patch release?

[mukesh27]

Should we re-open #61615 and close these one? As it's revert of that ticket

What do you think @hellofromtonya @joedolson

[hellofromTonya]

Replying to mukesh27:

Should we re-open #61615 and close these one? As it's revert of that ticket

Let's use this ticket for investigating why this is happening and if an adjustment or additional change is needed to fix this specific issue.

If it's determined that reverts of [58748] and [58759] are the only or best remedy, then #61615 should reopened after the revert (which should reference this ticket) so that work can continue on its bug.

cc @joedolson @sabernhardt

[hellofromTonya]

This is a copy of @rabmalin issue report from the original ticket

I have this in my theme:

function theme_slug_replace_howdy( $wp_admin_bar ) {
	$my_account = $wp_admin_bar->get_node( 'my-account' );
	$wp_admin_bar->add_node(
		array(
			'id'    => 'my-account',
			'title' => str_replace( 'Howdy,', 'Hello,', $my_account->title ),
		)
	);
}

add_action( 'admin_bar_menu', 'theme_slug_replace_howdy', 25 );

After 6.6.1 release I am getting following PHP warnings.

PHP Warning:  Attempt to read property "title" on null in /Users/nilambarsharma/Sites/staging/app/public/wp-content/themes/khai/functions.php on line 45
PHP Deprecated:  str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in /Users/nilambarsharma/Sites/staging/app/public/wp-content/themes/khai/functions.php on line 45

I noticed priority of this hook has been changed in the above commit. PHP notice is fixed when I keep priority 9999 for my custom hook in my theme. Just reporting here because I am not sure whether this issue should be fixed in core or in my theme.

[hellofromTonya]

In investigating why the 'my-account' node is returning null, here's what I found:

The node is added when wp_admin_bar_my_account_item() callback runs.

Prior to 6.6.1, the node was added at a priority level of 7.
With 6.6.1, the node is added at a priority level of 9991.

The 6.6.1 then requires custom hooks to modify their priority level to 9991 or higher, as this is when the node is available. Looking at the intent of change in #61615, this is by design (see the ticket more details).

The 6.6.1 then requires changes in custom implementations. A dev note could help to communicate the change. For example, the [58215] priority level change was ​communicated in the 6.6 Miscellaneous developer dev note.

That said, I do think it's worth revisiting the intent of #61615 with what's now known as a confidence check for the changes made. Do the changes still make sense? If yes, then a dev note is needed. Else, a partial revert is likely needed.

@sabernhardt @joedolson what do you think?

[pbiron]

I would argue that calling $wp_admin_bar->get_node() in a function hooked to admin_bar_menu is _doing_it_wrong().

@rabmalin A much more appropriate hook to use would be ​wp_before_admin_bar_render. As it says in the DocBlock of that hook:

The wp_before_admin_bar_render action allows developers to modify the $wp_admin_bar object before it is used to render the Toolbar to the screen.

[joedolson]

I'm inclined to agree with @pbiron, that the example code isn't the best way to do this. That said, the documentation for the action does specifically say that the hook is used to add, remove, or manipulate admin bar items, so it's reasonable that developers might be doing this.

That said, I think it's pretty normal that any action needs to be added at a priority later than the code to be manipulated, so this is working as expected.

It probably should have a dev note, but I don't feel it would be beneficial to revert. If we revert this change, then potentially a larger number of developers will choose to adjust their insertion priorities so that their menu items insert to the left of the user menu node. Leaving this in place just requires developers who are manipulating the user menu or the search nodes to change their priority levels, which I expect is a smaller number of cases.

[sabernhardt]

Directory searches for get_node with my-account did not match any themes but found ​47 plugins. (Not all are available or in use, and some might be at a higher priority already.)

[sabernhardt]

#61757 was marked as a duplicate.

[sabernhardt]

I was not aware of wp_before_admin_bar_render, but that seems to filter existing node content well.

To replace "Howdy" in the title, simply increasing the priority for the hook is a quick way to remove the error in 6.6.1 and to continue seeing a change in the visible text. However, the example is not thorough anymore, and it could be more defensive.

1. The sub-menu recently added an ARIA label with the same text, using menu_title in the meta array.
2. Switching the hook to wp_before_admin_bar_render (with the $wp_admin_bar global variable inside the function) can edit the text without defining a priority. My tests of that hook were successful with Core's wp_admin_bar_my_account_item priority at 7, 9991 and 99999999.
3. The custom function could check at least whether the result of get_node is set before trying to edit part of the object. A similar ​snippet from WPCode quits early if it does not find $my_account->title. Technically, another plugin (or some other code) might remove or replace the node that Core adds.

/**
 * Replaces the "Howdy" text in the WP admin toolbar.
 *
 * @global WP_Admin_Bar $wp_admin_bar The WP_Admin_Bar instance.
 */
function replace_howdy_in_admin_bar() {
	global $wp_admin_bar;

	$my_account = $wp_admin_bar->get_node( 'my-account' );

	// Return early if node contents are not available to edit.
	if ( ! isset( $my_account->title ) || ! isset( $my_account->meta['menu_title'] ) ) {
		return;
	}

	$wp_admin_bar->add_node(
		array(
			'id'    => 'my-account',
			'title' => str_replace( 'Howdy,', 'Hello,', $my_account->title ),
			'meta'  => array(
				'menu_title' => str_replace( 'Howdy,', 'Hello,', $my_account->meta['menu_title'] ),
			),
		)
	);
}
add_action( 'wp_before_admin_bar_render', 'replace_howdy_in_admin_bar' );

Of course, the documentation needs updating:

• The wp_admin_bar_render DocBlock says that admin_bar_menu adds menus at the "most optimal point, right before the admin bar is rendered." It may be the best hook for adding new menus and/or replacing them, but it does not fire immediately before rendering the admin bar.
• The DocBlock for wp_before_admin_bar_render only says it "Fires before the admin bar is rendered." The additional information is at least available on the ​DevHub page, though not in the DocBlock.
• For manipulating existing nodes, or removing them, wp_before_admin_bar_render seems to be the better choice.

My biggest concern with the error is that any visitor who has logged in can see it, but only site admins and/or plugin authors can do something to fix it.

[sabernhardt]

proof of concept: resets all three nodes to their original priorities, and repositions them together with the same function at 9999

[sabernhardt]

I am not fond of 61738.reposition-all.diff​, but it is another way to move the Core nodes to the end of their group.

• That would restore the established functions' priority values as they were in 6.5: 4, 7 and 8.
• As a single function at priority 9999, wp_admin_bar_reposition_secondary_nodes does not leave room for targeting a spot between its elements (as I had wanted between 9992 and 9999).

And I'm sure the code could be cleaner :)

[hellofromTonya]

Summary from today's 6.6.2 bug scrub discussion:

Though there are a lot of plugins hooked into 'admin_bar_menu', there are only a couple of reports of issues, i.e. this ticket and ​one support forum post that was resolved by increasing the priority level.

Consensus with @jorbin @costdev and me agrees with @joedolson assessment that reverting the change will have a higher risk of impacting more users.

This ticket though identified the lack of dev note and an opportunity to improve the docs for 'admin_bar_menu' to recommend using 'wp_before_admin_bar_render' instead.

Consensus from the scrub to resolve this ticket:

1. Leave the changes, i.e. do not revert nor patch.
2. Publish a dev note for the priority level changes.
3. Create a patch to improve the docs for 'admin_bar_menu'.

Updating the keywords accordingly.

[michaelwp85]

I can confirm switching to the wp_before_admin_bar_render hook resolves my issue.

Changes needed in my scenario:

• change hook
• remove $wp_admin_bar from the callback (this is not available in the wp_before_admin_bar_render hook)
• update callback return type to :void
• Add global var

  <?php
    /** @var \WP_Admin_Bar $wp_admin_bar */
    global $wp_admin_bar;
  

Trac ticket: https://core.trac.wordpress.org/ticket/61738

[narenin]

Hi @hellofromTonya

I have submitted the PR with doc update for

admin_bar_menu

Could you please look into this.

[hellofromTonya]

This ticket was ​discussed during today's bug scrub. Summary:

• This is a docs only ticket. Adding the focus.
• @sabernhardt will work on the dev note.

  I have not started one yet, but I could write a draft. Much of it is already on this ticket.
  

• ​The docs patch needs changes. @sabernhardt will provide a review and guidance.

  The change is incorrect and incomplete.
  wp_before_admin_bar_render is probably not the best option for adding items.
  

[hellofromTonya]

Summary from ​last week's bug scrub:

• @sabernhardt ​reviewed and made suggestions on the docs patch ✅.
• @sabernhardt is working on the dev note "but I have not made much progress since last week."
• @sabernhardt asked about how to add "More Information" section on the docs page.
  • @Clorith shared "IT's custom postmeta added manually via wp-admin on the DevHub"
  • @sabernhardt will ask in #docs for how to add this postmeta. cc @audrasjb for awareness

Summary from today's bug scrub:

• Requested changes were implemented ✅ and is ready for commit.
• @sabernhardt is looking into possibly editing or removing the "most optimal point" sentence in the wp_admin_bar_render() docblock, but noted it's not a blocker for 6.6.2 commit.

@sabernhardt @joedolson can you do another review please? Is it ready for commit?

[hellofromTonya]

Patch: ​https://github.com/WordPress/wordpress-develop/pull/7222

Changes are updated. LGTM for commit. As a confidence check, @joedolson any concerns for committing it?

[hellofromTonya]

Patch: ​https://github.com/WordPress/wordpress-develop/pull/7222

Both @sabernhardt and @joedolson approved the doc changes. It LGTM too. Self-assigning for commit.

Closing in favor of the #7222 docs changes.

[hellofromTonya]

In 58978:

Docs: Add changing existing items recommendations to 'admin_bar_menu' action.

Improves the 'admin_bar_menu' docblock by adding recommendations for customizing / changing existing items.

It also improves the wp_admin_bar_render() function's docblock by removing the "most optimal point" sentence.

The [58748] changeset moved the user menu to a higher priority level (of 9991) to achieve:

so nodes added with higher priorities will still be shown visually before the user and recovery menu items, as they were prior to [58215].

When hooking into 'admin_bar_menu' with a priority level less than 9991 and attempting to get the 'my-account' node, null is returned. While the preferred approach is to hook into 'wp_before_admin_bar_render', the documentation of the 'admin_bar_menu' action was previously misleading.

Follow-up to [58748], [58759].

Props michaelwp85, narenin, sabernhardt, joedolson, costdev, jorbin, hellofromTonya, mukesh27, pbiron.
Fixes #61738.

[hellofromTonya]

Reopening for 2nd committer sign-off to backport [58978] to the 6.6 branch.

@joedolson are you available for the sign-off?

Thank you everyone for your contributions 🌟 This PR was committed via https://core.trac.wordpress.org/changeset/58978.

[joedolson]

On it, @hellofromTonya!

[joedolson]

In 58982:

Docs: Add changing existing items recommendations to 'admin_bar_menu' action.

Improves the 'admin_bar_menu' docblock by adding recommendations for customizing / changing existing items.

It also improves the wp_admin_bar_render() function's docblock by removing the "most optimal point" sentence.

The [58748] changeset moved the user menu to a higher priority level (of 9991) to achieve:

so nodes added with higher priorities will still be shown visually before the user and recovery menu items, as they were prior to [58215].

When hooking into 'admin_bar_menu' with a priority level less than 9991 and attempting to get the 'my-account' node, null is returned. While the preferred approach is to hook into 'wp_before_admin_bar_render', the documentation of the 'admin_bar_menu' action was previously misleading.

Follow-up to [58748], [58759].

Reviewed by joedolson.
Merges [58978] to the 6.6 branch.

Props michaelwp85, narenin, sabernhardt, joedolson, costdev, jorbin, hellofromTonya, mukesh27, pbiron.
Fixes #61738.

[hellofromTonya]

Modifying the Summary of this ticket to reflect the scope of agreed to changes.

[sabernhardt]

The ​dev note is mainly about #61615, but I'm adding a link to the draft on this ticket, too.

[hellofromTonya]

Dev note is now published ​https://make.wordpress.org/core/2024/09/10/admin-toolbar-menu-has-new-higher-priority-values/