Blocks: Allow registering multiple items for all supported asset types #3108

gziolo · 2022-08-18T12:55:24Z

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

Part of WordPress/gutenberg#41236. More details in WordPress/gutenberg#33542.

This PR is heavily inspired by the work done by @adamziel in #2853.

The idea is to allow more than one script per block for editorScript, script, and viewScript in the block.json metadata file. @aristath already added that capability for style and editorStyle a long time ago with WordPress/gutenberg#32510. It was backported to WordPress core using the same hooks that Gutenberg uses, which causes issues like the one reported in WordPress/gutenberg#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.

Before

{
 	"editorScript": "tests-notice-editor-script",
 	"script": "tests-notice-script",
 	"viewScript": "tests-notice-view-script",
 	"editorStyle": "tests-notice-editor-style",
 	"style": "tests-notice-style"
}

After

{
 	"editorScript": "tests-notice-editor-script",
 	"script": "tests-notice-script",
 	"viewScript": [ "tests-notice-view-script", "tests-notice-view-script-2" ],
 	"editorStyle": "tests-notice-editor-style",
 	"style": [ "tests-notice-style", "tests-notice-style-2" ]
}

It's backward compatible and will work with a string or an array of strings. Both forms can be used with any asset type.

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.

gziolo · 2022-08-19T06:58:15Z

src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

-					'description' => __( 'Editor script handle.' ),
-					'type'        => array( 'string', 'null' ),
+					'description' => __( 'Editor script handles.' ),
+					'type'        => array( 'array', 'string', 'null' ),


The same applies to all fields related to scripts and styles. Is extending the list of return types for the field in the REST API endpoint a breaking change? Or maybe updating the schema and releasing a dev note is enough?

Without this patch, it's possible to register multiple styles, but it's handled with a hook in WordPress core. In practice, that means only the first registered style gets exposed through REST API, and the rest is ignored. We could keep the same old, imperfect behavior if introducing an array would be a breaking change.

While I have no special knowledge or experience to offer, in my own experience with the REST API, I think I would consider adding array to these properties to be a breaking change. It might still be an acceptable breaking change to make with a dev note, and there might even be precedent for adding array to properties before.

I think it would be unlikely that my (hypothetical) integration with this endpoint would be set up to handle an array correctly when string|null was expected until now. Would it work to create "plural" versions of these properties (editor_scripts, scripts, etc.) that support arrays with the intention of deprecating the existing "singular" properties (see https://core.trac.wordpress.org/ticket/52629)?

@dlh01, thank you for sharing your feedback.

I think it would be unlikely that my (hypothetical) integration with this endpoint would be set up to handle an array correctly when string|null was expected until now.

This is exactly what I'm worried about here. However, the same in theory would apply to WP_Block_Type and its properties here. Introducing new properties would resolve some of the issues, but it might also create another set of issues where we might need to keep singular and plural versions in sync. Unless, we use magic methods like __get and __set for plural fields that operate on the first item in the array.

The current way to deprecate REST API fields in the response looks as follows:

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

Lines 1430 to 1461 in 1d45670

'rotation' => array(

'description' => __( 'The amount to rotate the image clockwise in degrees. DEPRECATED: Use `modifiers` instead.' ),

'type' => 'integer',

'minimum' => 0,

'exclusiveMinimum' => true,

'maximum' => 360,

'exclusiveMaximum' => true,

),

'x' => array(

'description' => __( 'As a percentage of the image, the x position to start the crop from. DEPRECATED: Use `modifiers` instead.' ),

'type' => 'number',

'minimum' => 0,

'maximum' => 100,

),

'y' => array(

'description' => __( 'As a percentage of the image, the y position to start the crop from. DEPRECATED: Use `modifiers` instead.' ),

'type' => 'number',

'minimum' => 0,

'maximum' => 100,

),

'width' => array(

'description' => __( 'As a percentage of the image, the width to crop the image to. DEPRECATED: Use `modifiers` instead.' ),

'type' => 'number',

'minimum' => 0,

'maximum' => 100,

),

'height' => array(

'description' => __( 'As a percentage of the image, the height to crop the image to. DEPRECATED: Use `modifiers` instead.' ),

'type' => 'number',

'minimum' => 0,

'maximum' => 100,

),

+1 for introducing a new field and deprecating the old one – the existing plugins will probably not handle the new values correctly. FYI @spacedmonkey @TimothyBJacobs

As for the deprecation, I think the description method you suggested works nicely. I didn't find many other examples anyway:

> cd wordpress-develop > git grep -i deprecated | grep rest

It looks like we are in agreement on the way to move forward, which ensures that we optimize for backward compatibility. My only concern now is how to name new fields, so we remove the ambiguity between the existing styles (Block style variations.) and style (Block type front end and editor style handle.). We definitely can't rename style to styles 😞

Can handles be used in the property name?

Great thinking @TimothyBJacobs, it's exactly handles that we store there 👍🏻

I have a working prototype with f48b1c1. I still need to refactor the rest of the code to use new properties.

Everything is ready for the final review.

src/wp-includes/blocks.php

aristath · 2022-08-19T10:34:14Z

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 · 2022-08-19T11:18:17Z

@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' />

So something isn't right yet.

adamziel · 2022-08-19T11:24:11Z

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 · 2022-08-19T11:51:04Z

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 so we no longer assume that the first style handle on the list is the hardcoded path for that block.

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.

adamziel · 2022-08-22T08:01:38Z

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 · 2022-09-13T12:34:17Z

@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 ignored
only the first style handle passed gets special treatment
additional style handles need to be registered elsewhere

gziolo · 2022-09-13T12:49:01Z

@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 · 2022-09-13T13:04:09Z

@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 😊

gziolo · 2022-09-13T13:10:06Z

src/wp-includes/class-wp-block-type.php

+			return;
+		}
+
+		$new_name = $name . '_handles';


