Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#56408 closed task (blessed) (fixed)

Blocks: Allow registering multiple items for all supported asset types

Reported by: gziolo's profile gziolo Owned by: gziolo's profile gziolo
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests needs-dev-note
Focuses: Cc:

Description

Part of https://github.com/WordPress/gutenberg/issues/41236. More details in https://github.com/WordPress/gutenberg/issues/33542.

This PR is heavily inspired by the work done by @zieladam in #56094.

The idea is to allow more than one script per block for editorScript, script, and viewScript. @aristath already added that capability for style and editorStyle a long time ago with https://github.com/WordPress/gutenberg/pull/32510. It was backported to WordPress core using the same hooks that Gutenberg uses with [52069] as part of #54337, which causes issues like the one reported in https://github.com/WordPress/gutenberg/issues/43086. Here, the code gets refactored for style, editorStyle so multiple assets are supported natively in WP_Block_Type class and in the REST API endpoint for block types. The same implementation is mirrored for scripts: editorScript, script and viewScript.

Attachments (1)

Bildschirmfoto 2022-09-20 um 15.36.57.png (122.3 KB) - added by TobiasBg 22 months ago.
404 errors with [54155]

Download all attachments as: .zip

Change History (31)

This ticket was mentioned in PR #3108 on WordPress/wordpress-develop by gziolo.


2 years ago
#1

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

Part of https://github.com/WordPress/gutenberg/issues/41236. More details in https://github.com/WordPress/gutenberg/issues/33542.

This PR is heavily inspired by the work done by @adamziel in https://github.com/WordPress/wordpress-develop/pull/2853.

The idea is to allow more than one script per block for editorScript, script, and viewScript. @aristath already added that capability for style and editorStyle a long time ago with https://github.com/WordPress/gutenberg/pull/32510. It was backported to WordPress core using the same hooks that Gutenberg uses, which causes issues like the one reported in https://github.com/WordPress/gutenberg/issues/43086. Here, the code gets refactored for style, editorStyle so multiple assets are supported natively in WP_Block_Type class and in the REST API endpoint for block types. The same implementation is mirrored for scripts: editorScript, script and viewScript.

aristath commented on PR #3108:


2 years ago
#2

Brilliant! Thank you so much for working on this @gziolo 👍
I haven't tested the implementation yet, but the code looks good and makes perfect sense 🎉

adamziel commented on PR #3108:


2 years ago
#3

@gziolo The following block.json:

"style": ["wp-block-button", "file:test.css", "wp-column-block"]

Yields the following HTML:

{{{html
<link rel='stylesheet' id='wp-block-button-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
<link rel='stylesheet' id='wp-block-button-2-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
<link rel='stylesheet' id='wp-block-button-3-css' href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
}}}

So something isn't right yet.

adamziel commented on PR #3108:


2 years ago
#4

This code in register_block_style_handle overrides the original style path:

	// Check whether styles should have a ".min" suffix or not.
	$suffix    = SCRIPT_DEBUG ? '' : '.min';
	$style_uri = plugins_url( $style_path, $metadata['file'] );
	if ( $is_core_block ) {
		$style_path = "style$suffix.css";
		$style_uri  = includes_url( 'blocks/' . str_replace( 'core/', '', $metadata['name'] ) . "/style$suffix.css" );
	}

gziolo commented on PR #3108:


2 years ago
#5

This code in register_block_style_handle overrides the original style path:

@adamziel, great catch. It's tricky to fix as it's correct today for existing core blocks only because it's always the style targeting the block, and subsequent styles are handled with the hook. If someone would change the order, then a similar issue would apply. It seems like we need to redo the special handling for styles in core blocks.

adamziel commented on PR #3108:


2 years ago
#6

The case prefixed with file: would be pretty simple to exclude from this special handling. I guess that style from a different block "wp-column-block" could bail out early as it should get registered elsewhere.

It could make sense for blocks that depend on other blocks or block-like elements, e.g. the search block may want to include the button block's styles.

gziolo commented on PR #3108:


22 months ago
#7

@gziolo The following block.json:

"style": ["wp-block-button", "file:test.css", "wp-column-block"]

Yields the following HTML:

<link rel='stylesheet' id='wp-block-button-css'  href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
<link rel='stylesheet' id='wp-block-button-2-css'  href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />
<link rel='stylesheet' id='wp-block-button-3-css'  href='http://localhost:8888/wp-includes/blocks/button/style.css?ver=6.1-alpha-20220819.130951' media='all' />

The way I approached core blocks, it works as follows:

  • you can only pass style handles like it was possible before, entries starting with file: will get ingored
  • only the first style handle passed gets special treatment
  • additional style handles needs to be registered elsewhere

gziolo commented on PR #3108:


22 months ago
#8

@ockham, this backport is ready but I'm waiting for some additional feedback. Let me know if it's blocking the progress on syncing Gutenberg changes into WordPress core. I'm happy to commit those changes and iterate later when necessary.

ockham commented on PR #3108:


22 months ago
#9

