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

westonruter · 2025-12-22T21:08:04Z

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

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.

…tyle

github-actions · 2025-12-22T21:08:13Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props westonruter, jonsurrell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter · 2025-12-22T21:19:57Z

src/wp-includes/script-loader.php

 		if ( $path && $src ) {
 			$size = wp_filesize( $path );
-			if ( ! $size ) {
+			if ( 0 === $size && ! file_exists( $path ) ) {


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.

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 …

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.

westonruter · 2025-12-22T21:21:00Z

src/wp-includes/script-loader.php


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


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.

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)

Error suppression removed in 8cc52a2

github-actions · 2025-12-22T21:33:52Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

westonruter · 2025-12-23T06:42:11Z

Tests are failing due to the GHA runner not having the built assets.

sirreal

There's a lot going on in this deceptively small patch. I've left some feedback for consideration.

sirreal · 2025-12-23T08:55:04Z

src/wp-includes/script-loader.php


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


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)

sirreal · 2025-12-23T09:19:09Z

src/wp-includes/script-loader.php

 		if ( $path && $src ) {
 			$size = wp_filesize( $path );
-			if ( ! $size ) {
+			if ( 0 === $size && ! file_exists( $path ) ) {


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 …

Co-authored-by: Jon Surrell <[email protected]>

…tyles

This reverts commit 4ceb527.

tests/phpunit/tests/media.php

westonruter · 2025-12-24T01:41:10Z

Gemini 3 review:

The changes look solid and follow WordPress core coding standards:

Logic: The handling of empty files versus missing files is now explicit.
Error Handling: Appropriate _doing_it_wrong() calls have been added for missing files.
Indentation: Verified that tabs are used consistently for indentation.
Testing: New tests cover the edge cases (bad path with fake size, good path with zero size) and verify that incorrect usage is triggered when expected.
Documentation: Proper JSDoc/PHPDoc tags are used, including @ticket and @covers.

No issues found. Everything looks ready for commit.

sirreal

I spent some time to understand the motivation around wp_filesize(). This line jumps out:

Improves support for plugins like S3 plugins that outsource files to third party file store.

I think the way this is implemented now should continue to work correctly, but I want to check my understanding. $style['path'] is a way for styles to opt-in to this inlining. Is that correct?

Presumably, styles that aren't locally available would not add path and would not generate noise with the second _doing_it_wrong message.

sirreal

Approving on the assumption that my comment does not raise any new concernts.

westonruter · 2025-12-29T16:56:24Z

$style['path'] is a way for styles to opt-in to this inlining. Is that correct?

Presumably, styles that aren't locally available would not add path and would not generate noise with the second _doing_it_wrong message.

That is correct. This is how styles can opt in to inlining. IMO, this could be made automatic by looking at the URL. But for now the path is how this is done. See dev note:

Internally, only assets that have data for path defined get processed, so to opt-in, a stylesheet can add something like this:
wp_style_add_data( $style_handle, 'path', $file_path );

* trunk: Scripts: Remove non-HTML5 script support. Scripts: Remove CDATA script wrappers in WP Admin. Tests: Use `assertSame()` in some newly introduced tests. Docs: Improve description for `add_settings_section()`. Scripts: Remove default type attribute from tags. Plugins: Update plugin compatibility text to remove obsolete percentage-based number. Tests: Update fonts tests to use semantic HTML comparison. Help/About: Simplify help text on Updates screen to account for third-party updates. Tests: Rename the `admin/includesUser.php` test file to be more descriptive. Upgrade/Install: Correct and improve various docblocks relating to upgrades. Export: Fix fatal error when passing `null` to `wxr_cdata()` by casting passed value to `string`. Code Modernization: Replace `isset()` with null coalescing in `WP_Roles::get_role()`. Code Modernization: Replace some `isset()` ternary checks with null coalescing.

…nd allow inlining empty stylesheets. When a stylesheet is registered with a `path` that does not exist or which is not readable, then a `_doing_it_wrong()` is now issued. Previously, paths that did not exist were silently skipped; paths for empty styles were also needlessly skipped, since `wp_filesize()` also returns `0` for the failure case. Developed in #10653 Follow-up to [50836]. Props westonruter, jonsurrell, soyebsalar01. See #52620. Fixes #64447. git-svn-id: https://develop.svn.wordpress.org/trunk@61416 602fd350-edb4-49c9-b593-d223f7449a82

github-actions · 2025-12-29T19:00:16Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 61416
GitHub commit: b057ff2

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

…nd allow inlining empty stylesheets. When a stylesheet is registered with a `path` that does not exist or which is not readable, then a `_doing_it_wrong()` is now issued. Previously, paths that did not exist were silently skipped; paths for empty styles were also needlessly skipped, since `wp_filesize()` also returns `0` for the failure case. Developed in WordPress/wordpress-develop#10653 Follow-up to [50836]. Props westonruter, jonsurrell, soyebsalar01. See #52620. Fixes #64447. Built from https://develop.svn.wordpress.org/trunk@61416 git-svn-id: http://core.svn.wordpress.org/trunk@60728 1a063a9b-81f0-0310-95a4-ce76da25c4cd

…nd allow inlining empty stylesheets. When a stylesheet is registered with a `path` that does not exist or which is not readable, then a `_doing_it_wrong()` is now issued. Previously, paths that did not exist were silently skipped; paths for empty styles were also needlessly skipped, since `wp_filesize()` also returns `0` for the failure case. Developed in WordPress/wordpress-develop#10653 Follow-up to [50836]. Props westonruter, jonsurrell, soyebsalar01. See #52620. Fixes #64447. Built from https://develop.svn.wordpress.org/trunk@61416 git-svn-id: https://core.svn.wordpress.org/trunk@60728 1a063a9b-81f0-0310-95a4-ce76da25c4cd

Issue _doing_it_wrong() when invalid path is provided to registered s…

9828bac

…tyle

westonruter requested a review from sirreal December 22, 2025 21:08

westonruter added 2 commits December 22, 2025 13:13

Add test coverage for file read error

9c7273c

Ensure stylesheets of zero size can be inlined

295efcc

westonruter requested a review from peterwilsoncc December 22, 2025 21:19

westonruter commented Dec 22, 2025

View reviewed changes

westonruter changed the title ~~Issue _doing_it_wrong() when invalid path is provided to registered style~~ Issue _doing_it_wrong() when invalid path is provided to registered style and allow empty stylesheets to be inlined Dec 22, 2025

westonruter changed the title ~~Issue _doing_it_wrong() when invalid path is provided to registered style and allow empty stylesheets to be inlined~~ Issue _doing_it_wrong() when registered style has invalid path and allow empty stylesheets to be inlined Dec 22, 2025

westonruter mentioned this pull request Dec 22, 2025

Implement PHPStan level 8 compliance for script-loader.php #10627

Open

westonruter added 2 commits December 22, 2025 20:47

Remove redundant test and add missing expectedIncorrectUsage tag

8cb36ca

Ensure built CSS files are present in tests

ce80e7a

westonruter marked this pull request as draft December 23, 2025 06:41

sirreal reviewed Dec 23, 2025

View reviewed changes

westonruter and others added 13 commits December 23, 2025 10:12

Use is_readable() instead of error suppression

8cc52a2

Co-authored-by: Jon Surrell <[email protected]>

Touch more styles

16f8e46

Touch stub CSS files in wp-includes which are created from build process

23debdb

Improve ensuring CSS files are present during tests

15d5bd8

Add fail-fast for testing

4ceb527

Ensure wp-emoji-loader.js is present for test_wp_hoist_late_printed_s…

5d1ba9f

…tyles

Add assertion to ensure wp-block-library was printed

a7660a9

Ensure wp-block-library-theme is created

1f8a2f8

Add yet more debug code

f4694aa

Improve ensuring that wp-block-library is not empty

d3fbff0

Revert "Add fail-fast for testing"

5c5912a

This reverts commit 4ceb527.

Remove debug code

46afeb7

Remove conditional setting of path data

7152b33

westonruter marked this pull request as ready for review December 24, 2025 01:29

westonruter commented Dec 24, 2025

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

Fix grammar

7e0f735

westonruter requested a review from sirreal December 24, 2025 01:41

sirreal reviewed Dec 29, 2025

View reviewed changes

sirreal approved these changes Dec 29, 2025

View reviewed changes

github-actions bot closed this Dec 29, 2025

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

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

Uh oh!

Conversation

westonruter commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 22, 2025

Test using WordPress Playground

Some things to be aware of

Uh oh!

westonruter commented Dec 23, 2025

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

westonruter commented Dec 24, 2025

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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

westonruter commented Dec 22, 2025 •

edited

Loading

github-actions bot commented Dec 22, 2025 •

edited

Loading

westonruter commented Dec 29, 2025 •

edited

Loading