#62267 closed enhancement (fixed)
Allow registering block type collections with a single function call
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Editor | Keywords: | has-patch has-unit-tests has-dev-note |
Focuses: | performance | Cc: |
Description
#62002 introduced wp_register_block_metadata_collection()
which allows to improve performance of block type registration by using a built PHP manifest file instead of individual block.json
files for the block type metadata coming from a single source (e.g. Core or a plugin). See also the dev note.
With this function available, it feels somewhat odd that developers still have to individually call register_block_type()
or register_block_type_from_metadata()
to actually register their blocks, when wp_register_block_metadata_collection()
already has all the context needed to do so for most scenarios.
Note: We can't change wp_register_block_metadata_collection()
to automatically register the block types, since that would be a backward compatibility break. More importantly, it is still possible to pass $args
to register_block_type()
and register_block_type_from_metadata()
, so we don't know whether the developer would want to use that or not.
Based on all the reference code out there though, it's reasonable to assume that the vast majority of block types is registered purely from block.json
metadata and without additional $args
. Therefore, it would be reasonable to give those developers the option to register all their blocks with just the single block collection registration call.
Allowing this would be a nice DX enhancement, as it would allow developers to rely entirely on JS changes afterwards. You could literally introduce another block type in your plugin and wouldn't need to modify PHP at all. Today, the only way to accomplish a JS-only approach for this would be to e.g. use glob()
in PHP to find all block.json
files, but that has a performance penalty and would kind of defeat the purpose of what wp_register_block_metadata_collection()
is trying to solve.
cc @gziolo @mreishus who I discussed this with ahead of opening this ticket
Change History (18)
#2
@
2 months ago
- Keywords needs-patch needs-unit-tests added
- Owner set to flixos90
- Status changed from new to assigned
I'm planning to get back to working on a PR for this ticket either this or next week.
@gziolo @mreishus Any additional considerations for the approach on this? Are you on board with the auto_register => true
approach?
This ticket was mentioned in PR #8129 on WordPress/wordpress-develop by @flixos90.
2 months ago
#3
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
This PR introduces a new function wp_register_block_types_from_metadata_collection()
which can be used to register all block types from a block metadata collection.
It can be used in two different ways:
- Either to register all block types from a previously registered metadata collection, by referencing its path:
wp_register_block_types_from_metadata_collection( WP_PLUGIN_DIR . '/my-plugin/blocks' )
- In this scenario, the block metadata collection must have been registered prior, via
wp_register_block_metadata_collection( WP_PLUGIN_DIR . '/my-plugin/blocks', /* manifest file path */ )
. Otherwise this call would fail with a warning.
- In this scenario, the block metadata collection must have been registered prior, via
- Or by registering a new block metadata collection and then registering all its block types in the same function call:
wp_register_block_types_from_metadata_collection( WP_PLUGIN_DIR . '/my-plugin/blocks', WP_PLUGIN_DIR . '/my-plugin/block-manifest-json.php' )
For additional context on what block metadata collections are and how to use them, see the WordPress 6.7 dev note about block metadata collections.
Trac ticket: https://core.trac.wordpress.org/ticket/62267
@flixos90 commented on PR #8129:
2 months ago
#4
@gziolo @mreishus When looking at the code as I was starting to work on this, the idea of a new arguments array to also register the block types as part of the existing wp_register_block_metadata_collection()
function seemed not ideal to me. I think a new function (like wp_register_block_types_from_metadata_collection()
) is a more elegant solution as it separates concerns better. The two functions can be used together, but if someone wants to do it all in one function they can. And while in the latter case, the two functions still get intertwined, at least there is a clear difference in intent:
- One function's purpose is to register a block metadata collection.
- The other function's purpose is to register block types from a block metadata collection.
Please let me know what you think about this approach.
---
The other thing I'm wondering about is whether we can possibly use this for the Core blocks. The caveat is that many core blocks still provide a custom render_callback
argument that is not possible to handle that way via block.json
. Very few Core blocks also specify skip_inner_blocks
, but I think those two arguments is all I could find.
A potential solution I thought about is that we could allow an optional callback function to dynamically compute additional $args
based on the given block type name. For Core's usage, such a function implementation could look like this:
function get_args_for_block_type( string $block_name ) {
$args = array();
$function_name = 'render_block_core_' . str_replace( '-', '_', $block_name );
if ( function_exists( $function_name ) ) {
$args['render_callback'] = $function_name;
}
$blocks_to_skip_inner_blocks = array( /* Block name list. */ );
if ( in_array( $block_name, $blocks_to_skip_inner_blocks, true ) ) {
$args['skip_inner_blocks'] = true;
}
return $args;
}
Do you think something like this would be worthwhile? Or is it overkill and we should instead update to use block.json
for everything and encourage others to do so too?
@swissspidy commented on PR #8129:
2 months ago
#5
Dumb question, but why is block type registration made so complicated in the first place? Why didn't we support something like the following?
foreach ( require 'path/to/blocks-json.php' as $block_type => $metadata ) {
register_block_type_from_metadata( $block_type, $metadata );
}
This would help me better understand the context of this PR for doing a review.
Right now my first impression is that this is overly complex and that WP_Block_Metadata_Registry
is just a glorified global variable and file reader.
@flixos90 commented on PR #8129:
6 weeks ago
#6
@swissspidy
Dumb question, but why is block type registration made so complicated in the first place? Why didn't we support something like the following?
foreach ( require 'path/to/blocks-json.php' as $block_type => $metadata ) { register_block_type_from_metadata( $block_type, $metadata ); }
If you're referring to the use of JSON files, I don't have history on why it was made like that. I assume part of it was that the approach of JSON files is good for portability between PHP and JavaScript, but the performance consideration of reading and parsing dozens of individual JSON files on every page load only became evident as an issue later. If you're referring to the implementation in this PR, I don't understand why you refer to it as "complicated", given what's proposed here is a single function call (no foreach
loop necessary). Can you clarify what you consider complicated about it?
In any case, we wouldn't be able to change the foundation of how block types are registered without breaking backward compatibility. So this PR and ticket aim to make it more convenient to register multiple blocks from one source (e.g. one plugin) with a single function call, instead of having to add a separate function call for every block type. It improves DX because you would no longer have to modify general plugin files when you add a new block. After the initial scaffolding of your (multi-)block plugin, you could simply create a new directory for each block you need and implement the respective block.json
, view.js
, render.php
etc. files, and the block type would be registered "automatically" via this single collection registration call.
Right now my first impression is that this is overly complex and that
WP_Block_Metadata_Registry
is just a glorified global variable and file reader.
I'm not sure why it is overly complex, can you clarify? That said, WP_Block_Metadata_Registry
was already released in 6.7, so we may be limited in what can be changed now anyway. But if there are problems to resolve with it, we can try.
@joemcgill commented on PR #8129:
6 weeks ago
#7
If you're referring to the use of JSON files, I don't have history on why it was made like that. I assume part of it was that the approach of JSON files is good for portability between PHP and JavaScript, but the performance consideration of reading and parsing dozens of individual JSON files on every page load only became evident as an issue later.
I agree with this assessment. I also think I follow the theoretical benefit of this API as a DX improvement over manually registering all block types from a manifest file, but would be helpful to see some real use cases of where this would be used. Would you expect core to use this function to register core blocks, for example? Or do you have other examples of third party code that would benefit from this new API?
@flixos90 commented on PR #8129:
6 weeks ago
#8
Would you expect core to use this function to register core blocks, for example? Or do you have other examples of third party code that would benefit from this new API?
Core could definitely use it, although the main benefit of the API I see for any plugins using the newer render.php
file via block.json
approach for its blocks (in contrast to "manual" loading of PHP files with block rendering callback functions).
But yeah, the Core blocks could still in principle remove all their individual register_block_type_from_metadata()
calls and replace it with a single central wp_register_block_types_from_metadata_collection()
call covering all blocks. For Core, this would be pending something like the idea I outlined in https://github.com/WordPress/wordpress-develop/pull/8129#issuecomment-2593764966. This would be nice to have, but based on how all the Core blocks are currently implemented, they would still need their block-specific PHP callback function files to be manually require_once
d.
For plugins that register multiple block types using render.php
for them, the benefits are greater because those plugins wouldn't need to write any code other than for files referenced via block.json
. I don't know which plugins this applies to without doing some in-depth search, but it would apply to any plugin using the approach from https://developer.wordpress.org/block-editor/reference-guides/packages/packages-create-block/. In fact, that package itself could be modified to have the block manifest generation built-in and then contain a single PHP function to register all block types from the collection. That way developers could add as many block types as they like and would not need to alter any code outside of the block type's own folder to have it registered.
@swissspidy commented on PR #8129:
4 weeks ago
#9
Dumb question, but why is block type registration made so complicated in the first place? Why didn't we support something like the following?
foreach ( require 'path/to/blocks-json.php' as $block_type => $metadata ) { register_block_type_from_metadata( $block_type, $metadata ); }If you're referring to the use of JSON files, I don't have history on why it was made like that. I assume part of it was that the approach of JSON files is good for portability between PHP and JavaScript, but the performance consideration of reading and parsing dozens of individual JSON files on every page load only became evident as an issue later. If you're referring to the implementation in this PR, I don't understand why you refer to it as "complicated", given what's proposed here is a single function call (no
foreach
loop necessary).
No, I am not referring to JSON files . My examples uses a PHP file (_blocks-json.php
_)
I am also not referring to this PR specifically.
I am talking about the whole WP_Block_Metadata_Registry
in general.
Right now my first impression is that this is overly complex and that
WP_Block_Metadata_Registry
is just a glorified global variable and file reader.
That said,
WP_Block_Metadata_Registry
was already released in 6.7, so we may be limited in what can be changed now anyway. But if there are problems to resolve with it, we can try.
Yes I know it's older, but I want to better understand why it was built like that.
Here's how I read the code (overly simplified!):
wp_register_block_metadata_collection
/WP_Block_Metadata_Registry::register_collection
stores information about a collection and its manifest file in an array for later use.- This collection manifest file is basically a PHP representation of multiple
block.json
files combined in one - You then have to call
register_block_type_from_metadata
for each individual block that you want to register (this PR does this for you now, but it still has to be done) register_block_type_from_metadata
then looks up that block's specific metadata from the combined array originally stored inWP_Block_Metadata_Registry
- The data stored in
WP_Block_Metadata_Registry
is never used again afterwards
What I don't get is why we have WP_Block_Metadata_Registry
in the first place. The only thing it does is storing things in an array so it can be read _once_ when registering blocks. This temporary array storage seems pointless to me. Can't we cut out the middle man?
Take my example again:
foreach ( require 'path/to/blocks-json.php' as $block_type => $metadata ) {
register_block_type_from_metadata( $block_type, $metadata );
}
where blocks-json.php
is the block collection manifest containing all the metadata.
There is no point in having register_block_type_from_metadata
read data from that registry when you can just directly pass it. It's weird to me that register_block_type_from_metadata
doesn't allow it, even though it says "$args Array of block type arguments. Accepts any public property of WP_Block_Type
", even though that's not true. Right now it requires passing a block.json
path or using this registry. So this would need to be fixed.
Again, this is not a critique of this PR, but a general observation about WP_Block_Metadata_Registry
that I would love to better understand.
If it helps I can also try to whip up a PR to explain my thinking.
4 weeks ago
#10
@swissspidy made an interesting point about using the structured PHP file that contains the processed JSON files. The process of getting to this point took a while as the intermediate steps happened.
register_block_type_from_metadata
was never designed to skip the $file_or_folder
param, but eventually it got a capability where the first param gets ignored, and all metadata can be passed inside the $args
array. Initially these additional arguments were used to pass PHP constructs that can't be represented inside JSON object like functions or the data that needs to be dynamically constructed with PHP. It's worth mentioning that the limitation is that any assets like JavaScript or CSS can't be successfully registered when defined as path references because the logic needs to know the location of the block.json
file. We can tweak it further, but this will require additional code changes where we potentially use the folder where the original block.json
lived.
Before we started playing with block collections, there was no concept of blocks-json.php
containing multiple block metadata files combined together where every individual block lives in their folder at the same level of nesting inside the single parent folder. Now that we established that convention, there is something to work with.
Concluding, in the scenario where the block-json.php
is generated in the filesystem, then the approach outlined by @swissspidy might work perfectly fine. In the existing projects, the concept of collection might be easier to incorporate as they historically had a lot of places where these block types were registered at various times in the lifecycle of WordPress. In addition, some of these block types might get registered only in specific circumstances, so I see the place for all of these approaches.
@flixos90 commented on PR #8129:
4 weeks ago
#11
@swissspidy
Take my example again:
foreach ( require 'path/to/blocks-json.php' as $block_type => $metadata ) { register_block_type_from_metadata( $block_type, $metadata ); }where
blocks-json.php
is the block collection manifest containing all the metadata.
I agree that this is worth considering. FWIW, I would still say we should have a simple API for it if we go this route (instead of requiring plugin developers to write the above), which could be as simple as re-implementing the wp_register_block_types_from_metadata_collection()
function introduced here to have the code from your above snippet. This would allow central control for things like validation.
Let's continue discussing which path we should go with this.
There is no point in having
register_block_type_from_metadata
read data from that registry when you can just directly pass it. It's weird to me thatregister_block_type_from_metadata
doesn't allow it, even though it says "$args Array of block type arguments. Accepts any public property ofWP_Block_Type
", even though that's not true. Right now it requires passing ablock.json
path or using this registry. So this would need to be fixed.
I think there are two reasons it was implemented this way:
- The first one @gziolo already outlined, while
register_block_type_from_metadata
supports$args
, it's primarily intended to get the data directly from the metadata file. I agree this is probably not the greatest design for this function, but I guess that's how it evolved over time. I think thatblock.json
files are the recommended way to provide block type metadata as of today, and around this paradigm this is built. - The other reason it was implemented as its own registry is that
register_block_type_from_metadata
already existed and _must_ be called to register a block type. On the other handwp_register_block_metadata_collection
_can_ be called.register_block_type_from_metadata
already looks up the correspondingblock.json
file and loads the metadata from it. Implementing a lookup mechanism for that metadata fit nicely into the existing implementation and, more importantly for developers, means that allregister_block_type_from_metadata
can remain exactly how they are. Developers are free to _additionally_ register their block metadata collections to improve performance (by avoiding JSON parsing etc.), but it's optional (and has to be, otherwise it would be a BC break).
What you're proposing would definitely be simpler than the current approach. I would argue that it moves us closer to a situation where developers are more strongly encouraged to generate a PHP file from their block.json
file. This is probably not a bad thing, especially for performance. Though today probably very few developers are familiar with this technique yet, because it was only added in WordPress 6.7 and requires using separate tooling such as the one recently added to @wordpress/scripts
. The current approach of a metadata registry where this PHP file is loaded and then relevant block metadata is looked up allows for a more gradual transition to better performance because it's decoupled from the actual block registration.
To be clear, I personally am not strongly in favor of either approach, they both have tradeoffs. I think your overall point is great in that we should be simplifying this. A potential solution to consider would be to continue with the current WP_Block_Metadata_Registry
approach for a transition period. We could eventually deprecate the approach of using block.json
metadata files in PHP directly and ask developers to always pass $args
instead - which technically would not mean we would want them to manually provide the arguments, but that we would want them to use the generated PHP file from all their block.json
files. This would slowly bring us closer to a future where developers use the API like in your code example. At some point further down the road we could then consider deprecating/removing the entire WP_Block_Metadata_Registry
approach in favor of always directly passing the metadata from the generated PHP file as $args
.
@flixos90 commented on PR #8129:
3 weeks ago
#12
@swissspidy Could you take another look at this PR? I still think in its current state it would be good to land in 6.8, and it already has an approval.
We should continue the discussion above in its own ticket, and as mentioned I see your point. But for registering block types from a collection with a single function call IMO shouldn't be blocked on the internals of how we're implementing it, given the internals (WP_Block_Metadata_Registry
) are already present in a previously shipped WordPress Core release.
The function added here (wp_register_block_types_from_metadata_collection()
) could be rewritten in the future if we revised the approach and got rid of the WP_Block_Metadata_Registry
class for example, without changing its signature.
So I think this could be merged, but wanted to check with you first.
@swissspidy commented on PR #8129:
3 weeks ago
#13
To clarify, this was never a blocker, I was just trying to better understand the whole code, as I realized how bad that whole API design is.
@flixos90 commented on PR #8129:
2 weeks ago
#14
@gziolo Thanks, and great point! I opened https://core.trac.wordpress.org/ticket/63027 as a follow up ticket for us to review throughout the block type registration logic.
#16
@
2 weeks ago
- Keywords needs-dev-note added
Adding needs-dev-note
since this new API function should be promoted and explained to plugin developers.
#17
@
10 days ago
Dev note draft is ready for review: https://docs.google.com/document/d/1WWQc2Aj5IEFJqmFsyZ2rD_TbtqPXgOav71KzMu3L-Es/edit
cc @gziolo @joemcgill @swissspidy
What @gziolo @mreishus and I discussed in Slack so far was to potentially add a flag to the new function, e.g. like so:
wp_register_block_metadata_collection( WP_PLUGIN_DIR . '/my-block-library/build', WP_PLUGIN_DIR . '/my-block-library/build/blocks-manifest.php', array( 'auto_register' => true ) );
This alone would be a quite straightforward change to make and already address what's outlined in the ticket.
Additionally though, there would be value in combining the block type asset metadata into this process as well. Today, many block type assets still need to be individually registered, and block types only reference the asset handles registered in PHP. For the registration, it is already established to generate files with the metadata (
{file}.asset.php
), so potentially this could be integrated intoblock.json
files (and thus also the built PHP manifest) to simplify scaffolding a block, and in particular the block registration code needed, even more. See related Gutenberg issue https://github.com/WordPress/gutenberg/issues/46954.