Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 6 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 18, 2022

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.

'description' => __( 'Editor script handle.' ),
'type' => array( 'string', 'null' ),
'description' => __( 'Editor script handles.' ),
'type' => array( 'array', 'string', 'null' ),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'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,
),

Copy link
Contributor

@adamziel adamziel Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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

Copy link
Member Author

@gziolo gziolo Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can handles be used in the property name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is ready for the final review.

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@gziolo 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
@aristath
Copy link
Member

aristath commented Aug 19, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

adamziel commented Aug 19, 2022

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
Copy link
Member Author

gziolo commented Aug 19, 2022

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
Copy link
Contributor

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 gziolo force-pushed the update/multiple-assets-support branch 2 times, most recently from 709ed1b to a6b0e7d Compare September 12, 2022 13:55
@gziolo gziolo force-pushed the update/multiple-assets-support branch 2 times, most recently from 4cde19d to b99cb16 Compare September 12, 2022 15:50
@gziolo gziolo force-pushed the update/multiple-assets-support branch from b99cb16 to 772fb62 Compare September 13, 2022 11:47
@gziolo
Copy link
Member Author

gziolo commented Sep 13, 2022

@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
Copy link
Member Author

gziolo commented Sep 13, 2022

@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
Copy link
Contributor

ockham commented Sep 13, 2022

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

return;
}

$new_name = $name . '_handles';
Copy link
Member Author

@gziolo gziolo Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

gziolo commented Sep 14, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants