WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#53375 closed defect (bug) (fixed)

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

Reported by: walbo Owned by: desrosj
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch commit
Focuses: Cc:

Description

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

Attachments (2)

53375-testing-pr1370.png (1.4 MB) - added by hellofromTonya 3 months ago.
Testing before and after applying PR 1370
53375.gif (2.0 MB) - added by walbo 3 months ago.

Change History (38)

This ticket was mentioned in PR #1362 on WordPress/wordpress-develop by walbo.


3 months ago

  • Keywords has-patch added

#2 @SergeyBiryukov
3 months ago

  • Component changed from General to Editor
  • Milestone changed from Awaiting Review to 5.8

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 months ago

#4 @chaion07
3 months ago

  • Keywords needs-testing added

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

#5 @desrosj
3 months ago

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

#6 @jorbin
3 months ago

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?

#8 @aristath
3 months ago

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.

#9 follow-up: @walbo
3 months ago

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

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 months ago

#11 in reply to: ↑ 9 ; follow-up: @desrosj
3 months ago

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.

#12 in reply to: ↑ 11 @walbo
3 months ago

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

#13 @SergeyBiryukov
3 months ago

  • Summary changed from Loading seperate core block assets tries to load stylesheets on blocks that dosn't have styles to Loading separate core block assets tries to load stylesheets on blocks that dosn't have styles

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 months ago

@hellofromTonya
3 months ago

Testing before and after applying PR 1370

#15 @hellofromTonya
3 months ago

  • Keywords needs-testing removed

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.

Last edited 3 months ago by hellofromTonya (previous) (diff)

#16 @hellofromTonya
3 months ago

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.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 months ago

#18 @desrosj
3 months ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#19 @prbot
3 months ago

hellofromtonya commented on PR #1370:

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

#20 @prbot
3 months ago

walbo commented on PR #1362:

Closing in favor of #1370

#21 @hellofromTonya
3 months ago

  • Keywords commit added

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 months ago

#23 @desrosj
3 months ago

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

#24 @desrosj
3 months ago

  • Summary changed from Loading separate core block assets tries to load stylesheets on blocks that dosn't have styles to Loading separate core block assets tries to load stylesheets on blocks that doesn't have styles

#25 @jorbin
3 months ago

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.

#26 @desrosj
3 months ago

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.

#27 @walbo
3 months ago

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.

#28 @desrosj
3 months ago

  • Keywords commit removed

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?

#29 @walbo
3 months ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 months ago

#31 @walbo
3 months ago

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

@walbo
3 months ago

#32 @desrosj
3 months ago

  • Keywords commit added

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

Going to commit the fix.

#33 @desrosj
3 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#35 @desrosj
3 months ago

In 51255:

Coding Standards: Apply an alignment fix after composer format.

Follow up to [51254].

See #53375.

#36 @desrosj
3 months ago

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.

Note: See TracTickets for help on using tickets.