In the future, we might want to use some sort of deprecation mechanism similar to _deprecated_argument for functions/methods. It would be a great opportunity to share the message with those block developers that still use register_block_type to pass an array with the deprecated properties instead of block.json. In case blocks work with block.json already, they will automatically switch to the new class properties.

gziolo · 2022-09-14T11:34:37Z

@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.

gziolo self-assigned this Aug 18, 2022

This was referenced Aug 18, 2022

Passing array of styles causes PHP Warnings and Notices WordPress/gutenberg#43086

Closed

Block API WordPress/gutenberg#41236

Open

gziolo commented Aug 19, 2022

View reviewed changes

gziolo mentioned this pull request Aug 19, 2022

Blocks: Further improve assets handling for blocks WordPress/gutenberg#33542

Closed

11 tasks

gziolo commented Aug 19, 2022

View reviewed changes

src/wp-includes/blocks.php Outdated Show resolved Hide resolved

gziolo changed the title ~~Blocks: Allow registering multiple assets for all supported types~~ Blocks: Allow registering multiple items for all supported asset types Aug 19, 2022

adamziel mentioned this pull request Aug 19, 2022

Allow the style property of block.json to be an array and add support for object-based block styles #2853

Closed

This was referenced Sep 6, 2022

Sync Gutenberg packages #3154

Closed

Plugin: Backport PHP changes for WordPress 6.1 release WordPress/gutenberg#43440

Closed

gziolo force-pushed the update/multiple-assets-support branch 2 times, most recently from 709ed1b to a6b0e7d Compare September 12, 2022 13:55

spacedmonkey self-requested a review September 12, 2022 14:03

gziolo force-pushed the update/multiple-assets-support branch 2 times, most recently from 4cde19d to b99cb16 Compare September 12, 2022 15:50

gziolo added 6 commits September 13, 2022 12:17

Blocks: Allow registering multiple assets for all supported types

15c305e

Fix linting errors and failing tests

0579236

Update src/wp-includes/blocks.php

3bae40d

Refactor code to use deprecated block type properties

6712419

Refactor block registration processing to use new properties

d56a061

Use new fields when enqueuing styles and scripts

772fb62

gziolo force-pushed the update/multiple-assets-support branch from b99cb16 to 772fb62 Compare September 13, 2022 11:47

gziolo commented Sep 13, 2022

View reviewed changes

gziolo requested review from dream-encode, getsource and jeffpaul September 13, 2022 21:30

