Make WordPress Core

Opened 12 years ago

Last modified 3 months ago

#28569 accepted enhancement

Calling show_admin_bar( false ) should dehook toolbar entirely

Reported by: danielbachhuber Owned by: joedolson
Priority: normal Milestone: Future Release
Component: Toolbar Version:
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

Because I'm only hiding the toolbar in certain contexts, I'm calling show_admin_bar( false ) right before get_header(). #16249 was closed suggesting the function should only be called on init. #21746 suggests calling after plugins_loaded However, I think calling it after the query is set and the template is picked is a valid use case.

As such, I'd propose calling show_admin_bar( false ) should run remove_action('wp_head', '_admin_bar_bump_cb'); and similar setup callbacks.

Attachments (2)

28569.patch​ (428 bytes ) - added by paulschreiber 12 years ago.
28569.1.diff​ (440 bytes ) - added by audrasjb 5 years ago.
Toolbar: unhook the admin bar when a falsey value is used on show_admin_bar()

Download all attachments as: .zip

Change History (25)

#1 @paulschreiber
12 years ago

What other callbacks count as "and similar setup callbacks"?

#2 @helen
12 years ago

  • Keywords has-patch added; needs-patch removed

#3 @DrewAPicture
12 years ago

  • Owner set to paulschreiber
  • Status new → assigned

#4 @chriscct7
11 years ago

  • Owner changed from paulschreiber to chriscct7
  • Status assigned → reviewing

This ticket was mentioned in ​Slack in #core by sabernhardt. ​View the logs.


6 years ago

#6 @sabernhardt
6 years ago

  • Milestone → 5.8
  • Owner changed from chriscct7 to davidbaumwald

The patch still applies, though it could use a ​Yoda condition on the check.

Also, it would help if someone with deeper knowledge of the admin bar (and/or themes) agrees this is the correct way to do this. I have used the show_admin_bar filter before, and returning false with that removes the style tag. But I have not used show_admin_bar( false ) to know how that works.

Last edited 6 years ago by sabernhardt (previous) (diff)

This ticket was mentioned in ​Slack in #core by sabernhardt. ​View the logs.


5 years ago

#8 @sabernhardt
5 years ago

  • Keywords needs-refresh added; good-first-bug removed
  • Milestone 5.8 → Future Release

It's a bit late for 5.8. Moving to future release.

#9 @audrasjb
5 years ago

  • Keywords commit added; needs-refresh removed
  • Milestone Future Release → 5.9

Patch refreshed against trunk.
I tested the patch and I can confirm it unhook entirely the admin bar and its assets.

Adding commit keyword and moving to milestone 5.9.

@audrasjb
5 years ago

Toolbar: unhook the admin bar when a falsey value is used on show_admin_bar()

#10 @peterwilsoncc
5 years ago

  • Keywords needs-refresh added; commit removed

Removing the commit keyword as it needs a little extra work:

  • admin-bar.css is still enqueued in the header
  • admin-bar.js is still enqueued in the footer
  • <style media="print">#wpadminbar { display:none; }</style> is still output in the header (harmless but not ideal)
  • it's possible for themes & plugins to set a custom callback add_theme_support( 'admin-bar', array( 'callback' => '28569_admin_bar_cb' ) );

Testing notes: added show_admin_bar( false ); to my theme's header.php prior to the wp_head() function call.

I'll reach out to a Jetpack team member to ensure they're aware of this change coming so they can check for compatibility with the WordPress.com bar. I'd rather not surprise them and break 5 million sites.

#11 @obenland
5 years ago

Let's also make sure that it works as expected when set to true again after.

<?php
show_admin_bar( false );
$this->assertFalse( has_action( 'wp_head', '_admin_bar_bump_cb' ) );

show_admin_bar( true );
$this->assertEquals( 10, has_action( 'wp_head', '_admin_bar_bump_cb' ) );

#12 @obenland
5 years ago

Looking at this a little bit more, the toolbar can also be toggled on/off through the show_admin_bar filter, which has no connection to the function.

