Skip to content

Issue _doing_it_wrong() when registered style has invalid path and allow empty stylesheets to be inlined #10653

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

Closed
westonruter wants to merge 20 commits into from
+104 −7
Closed

Issue _doing_it_wrong() when registered style has invalid path and allow empty stylesheets to be inlined #10653

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9828bac
Issue _doing_it_wrong() when invalid path is provided to registered s…
westonruter Dec 22, 2025
9c7273c
Add test coverage for file read error
westonruter Dec 22, 2025
295efcc
Ensure stylesheets of zero size can be inlined
westonruter Dec 22, 2025
8cb36ca
Remove redundant test and add missing expectedIncorrectUsage tag
westonruter Dec 23, 2025
ce80e7a
Ensure built CSS files are present in tests
westonruter Dec 23, 2025
8cc52a2
Use is_readable() instead of error suppression
westonruter Dec 23, 2025
16f8e46
Touch more styles
westonruter Dec 23, 2025
23debdb
Touch stub CSS files in wp-includes which are created from build process
westonruter Dec 23, 2025
15d5bd8
Improve ensuring CSS files are present during tests
westonruter Dec 23, 2025
4ceb527
Add fail-fast for testing
westonruter Dec 24, 2025
5d1ba9f
Ensure wp-emoji-loader.js is present for test_wp_hoist_late_printed_s…
westonruter Dec 24, 2025
a7660a9
Add assertion to ensure wp-block-library was printed
westonruter Dec 24, 2025
1f8a2f8
Ensure wp-block-library-theme is created
westonruter Dec 24, 2025
f4694aa
Add yet more debug code
westonruter Dec 24, 2025
d3fbff0
Improve ensuring that wp-block-library is not empty
westonruter Dec 24, 2025
5c5912a
Revert "Add fail-fast for testing"
westonruter Dec 24, 2025
46afeb7
Remove debug code
westonruter Dec 24, 2025
7152b33
Remove conditional setting of path data
westonruter Dec 24, 2025
7e0f735
Fix grammar
westonruter Dec 24, 2025
4b94b3b
Merge branch 'trunk' into trac-64447
westonruter Dec 29, 2025
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
29 changes: 27 additions & 2 deletions src/wp-includes/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3081,7 +3081,18 @@ function wp_maybe_inline_styles() {
$path = $wp_styles->get_data( $handle, 'path' );
if ( $path && $src ) {
$size = wp_filesize( $path );
if ( ! $size ) {
if ( 0 === $size && ! file_exists( $path ) ) {
Copy link
Member Author

@westonruter westonruter Dec 22, 2025

Choose a reason for hiding this comment

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

Note that wp_filesize() returns 0 both when the path is invalid and when the file has a zero size. So this file_exists() check here ensures that an empty stylesheet can be inlined.

Copy link
Member

@sirreal sirreal Dec 23, 2025

Choose a reason for hiding this comment

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

There's no reason to inline an empty stylesheet, is there? I suspect the falsy check here is appropriate.

This is interesting, is wp_filesize here intended to be an optimization where filters can be used to avoid hitting the filesystem?

I tend to use is_readable() for checks like this.

Strictly considering the logic, I think ideally this would be:

// Missing or unreadable files show notice
if ( ! is_readable(…) ) {
  _doing_it_wrong(…);
  continue;
}
// Empty files do not need to be included and are not an error.
if ( ! wp_filesize(…) {
  continue;
}

I think this would make it very difficult to hit a warning on the file_get_contents below, although not impossible.

I see wp_filesize() uses filesize which issues a warning if the file isn't present, but I think those will be handled by the readable check.

Warning: filesize(): stat failed for {FILE} in …

Copy link
Member Author

@westonruter westonruter Dec 23, 2025

Choose a reason for hiding this comment

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

There's no reason to inline an empty stylesheet, is there? I suspect the falsy check here is appropriate.

It's better to inline an empty stylesheet as opposed to wastefully loading it as a blocking resource. Right?

Strictly considering the logic, I think ideally this would be:

This may conflict with the original purpose for introducing the pre_wp_filesize filter. If the intention is to bypass the filesystem, then adding the separate ! is_readable() check first would conflict with that. I see this was introduced in r52837.

I tend to use is_readable() for checks like this.

I think file_exists() is better because this is what wp_filesize() specifically is using to return 0.

That said, I've added 8cc52a2 to use is_readable() instead of using error suppression.

_doing_it_wrong(
__FUNCTION__,
sprintf(
/* translators: 1: 'path', 2: filesystem path, 3: style handle */
__( 'Unable to read the "%1$s" key with value "%2$s" for stylesheet "%3$s".' ),
'path',
esc_html( $path ),
esc_html( $handle )
),
'7.0.0'
);
continue;
}
$styles[] = array(
Expand Down Expand Up @@ -3119,7 +3130,21 @@ static function ( $a, $b ) {
}

// Get the styles if we don't already have them.
$style['css'] = file_get_contents( $style['path'] );
$style['css'] = @file_get_contents( $style['path'] );
Copy link
Member Author

@westonruter westonruter Dec 22, 2025

Choose a reason for hiding this comment

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

The @ error suppression is added here because in the improved test coverage, this logic path is forced by using a filter to force an invalid path to have a non-zero file size.

Copy link
Member

@sirreal sirreal Dec 23, 2025

Choose a reason for hiding this comment

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

It would be good to comment (in code) with justification for the error suppression.

  • file_get_contents may generate E_WARNING
  • it will return false on failure (checked below)

Copy link
Member Author

@westonruter westonruter Dec 23, 2025

Choose a reason for hiding this comment

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

Error suppression removed in 8cc52a2

if ( false === $style['css'] ) {
_doing_it_wrong(
__FUNCTION__,
sprintf(
/* translators: 1: 'path', 2: filesystem path, 3: style handle */
__( 'Unable to read the "%1$s" key with value "%2$s" for stylesheet "%3$s".' ),
'path',
esc_html( $style['path'] ),
esc_html( $style['handle'] )
),
'7.0.0'
);
continue;
}

/*
* Check if the style contains relative URLs that need to be modified.
Expand Down
83 changes: 83 additions & 0 deletions tests/phpunit/tests/dependencies/styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,89 @@ public function test_wp_maybe_inline_styles_no_path() {
$this->assertSame( $GLOBALS['wp_styles']->registered['test-handle']->src, $url );
}

/**
* @ticket 64447
*
* @covers ::wp_maybe_inline_styles
* @expectedIncorrectUsage wp_maybe_inline_styles
*/
public function test_wp_maybe_inline_styles_bad_path_at_file_size_check() {
$handle = 'test-handle';
$inc_path = '/css/ancient-themes.css'; // Does not exist.
$url = '/' . WPINC . $inc_path;
wp_register_style( $handle, $url );
wp_style_add_data( $handle, 'path', ABSPATH . WPINC . $inc_path );
wp_enqueue_style( $handle );

wp_maybe_inline_styles();

$this->assertSame( $GLOBALS['wp_styles']->registered[ $handle ]->src, $url );
}

/**
* @ticket 64447
*
* @covers ::wp_maybe_inline_styles
* @expectedIncorrectUsage wp_maybe_inline_styles
*/
public function test_wp_maybe_inline_styles_bad_path_with_file_size_provided() {
$inc_path = '/css/ancient-themes.css'; // Does not exist.

// This ensures the initial file size check is bypassed.
add_filter(
'pre_wp_filesize',
static function ( $size, $path ) use ( $inc_path ) {
if ( str_contains( $path, $inc_path ) ) {
$size = 1000;
}
return $size;
},
10,
2
);

$handle = 'test-handle';
$url = '/' . WPINC . $inc_path;
wp_register_style( $handle, $url );
wp_style_add_data( $handle, 'path', ABSPATH . WPINC . $inc_path );
wp_enqueue_style( $handle );

wp_maybe_inline_styles();

$this->assertSame( $GLOBALS['wp_styles']->registered[ $handle ]->src, $url );
}

/**
* @ticket 64447
*
* @covers ::wp_maybe_inline_styles
*/
public function test_wp_maybe_inline_styles_good_path_with_zero_file_size_provided() {
$inc_path = '/css/classic-themes.css';

// This simulates the file having a zero size.
add_filter(
'pre_wp_filesize',
static function ( $size, $path ) use ( $inc_path ) {
if ( str_contains( $path, $inc_path ) ) {
$size = 0;
}
return $size;
},
10,
2
);

$handle = 'test-handle';
wp_register_style( $handle, '/' . WPINC . $inc_path );
wp_style_add_data( $handle, 'path', ABSPATH . WPINC . $inc_path );
wp_enqueue_style( $handle );

wp_maybe_inline_styles();

$this->assertFalse( $GLOBALS['wp_styles']->registered[ $handle ]->src );
}

/**
* @ticket 63887
*/
Expand Down
Loading