gziolo closed this Sep 14, 2022

gziolo deleted the update/multiple-assets-support branch September 14, 2022 11:34

gziolo mentioned this pull request Sep 14, 2022

Docs: Include information about multiple scripts support added in WP 6.1 WordPress/gutenberg#44155

Merged

cbravobernal mentioned this pull request Oct 3, 2022

[WP 6.1] Navigation overlay menu does not open WordPress/gutenberg#44638

Closed

This was referenced Oct 19, 2022

register_block_type: Accept editor_script array for back-compat #3487

Closed

PHP unit tests are failing WordPress/gutenberg#45254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Allow registering multiple items for all supported asset types #3108

Blocks: Allow registering multiple items for all supported asset types #3108

gziolo commented Aug 18, 2022 •

edited

Loading

gziolo Aug 19, 2022

dlh01 Aug 22, 2022

gziolo Aug 26, 2022

gziolo Aug 29, 2022

adamziel Aug 29, 2022 •

edited

Loading

gziolo Aug 30, 2022 •

edited

Loading

TimothyBJacobs Aug 30, 2022

gziolo Aug 30, 2022

gziolo Sep 9, 2022

gziolo Sep 13, 2022

aristath commented Aug 19, 2022 •

edited

Loading

adamziel commented Aug 19, 2022

adamziel commented Aug 19, 2022 •

edited

Loading

gziolo commented Aug 19, 2022 •

edited

Loading

adamziel commented Aug 22, 2022

gziolo commented Sep 13, 2022 •

edited

Loading

gziolo commented Sep 13, 2022

ockham commented Sep 13, 2022

gziolo Sep 13, 2022 •

edited

Loading

gziolo commented Sep 14, 2022

	'rotation' => array(
	'description' => __( 'The amount to rotate the image clockwise in degrees. DEPRECATED: Use `modifiers` instead.' ),
	'type' => 'integer',
	'minimum' => 0,
	'exclusiveMinimum' => true,
	'maximum' => 360,
	'exclusiveMaximum' => true,
	),
	'x' => array(
	'description' => __( 'As a percentage of the image, the x position to start the crop from. DEPRECATED: Use `modifiers` instead.' ),
	'type' => 'number',
	'minimum' => 0,
	'maximum' => 100,
	),
	'y' => array(
	'description' => __( 'As a percentage of the image, the y position to start the crop from. DEPRECATED: Use `modifiers` instead.' ),
	'type' => 'number',
	'minimum' => 0,
	'maximum' => 100,
	),
	'width' => array(
	'description' => __( 'As a percentage of the image, the width to crop the image to. DEPRECATED: Use `modifiers` instead.' ),
	'type' => 'number',
	'minimum' => 0,
	'maximum' => 100,
	),
	'height' => array(
	'description' => __( 'As a percentage of the image, the height to crop the image to. DEPRECATED: Use `modifiers` instead.' ),
	'type' => 'number',
	'minimum' => 0,
	'maximum' => 100,
	),

Blocks: Allow registering multiple items for all supported asset types #3108

Blocks: Allow registering multiple items for all supported asset types #3108

Conversation

gziolo commented Aug 18, 2022 • edited Loading

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

gziolo Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aristath commented Aug 19, 2022 • edited Loading

adamziel commented Aug 19, 2022

adamziel commented Aug 19, 2022 • edited Loading

gziolo commented Aug 19, 2022 • edited Loading

adamziel commented Aug 22, 2022

gziolo commented Sep 13, 2022 • edited Loading

gziolo commented Sep 13, 2022

ockham commented Sep 13, 2022

gziolo Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

gziolo commented Sep 14, 2022

gziolo commented Aug 18, 2022 •

edited

Loading

adamziel Aug 29, 2022 •

edited

Loading

gziolo Aug 30, 2022 •

edited

Loading

aristath commented Aug 19, 2022 •

edited

Loading

adamziel commented Aug 19, 2022 •

edited

Loading

gziolo commented Aug 19, 2022 •

edited

Loading

gziolo commented Sep 13, 2022 •

edited

Loading

gziolo Sep 13, 2022 •

edited

Loading