Skip to content

Refactor wp_admin_bar_add_color_scheme_to_front_end() to not use HTTP and rewrite tests #11255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
westonruter wants to merge 2 commits into
base: trunk
Choose a base branch
from
Draft

Refactor wp_admin_bar_add_color_scheme_to_front_end() to not use HTTP and rewrite tests #11255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9256947
Refactor string logic
westonruter Mar 14, 2026
c2ebf0d
Refactor wp_admin_bar_add_color_scheme_to_front_end() to not use HTTP…
westonruter Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 37 additions & 19 deletions src/wp-includes/admin-bar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1443,9 +1443,9 @@ function _get_admin_bar_pref( $context = 'front', $user = 0 ) {
*
* @since 7.0.0
*
* @global array $_wp_admin_css_colors Registered administration color schemes.
* @global array<string, object{ url: string }> $_wp_admin_css_colors Registered administration color schemes.
*/
function wp_admin_bar_add_color_scheme_to_front_end() {
function wp_admin_bar_add_color_scheme_to_front_end(): void {
if ( is_admin() ) {
return;
}
Expand All @@ -1463,23 +1463,41 @@ function wp_admin_bar_add_color_scheme_to_front_end() {
}

$color = $_wp_admin_css_colors[ $color_scheme ] ?? null;
$url = $color->url ?? '';

if ( $url ) {
$response = wp_remote_get( $url );
if ( ! is_wp_error( $response ) ) {
$css = $response['body'];
if ( is_string( $css ) && str_contains( $css, '#wpadminbar' ) ) {
$start_position = strpos( $css, '#wpadminbar' );
$end_position = strpos( $css, '.wp-pointer' );
if ( false !== $end_position && $end_position > $start_position ) {
$css = substr( $css, $start_position, $end_position - $start_position );
if ( SCRIPT_DEBUG ) {
$css = str_replace( '/* Pointers */', '', $css );
}
}
wp_add_inline_style( 'admin-bar', $css );
}
if ( ! is_object( $color ) || ! isset( $color->url ) || ! is_string( $color->url ) ) {
return;
}

$path = wp_parse_url( $color->url, PHP_URL_PATH );
if ( ! $path || ! str_ends_with( $path, '.css' ) ) {
return;
}

$css_admin_relative_path = strstr( $path, '/wp-admin/' );
Copy link
Member Author

@westonruter westonruter Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hummm. Maybe this won't work since in theory wp_admin_css_color() could be called by a plugin in which case a CSS file in a plugin could be registered, or else a file located anywhere. It seems Jetpack and bbPress, among others, register their own color schemes: https://veloria.dev/search/c4128300-74c8-4bac-8207-032762a7bebb

Copy link
Member Author

@westonruter westonruter Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, we could try first to see if it is in wp-admin, and then use the local file from the filesystem in that case.

Otherwise, if it isn't in wp-admin, then I guess we could resort to using wp_remote_get(), but we should make sure that we store it in a transient.

In the same way that stylesheets can have path data added to them for inlining, it seems this is "needed" for admin color scheme styles as well.

Copy link

@sabernhardt sabernhardt Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with making the function work only with the core schemes in wp-admin, which should have a lower chance of breaking custom schemes. The priority for this release cycle is to show the Modern scheme on the front end.

if ( false === $css_admin_relative_path ) {
Copy link
Member Author

@westonruter westonruter Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: This should also bail if the URL contains any ../ to avoid the possibility of a malicious URL being registered to somehow read from a file (a CSS one) somewhere outside of the ABSPATH directory.

Suggested change
if ( false === $css_admin_relative_path ) {
if ( false === $css_admin_relative_path || str_contains( $css_admin_relative_path, '../' ) ) {

return;
}

$css_path = untrailingslashit( ABSPATH ) . $css_admin_relative_path;
if ( ! is_readable( $css_path ) ) {
Copy link
Member Author

@westonruter westonruter Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat surprised that is_readable( $css_path ) isn't returning false during unit tests on GHA, since I thought the file here would depend on a build.

return;
}

$css = file_get_contents( $css_path );
if ( false === $css ) {
return;
}

$start_position = strpos( $css, '#wpadminbar' );
if ( false === $start_position ) {
return;
}

$end_position = strpos( $css, '.wp-pointer' );
Copy link

@siliconforks siliconforks Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSS parsing seems really fragile here. It will be even more fragile for color schemes in plugins, which might have .wp-pointer in an unexpected place. (It's possible that the front end could get a whole bunch of unwanted CSS.) That may be another reason to do this only for core color schemes and not even attempt it for third-party color schemes.

if ( false !== $end_position && $end_position > $start_position ) {
$css = substr( $css, $start_position, $end_position - $start_position );
if ( SCRIPT_DEBUG ) {
$css = str_replace( '/* Pointers */', '', $css );
}
}
wp_add_inline_style( 'admin-bar', $css );
}
61 changes: 55 additions & 6 deletions tests/phpunit/tests/dependencies/wpStyleLoaderSrc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,26 @@
*/
class Tests_Dependencies_wpStyleLoaderSrc extends WP_UnitTestCase {

public ?WP_Styles $original_styles;

public ?WP_Screen $original_screen;

public function set_up(): void {
global $wp_styles, $current_screen;
parent::set_up();

$this->original_styles = $wp_styles;
$this->original_screen = $current_screen;
unset( $wp_styles, $current_screen );
}

public function tear_down(): void {
global $wp_styles, $current_screen;
$wp_styles = $this->original_styles;
$current_screen = $this->original_screen;
parent::tear_down();
}

/**
* Tests that PHP warnings are not thrown when wp_style_loader_src() is called
* before the `$_wp_admin_css_colors` global is set within the admin.
Expand All @@ -20,12 +40,41 @@ class Tests_Dependencies_wpStyleLoaderSrc extends WP_UnitTestCase {
*
* @ticket 61302
* @ticket 64762
*
* @covers ::wp_admin_bar_add_color_scheme_to_front_end
*/
public function test_without_wp_admin_css_colors_global_frontend(): void {
unset( $GLOBALS['current_screen'] );
$this->assertFalse( is_admin(), 'Expected not admin.' );
wp_styles();
wp_admin_bar_add_color_scheme_to_front_end();
$inline_styles = wp_styles()->get_data( 'admin-bar', 'after' );
$this->assertIsArray( $inline_styles, 'Expected inline styles to be added.' );
$inline_css = implode( "\n", $inline_styles );
$this->assertStringContainsString( '#wpadminbar', $inline_css );
$this->assertStringNotContainsString( '/* Pointers */', $inline_css );
$this->assertStringNotContainsString( '.wp-pointer', $inline_css );

$style_loader_src = wp_style_loader_src( '', 'colors' );
$this->assertIsString( $style_loader_src );
$this->assertStringContainsString( '/colors.css', $style_loader_src );
Comment on lines +58 to +60
Copy link
Member Author

@westonruter westonruter Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: The assertion here also passes when when is_admin(). So what is its purpose?

Copy link

@sabernhardt sabernhardt Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My edits were designed to make the test pass, and I still wanted to have an assertion when it was not in the admin. The value of checking whether it contains the /colors.css string (or /colors.min.css) was minimal.

}

/**
* Tests that nothing is done when in the admin.
*
* @ticket 61302
* @ticket 64762
*
* @covers ::wp_admin_bar_add_color_scheme_to_front_end
*/
public function test_without_wp_admin_css_colors_global() {
if ( is_admin() ) {
$this->assertFalse( wp_style_loader_src( '', 'colors' ) );
Copy link
Member Author

@westonruter westonruter Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion wasn't actually false when is_admin() as noted above.

Copy link

@sabernhardt sabernhardt Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r61388 added the test asserting $this->assertFalse( wp_style_loader_src( '', 'colors' ) ); if "used in a context where the $_wp_admin_css_colors global does not exist." It needs to continue meeting that expectation with the new wp_admin_bar_add_color_scheme_to_front_end() function.

} else {
$this->assertStringContainsString( '/colors.css', wp_style_loader_src( '', 'colors' ) );
}
public function test_without_wp_admin_css_colors_global_admin(): void {
global $wp_styles;
set_current_screen( 'index.php' );
$wp_styles = null;
$this->assertTrue( is_admin(), 'Expected admin.' );
wp_styles();
wp_admin_bar_add_color_scheme_to_front_end();
$this->assertFalse( wp_styles()->get_data( 'admin-bar', 'after' ), 'Expected no inline style to be added in the admin.' );
}
}
Loading