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

[westonruter]

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.

[github-actions]

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]

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.

[sirreal]

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 …

[westonruter]

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]

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.

[sirreal]

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)

[westonruter]

Error suppression removed in 8cc52a2

[github-actions]

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]

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]

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]

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 …

[westonruter]

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.