This ticket was mentioned in ​Slack in #core by chaion07. ​View the logs.


5 years ago

#14 @chaion07
5 years ago

  • Keywords needs-unit-tests added

Hi @afercia! Thank you for reporting this. This ticket was discussed during a recent ​Bug-scrub session. Based on the feedback received we're updating the ticket. It would be great to have a patch too. Thanks!

Props to @audrasjb & @costdev

#15 @sabernhardt
5 years ago

  • Milestone 5.9 → Future Release

Moving to Future Release until the patch is refreshed, with unit tests.

This ticket was mentioned in ​PR #12402 on ​WordPress/wordpress-develop by ​@sainathpoojary.


3 months ago
#16

  • Keywords needs-refresh removed

Trac ticket: #28569

#17 @joedolson
3 months ago

  • Milestone Future Release → 7.1

See #65091.

It would be good to resolve this at the same time, so removing the adminbar is more thorough.

Moving to 7.1 for consideration.

#18 @davidbaumwald
3 months ago

  • Owner davidbaumwald removed

With 7.1 Beta 1 tomorrow, I don't have time to commit it. Removing myself as owner so someone else can pick it up.

This ticket was mentioned in ​PR #12533 on ​WordPress/wordpress-develop by ​@khokansardar.


3 months ago
#19

  • Keywords has-unit-tests added; needs-unit-tests removed

Makes show_admin_bar( false ) fully dehook the toolbar when it is called after the admin bar has already been initialized.

What the problem was:

  • show_admin_bar( false ) only set the $show_admin_bar global to false.
  • When called after _wp_admin_bar_init() has run (e.g. right before get_header()), the actions registered by WP_Admin_Bar::initialize() stayed hooked and the admin-bar assets stayed enqueued.
  • Result: _admin_bar_bump_cb still added the margin-top: 32px body offset and the admin bar CSS/JS still loaded, even though no toolbar rendered.

What the fix does:

  • When disabling, show_admin_bar() now removes wp_admin_bar_header, _admin_bar_bump_cb, and the enqueue-style callbacks, and dequeues the admin-bar script and style.
  • When re-enabling after admin_bar_init has fired, those actions and assets are restored so toggling stays symmetric.

Approach and why:

  • Follows the approach in the ticket description and PR #12402, which a committer was ready to land for 7.1.
  • The did_action( 'admin_bar_init' ) guard keeps the common early call ("immediately upon plugin load", per the docblock) a pure setter, so no hooks are added before the toolbar is initialized.
  • Kept the enable/disable path symmetric per @joedolson's request in the ticket for a more thorough toggle (see #65091).

Testing instructions:

  1. As a logged-in admin on the front end, add to a theme's functions.php: add_action( 'template_redirect', function () { show_admin_bar( false ); }, 20 );
  2. Before the patch: view the front end — the page still has a ~32px top margin and loads the admin-bar stylesheet even though no toolbar shows.
  3. After the patch: the top margin is gone and the admin-bar assets are no longer enqueued.
  4. Edge cases: calling show_admin_bar( true ) again after disabling restores the toolbar; calling show_admin_bar( false ) early (before init) remains a harmless no-op.
  5. Run the tests: npm run test:php -- --filter show_admin_bar tests/phpunit/tests/adminbar.php

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

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: Ticket analysis and implementation tests cases. All changes were reviewed, validated against the codebase, and are taken responsibility for by me.

​@wildworks commented on ​PR #12533:


3 months ago
#20

Closing this in favor of #12402.

This ticket was mentioned in ​Slack in #core by adrianduffell. ​View the logs.


3 months ago

#22 @adrianduffell
3 months ago

  • Milestone 7.1 → Future Release

This would have been good to pair with #65091, but with the 7.1 beta a few hours away, I'll punt this. It would be good to pick up again early in the 7.2 cycle.

#23 @joedolson
3 months ago

  • Owner set to joedolson
  • Status reviewing → accepted
Note: See TracTickets for help on using tickets.