-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
'description' => __( 'Editor script handle.' ), | ||
'type' => array( 'string', 'null' ), | ||
'description' => __( 'Editor script handles.' ), | ||
'type' => array( 'array', 'string', 'null' ), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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, | |
), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
😞
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍🏻
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Brilliant! Thank you so much for working on this @gziolo 👍 |
@gziolo The following block.json:
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. |
This code in // 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" );
} |
@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 |
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. |
709ed1b
to
a6b0e7d
Compare
4cde19d
to
b99cb16
Compare
b99cb16
to
772fb62
Compare
The way I approached core blocks, it works as follows:
|
@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'; |
There was a problem hiding this comment.
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.
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. |
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
, andviewScript
in theblock.json
metadata file. @aristath already added that capability forstyle
andeditorStyle
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 forstyle
,editorStyle
so multiple assets are supported natively inWP_Block_Type
class and in the REST API endpoint for block types. The same implementation is mirrored for scripts:editorScript
,script
andviewScript
.Before
After
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.