Refactor wp_admin_bar_add_color_scheme_to_front_end() to not use HTTP and rewrite tests#11255
Refactor wp_admin_bar_add_color_scheme_to_front_end() to not use HTTP and rewrite tests#11255westonruter wants to merge 2 commits intoWordPress:trunkfrom
wp_admin_bar_add_color_scheme_to_front_end() to not use HTTP and rewrite tests#11255Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| $style_loader_src = wp_style_loader_src( '', 'colors' ); | ||
| $this->assertIsString( $style_loader_src ); | ||
| $this->assertStringContainsString( '/colors.css', $style_loader_src ); |
There was a problem hiding this comment.
TODO: The assertion here also passes when when is_admin(). So what is its purpose?
There was a problem hiding this comment.
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.
| */ | ||
| public function test_without_wp_admin_css_colors_global() { | ||
| if ( is_admin() ) { | ||
| $this->assertFalse( wp_style_loader_src( '', 'colors' ) ); |
There was a problem hiding this comment.
This assertion wasn't actually false when is_admin() as noted above.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| $css_path = untrailingslashit( ABSPATH ) . $css_admin_relative_path; | ||
| if ( ! is_readable( $css_path ) ) { |
There was a problem hiding this comment.
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.
|
With the Sunrise color scheme applied. Before ❌ 
After ✅ 
|
| } | ||
|
|
||
| $css_admin_relative_path = strstr( $path, '/wp-admin/' ); | ||
| if ( false === $css_admin_relative_path ) { |
There was a problem hiding this comment.
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.
| if ( false === $css_admin_relative_path ) { | |
| if ( false === $css_admin_relative_path || str_contains( $css_admin_relative_path, '../' ) ) { |
| return; | ||
| } | ||
|
|
||
| $css_admin_relative_path = strstr( $path, '/wp-admin/' ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| return; | ||
| } | ||
|
|
||
| $end_position = strpos( $css, '.wp-pointer' ); |
There was a problem hiding this comment.
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.
3 participants
Trac ticket: https://core.trac.wordpress.org/ticket/64762
Use of AI Tools
None 🧠
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.