@ockham, this backport is ready but I'm waiting for some additional feedback. Let me know if it's blocking the progress on syncing Gutenberg changes into WordPress core. I'm happy to commit those changes and iterate later when necessary.

Thank you, @gziolo! It looks like we will eventually need it to unblock the sync. We have another week until Feature Freeze -- do you think that's enough time to get feedback and potentially make changes? If it isn't, it might be better to merge it as-is and iterate on feedback in a separate issue/PR 😊

#10 @gziolo
22 months ago

  • Keywords needs-dev-note added

#11 @gziolo
22 months ago

  • Owner set to gziolo
  • Resolution set to fixed
  • Status changed from assigned to closed

In 54155:

Blocks: Allow registering multiple items for all supported asset types

Follow-up #54337, [52069]. Part of https://github.com/WordPress/gutenberg/issues/41236. More details in https://github.com/WordPress/gutenberg/issues/33542.

Allow passing more than one script per block for editorScript, script, and viewScript fields in the block.json metadata file. This aligns with the previously added changes for style and editorStyle fields.

This change impacts the WP_Block_Type class and the REST API endpoint for block types. To ensure backward compatibiliy old names were soft deprecated in favor of new fields that work with array values and have _handles suffix.

Props zieladam, dlh, timothyblynjacobs, aristath, bernhard-reiter.
Fixes #56408.

#12 @gziolo
22 months ago

  • Keywords dev-feedback removed

gziolo commented on PR #3108:


22 months ago
#13

@ockham, this backport is ready but I'm waiting for some additional feedback. Let me know if it's blocking the progress on syncing Gutenberg changes into WordPress core. I'm happy to commit those changes and iterate later when necessary.

Thank you, @gziolo! It looks like we will eventually need it to unblock the sync. We have another week until Feature Freeze -- do you think that's enough time to get feedback and potentially make changes? If it isn't, it might be better to merge it as-is and iterate on feedback in a separate issue/PR 😊

I committed all the changes with https://core.trac.wordpress.org/changeset/54155 to unblock the Gutenberg sync process. I'll watch this PR and the Trac ticket for further feedback and take the necessary actions.

#14 @TobiasBg
22 months ago

@gziolo:
In the backwards compat layer, there are a few $this->{$new_name }[0] with an extra space after new_name that could/should be removed.

#15 @gziolo
22 months ago

Thank you, @TobiasBg, for letting me know. I fixed that with [54158].

#16 @desrosj
22 months ago

In 54210:

Coding Standards: Various alignment fixes from composer format.

Follow up to [53874], [54097], [54110], [54155], [54162], [54184].

See #39210, #55443, #56288, #56092, #56408, #56467, #55881.

#17 @TobiasBg
22 months ago

It looks like [54155] broke loading of style.css files, namely it's enqueuing style.css files that don't exist on the frontend, which leads to 404 errors.
I'm for example getting the ones from the attached screenshot. All these style.css files don't exist in Core.

The errors don't appear when working on a checkout of the commit's predecessor, r54154.

#18 @TobiasBg
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @TobiasBg
22 months ago

#56614 was marked as a duplicate.

#20 @gziolo
22 months ago

@TobiasBg, thank you for the report. As far as I can tell, all the blocks that trigger 404 don't have style field in block.json so it must be one of the filters that inject this type of style to all core blocks with no content. I will investigate.

#22 @gziolo
22 months ago

I included the patch on GitHub that hopefully will fix the issue with 404 HTTP codes for blocks that don't have style files but still register handles.

#23 @gziolo
22 months ago

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

In 54323:

Blocks: Fix 404 error for core styles with no file

[54155] broke loading of style.css files, namely it was enqueuing style.css files that don't exist on the frontend, which lead to 404 HTTO errors. All these style.css files don't exist for core blocks as they should be registered style handlers without a file path.

Follow-up to [54155].
Props tobiasbg, nendeb55.
Fixes #56408, #56614.

This ticket was mentioned in PR #3395 on WordPress/wordpress-develop by aristath.


22 months ago
#25

It appears that an extra condition was preventing multiple scripts from loading in blocks like the navigation block - where we need a separate script for the modal.

This patch removes an additional condition that is not necessary.
Whether a style or script gets enqueued should not be related to the block's render callback.

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

c4rl0sbr4v0 commented on PR #3395:


22 months ago
#26

Nice job!! Work as expected

#28 @davidbaumwald
22 months ago

In 54367:

Editor: Ensure block script is enqueued, regardless of ronder_callback.

Follow-up to [54155].

Props aristath, cbravobernal.
See #56408.

#30 @gziolo
22 months ago

[54367] effectively closes #56470 as resolved. It will have to be included in the dev note for Block API changes in WordPress 6.1. We should also update the following section in the documentation:

https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#frontend-enqueueing

In particular, the following section must be updated to reflect the changes applied:

viewScript (when the block defines render_callback during registration in PHP or a render field in its block.json, then the script is registered but the block author is responsible for enqueuing it)

I agree that the old behavior was too confusing, and we seek alternative ways to allow conditional enqueueing of view scritps.

Note: See TracTickets for help on using tickets.