Loading separate core block assets tries to load stylesheets on blocks that doesn't have styles

[walbo]

In [51102] support for adding inline styles for all blocks was added, but this makes WP register styles to non-existing stylesheets.

Steps to replicate:

1. Add add_filter( 'should_load_separate_core_block_assets', '__return_true' );
2. Insert a block that dosn't have a block css. Ex. core/column.
3. Visit the page and check the network tab to confirm that WP tries to load /wp-includes/blocks/column/style.css with status 404.

In Gutenberg this was fixed by adding false url to blocks that dosn't have a stylesheet. See ​https://github.com/WordPress/gutenberg/pull/32275

Related: #53358

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

[chaion07]

Hi @walbo! Thank you for reporting this. We reviewed the ticket during a [recent bug-scrub session in Core]​https://wordpress.slack.com/archives/C02RQBWTW/p1623702905385100. Based on the feedback from the team we feel the need for an owner to ensure that it gets to commit in the release cycle. The PR/patch seems good. Thanks!

Props to @JeffPaul

[desrosj]

Thanks for this one @walbo! I have some thoughts on the PR that I'll share inline.

[jorbin]

The file_exists and filemtime was added in [50836] by @gziolo. Related: #50328, #52620.

Would it be possible to get rid of them and have blocks opt in to individual stylesheets and if they don't opt in, not output anything?

Backports ​https://github.com/WordPress/gutenberg/pull/32275

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

[aristath]

Pushed an alternative patch on ​https://github.com/WordPress/wordpress-develop/pull/1370 backporting the Gutenberg tweaks. A followup will be pushed in Gutenberg to remove some file_exists checks.

[walbo]

Seeing file_exists is used some other places in core as well so should maybe create a second ticket to explore removal of those.

[desrosj]

Replying to walbo:

Seeing file_exists is used some other places in core as well so should maybe create a second ticket to explore removal of those.

I haven't taken a deep dive into the other uses in Core, but this one happens within a loop through register_core_block_types_from_metadata(). On brief glance, the other instances seem like one off file checks.

[walbo]

Have tested the alternative PR from @aristath and it solves the issue. That PR removes one of the file_exists.

[hellofromTonya]

Testing before and after applying PR 1370

[hellofromTonya]

Testing

Env:

• OS: macOS Big Sur
• Browser: Firefox 89.0.1 and Chrome 91.0.4472.106
• ​PR 1370

Before applying the PR results:

Able to reproduce the reported problem as the following asset was requested in the browser (though it does not exist):

http://localhost:8889/wp-includes/blocks/column/style.css/?ver=5.8-beta2-51165-src

UPDATE:
I was not able to reproduce the 404 as my local test environment redirects it back to the root page. But the asset doesn't exist and should return 404. This specific problem is not related to the problem reported in this ticket and needs to be resolved separately.

After applying the PR results:

The nonexistent stylesheet was not requested.

Conclusion

Works as expected ✅

Removing the needs-testing keyword.

[hellofromTonya]

Code Review of ​PR 1370:

• Remove the loose implicit conditionals and replace with explicit conditional. My suggestion is to add a new variable called $has_style_file. See notes in the PR.

• @desrosj and I discussed filemtime() for non-core blocks.

I'm not seeing another dynamic way of setting a version number for these non-core blocks without checking the stylesheet's modification time. Non-dynamic ways would require adding a version to the metadata or opening the file and reading a version annotation (significantly less performant than filemtime. IMO filemtime() is the best route for dynamic versioning to ensure the browser requests the newest version of the non-core asset from the server.

Added some formatting comments that the core committer could take care of. Else, the patch looks good to me 👍

Closing in favor of #1370

[hellofromTonya]

Approved the PR. Marking ready for commit and Beta 4.

[desrosj]

#53507 has been opened to handle the filemtime() instance.

[jorbin]

In my eyes, there are two good options and I don't know how realistic 1 of them is:

• Revert the entire feature
• Work to remove the filemtime before 5.8

Especially when using network based storage, this can be a slower than necessary operation and we should be aiming to keep the front end of the site as speedy as humanly possible.

[desrosj]

I've moved #53507 to the 5.8 milestone to investigate. As far as I can tell, the version field in metadata is used already in register_block_script_handle(). So I think we could try:

• Using this field if present in the metadata.
• Fall back to false if version is not set.
• Publish a dev note encouraging developers to always include a top level version field in there block.json file.

[walbo]

I agree that the filemtime should be removed, but think the discussion should continue in #53507

The current patch doesn't add the filemtime and fixes the issue described in the ticket.

[desrosj]

I was going to test this one more time and commit, but I'm now having trouble getting any individual style calls following the steps with add_filter( 'should_load_separate_core_block_assets', '__return_true' );.

Ignoring the filemtime() occurrence that we'll address in #53507, the changes in the PR definitely look like they are improvements to the underlying code. But I'd like to identify whether the original issue was fixed by another change before committing.

Can everyone retest with WP 5.8 beta4 to see if I'm missing something, or confirm that the issue was fixed?

[walbo]

Still getting a GET http://localhost:8889/wp-includes/blocks/column/style.css?ver=5.8-beta4-51243-src net::ERR_ABORTED 404 (Not Found) in trunk.

With the patch everything works as expected.

[walbo]

Here is a detailed steps to replicate after a request in slack:

1. Create a new WordPress installation. I use npm run env:start in wordpress-develop.
2. Add add_filter( 'should_load_separate_core_block_assets', '__return_true' ); in functions.php. I have twenty twenty-one enabled.
3. Set permalinks to Day and name. (if not the file will return 301. See #53506)
4. Edit the hello world post. Insert columns. Select 50/50. Insert a paragraph in one of the columns.
5. Save the post and visit the page frontend.
6. Open the network and look for a 404 on http://localhost:8889/wp-includes/blocks/column/style.css

[desrosj]

Thanks @walbo! I'm not sure which step I was missing before, but I am seeing this now.

Going to commit the fix.

[desrosj]

In 51254:

Editor: Prevent block stylesheets from loading when they do not exist.

This fixes an issue where block stylesheets were being loaded even if they did not exist, causing 404 errors. The issue presented itself when the site was choosing to load block assets individually through the should_load_separate_core_block_assets filter hook.

This also fixes an issue where non-Core blocks would only be registered if they actually had asset files. This prevents developers from adding additional information to a style handle, such as inline styles through wp_add_inline_style().

Props walbo, jorbin, aristath, desrosj, hellofromTonya.
Fixes #53375.

Merged into Core in https://core.trac.wordpress.org/changeset/51254.

[desrosj]

In 51255:

Coding Standards: Apply an alignment fix after composer format.

Follow up to [51254].

See #53375.

[desrosj]

I've opened ​https://github.com/WordPress/gutenberg/pull/33075 to discuss moving forward with using and encouraging version in block.json in a more organized way.