Extend Block Metadata PHP Cache to Third-Party Blocks #7303

mreishus · 2024-09-06T14:22:35Z

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

To run relevant unit tests: ./vendor/bin/phpunit --filter register ; ./vendor/bin/phpunit --filter Tests_Blocks_WpBlockMetadataRegistry

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.

github-actions · 2024-09-06T14:22:51Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mreishus, azaozz, flixos90, mukesh27, gziolo, spacedmonkey.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-09-06T14:48:50Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

azaozz · 2024-09-12T23:01:35Z

src/wp-includes/class-wp-block-metadata-registry.php

+	 * @since 6.X.0
+	 * @var WP_Block_Metadata_Registry
+	 */
+	private static $instance;


Would it be better to make this a (proper) static class, no instances? If I'm reading this right there are no benefits to instantiate it.

Agree that there is no need to instantiate it. Changed all methods to be static in 48e2bfa and prevented instantiation.

azaozz · 2024-09-12T23:05:17Z

src/wp-includes/blocks.php

+		if ( $metadata_file_exists ) {
+			$metadata = wp_json_file_decode( $metadata_file, array( 'associative' => true ) );
+		} else if ( ! $metadata_file_exists && empty( $args['name'] ) ) {
+			return false;


Would it make sense to somehow register the fact that a metadata file doesn't exist so it is cached? Seems that should prevent looking for a non-existing file on every page load.

I don't think this will be much of an issue. We only do the file_exists() check if $metadata is empty, and $metadata is empty only if we failed to get the data from the metadata registry. In other words, I only expect the file_exists() check to fail if something has gone wrong, and it shouldn't be looking for non existent files in normal operation.

gziolo · 2024-09-13T09:24:15Z

The recommended and most popular usage for block registration is through register_block_type, which uses register_block_type_from_metadata internally when the path to block.json is passed as the first argument. Example:

function my_plugin_my_block_block_init() {
	register_block_type( __DIR__ . '/build' );
}
add_action( 'init', my_plugin_my_block_block_init' );

There were some refactorings in the past that I'm not deeply familiar with, but that made the implementation more flexible and more performant. In the world where we have the proposed cache registry for metadata, it would get to another level. Although, it also raises the question of whether we can simplify everything. I took a look at how it all works today.

This is an example of core block registration:

wordpress-develop/src/wp-includes/blocks/index.php

Lines 149 to 157 in a78540b

    
           function register_core_block_types_from_metadata() { 
        
           	$block_folders = require BLOCKS_PATH . 'require-static-blocks.php'; 
        
           	foreach ( $block_folders as $block_folder ) { 
        
           		register_block_type_from_metadata( 
        
           			BLOCKS_PATH . $block_folder 
        
           		); 
        
           	} 
        
           } 
        
           add_action( 'init', 'register_core_block_types_from_metadata' );

There are multiple steps here in WP core:

Use webpack to generate require-static-blocks.php file
Use also webpack to generate the cache blocks-json.php with all block.json files loaded
Register the block type by providing the path to metadata based on paths generated with (1). Metadata is consumed from file (2), which has everything bundled, with keys being core block names.

Some aspects I was thinking about, as register_block_type_from_metadata has the following steps after many iterations:

Load the block.json file from the disk or from the cache from core blocks
Translate the metadata to block settings, which means either mapping the properties or processing them so some additional work happens, like registering scripts and styles, callbacks, etc

Register the block type in the registry:

wordpress-develop/src/wp-includes/blocks.php

Lines 707 to 710 in a78540b

    
           return WP_Block_Type_Registry::get_instance()->register( 
        
           	$metadata['name'], 
        
           	$settings 
        
           );

What I'm currently thinking about is whether we can keep the existing public API. In effect, we would need a single global registry where the metadata gets stored. $file_or_folder could become the key for setting and getting the entry from the registry:

function register_block_type_from_metadata( $file_or_folder_or_cache_key, $args = array() ) {
	// Try to get metadata from the static cache for core blocks.
	$metadata = array();    
	$registry = WP_Block_Metadata_Registry::get_instance();
	if ( $registry->has( $file_or_folder_or_cache_key ) {
		$metadata = $registry->get( $file_or_folder_or_cache_key );
	} else {
		// Load the file from the disk as before.
	}
	// The rest of the logic.
}

I assumed that the file no longer needs to exist on the disk and instead anything can be put as a cache key so I renamed the first param to $file_or_folder_or_cache_key.

The registry itself could have some helper method to register multiple block type metadata entries in one call, so for WP core that would be the following:

$registry = WP_Block_Metadata_Registry::get_instance();
$registry->addCollection( require ABSPATH . WPINC . '/blocks/blocks-json.php' );

I'm not entirely sure how that would work with the Gutenberg plugin, but we can figure it out later. Let me know what do you think about all of the above. I'm curious about the rationale for having namespaces in the metadata registry that in effect would trigger adding the 3rd param. I'm happy to discuss it further to align on the path forward.

mreishus · 2024-09-13T22:20:25Z

Thank you for the feedback, this is very helpful in refining the approach.

Simplifying the API
I agree with your suggestion to simplify the API. The idea of using a single global registry with $file_or_folder_or_cache_key as the first parameter makes sense and would allow us to remove the third parameter I initially proposed. This approach seems cleaner and more in line with existing WordPress conventions. I have modified the code to give this a try.

Namespaces in the metadata registry
I initially introduced namespaces due to concerns about potential conflicts, particularly noticing that both core and the Gutenberg plugin can have differing metadata for the same blocks. However, I'm open to removing namespaces if we can address the potential overwriting issues in another way. Perhaps we can ensure that Gutenberg always registers its metadata after core.

Third-party plugin usage
I did imagine third party plugins having to add their own build step to create a compiled php file with all of the block data. I have a prospective approach for WooCommerce here: woocommerce/woocommerce#51184 (In the next hour or so, I will update it to match the latest changes in this PR).

register_block_type()
I did not know this was the main way to register blocks. In accordance with the API changes above, I have also changed this to feed through and use the same mechanism. If you check the code, you'll see something strange about checking the type of $args - I noticed the unit test test_should_return_a_default_fallback_navigation_menu_with_no_blocks_if_page_list_block_is_not_registered is passing the undocumented type of WP_Block_Type to $args (instead of an array with the same properties as WP_Block_Type). In this case, WP_Block_Type_Registry::get_instance()->register() can handle an $args as WP_Block_Type here, but register_block_type_from_metadata() cannot, so I had to add the safeguard. However, this feels a bit strange and might warrant further inspection.

register all helper
Didn't get a chance to look at this yet.

gziolo · 2024-09-16T11:48:12Z

I did not know this was the main way to register blocks. In accordance with the API changes above, I have also changed this to feed through and use the same mechanism.

The nice part is that we will be able to avoid all the file system checks if the metadata is cached. We definitely will have to adjust register_block_type in the way you did it. I'll have a closer look at the edge case you highlighted later. It sounds like the test might need some adjustments. In my opinion, everything is on the right track.

felixarntz

@mreishus I like the idea where this PR is going, it will be super valuable to have this in Core. That said, I think we can simplify the implementation - most importantly, supporting yet another possible type of value for the $block_type parameter of register_block_type() makes usage more confusing, and the code more complex.

I think we should double down on the idea of having the registry be based on an entire collection of blocks (most typically, the collection of all blocks within one plugin). See more detailed feedback inline.

felixarntz · 2024-09-23T16:19:33Z

src/wp-includes/blocks.php

+/**
+ * Registers block metadata from a given source.
+ *
+ * This function allows core and third-party plugins to register their block metadata
+ * in a centralized location. Registering metadata can improve performance by avoiding
+ * multiple reads from the filesystem.
+ *
+ * @since 6.X.0
+ *
+ * @param string $source    The source identifier for the metadata within the namespace.
+ *                          This can be a unique identifier for your plugin's blocks.
+ * @param array  $metadata  The block metadata to be registered.
+ */
+function wp_register_block_metadata( $source, $metadata ) {
+	WP_Block_Metadata_Registry::register( $source, $metadata );
+}


Rather than a function to register block metadata for a single block, I think we should go with the approach of registering a block collection file, as @gziolo suggested in #7303 (comment).

This is more performant when it comes to plugins with multiple blocks, but still works for plugins with a single block as well.

Last but not least, it more intuitively separates the API since a "block collection" would be clearly different from an individual "block" (while a function name like the current one may be confused with the actual functions to register a block type, which in a way also register block metadata depending on how they're called).

felixarntz · 2024-09-23T16:31:28Z

src/wp-includes/blocks.php

@@ -387,49 +404,60 @@ function get_block_metadata_i18n_schema() {
 * @since 6.5.0 Added support for `allowedBlocks`, `viewScriptModule`, and `viewStyle` fields.
 * @since 6.7.0 Allow PHP filename as `variations` argument.
 *
- * @param string $file_or_folder Path to the JSON file with metadata definition for
+ * @param string $file_or_metadata_source Path to the JSON file with metadata definition for


I think we should try to find a way to keep this parameter the way it was before. It already supports file or folder, and supporting yet another way to call it makes this function more and more complex.

We could consider to shape the block collection mechanism in a way that continues to support passing a file or folder, for example:

Allow registering block collections as a combination of:

Directory in which block.json files will be considered part of that collection.

The built PHP file name which contains the block metadata for all of the blocks in the collection.

With such an approach, you could look up the correct collection for the given file or folder (if there is one registered), and then on demand require the built PHP file, to read the metadata from there.

This would simplify the usage and implementation of this function. It would also allow us to handle Core blocks in the same way as third party blocks.

Example code to illustrate what I mean (with the example of how Core itself would use this):

WP_Block_Metadata_Registry::register_collection( ABSPATH . WPINC . '/blocks/', ABSPATH . WPINC . '/blocks/blocks-json.php' );

The WP_Block_Metadata_Registry::get_metadata() method would then receive a file or folder with a block.json file and find the correct registered collection, i.e. the one where starts_with( $file_or_folder, $registered_blocks_folder ). If it finds one, it would require $registered_blocks_php_file (only once of course, otherwise it would look at the already parsed metadata from that file).

This way, register_block_type_from_metadata() can continue to use $file_or_folder as is and we could even remove some of the Core specific code from it.

Thanks, this is a good idea and I'll give it a shot. One thing I want to nail down is the matching of specific blocks from the registered collection and later register_block_type_from_metadata() calls.

To use a concrete example, we might call:

WP_Block_Metadata_Registry::register_collection( '/srv/http/wordpress/wp-includes/blocks/', '/srv/http/wordpress/wp-includes/blocks/blocks-json.php' )

The blocks-json.php file is unchanged and is formatted like:

<?php return array( 'archives' => array( 'name' => 'core/archives', 'title' => 'Archives', // ..properties omitted.. ), 'audio' => array( 'name' => 'core/audio', 'title' => 'Audio', // ..properties omitted.. ), 'avatar' => array( 'name' => 'core/avatar', 'title' => 'Avatar', // ..properties omitted.. ), // ..other blocks.. );

In this case, to match up block names, we would look for calls like:

register_block_type_from_metadata( '/srv/http/wordpress/wp-includes/blocks/archives' ); OR register_block_type_from_metadata( '/srv/http/wordpress/wp-includes/blocks/archives.json' );

In each of these cases we would:

Extract the last directory of the path (archives) and use this as our key

Extract the path except the last directory (/srv/http/wordpress/wp-includes/blocks) and use this as a collection

For the collection, we'd check if the collection is registered and if the data read from the provided PHP file had an array key matching the last part of our path (archives), and whatever is found there would be used as the block metadata.

Does that sound right? I considered some other ideas, like matching block names and looping through collections looking for them, but that doesn't seem to work, especially with core and the Gutenberg plugin registering different metadata for blocks with the same name.

( The only tricky bit I've found so far is that the Gutenberg plugin does some registrations with ../ in them, like /srv/http/wordpress/wp-content/plugins/gutenberg/lib/../build/block-library/blocks/group/block.json, while others do not, like /srv/http/wordpress/wp-content/plugins/gutenberg/build/block-library/blocks/categories. )

Great question, I hadn't yet thought about how to map the PHP file contents back to the specific block name.

I think the following would work well:

If registered with something like /srv/blocks/archives/block.json or just a directory /src/blocks/archives (in which case the file name of block.json is assumed), we use the last directory (i.e. archives in this example).

If registered with a path to a JSON file that is not block.json, we use the name of that file, e.g. /srv/blocks/button.json will lead to using button.

These would work well as defaults, but I think we should also support alternatives since both of the above are, at the end of the day, still just assumptions how people will use this. So I think a good idea would be for WP_Block_Metadata_Registry::register_collection() to support an optional argument for a callback on how to parse a block JSON file name into the block's identifier within the built PHP file array. This would be more advanced to use, but the good thing is that most developers wouldn't need it since they probably use one of the two previous approaches that we would support by default.

For knowing which collection a specific block JSON file belongs to, I think we could always rely on a check whether it starts with a specific collection's registered root directory. We could add some safe guards to WP_Block_Metadata_Registry::register_collection() so that you can't register a path that will cause problems - for example nobody should ever register WP_PLUGIN_DIR as a collection, as that would mean every single block in every plugin would fall under this hypothetical collection. In other words, only a path within a specific plugin directory should be allowed in this context (e.g. WP_PLUGIN_DIR . '/gutenberg/blocks').

I've just pushed an implementation of the 'last directory' idea to this PR. In my testing, it works for all of core, and about 80% of WooCommerce. The remaining 20% that doesn't work are in two tiered directories, I think.

Will look at some of these other ideas tomorrow.

If I'm reading the current version of the function correctly, when passing a file or directory, it only supports block.json files or directories [link]. It does not support files like arbitrary_string.json. Keeping this constraint might allow the new code to be simpler and avoid disk access (ie, not checking is_dir()).

Ah you're right, good point. In that case, it's probably fine to always go with the last directory above the block.json file to get the block name as the default behavior. We could add support for an optional custom callback to manually determine this later, doesn't have to be in the first iteration of the API.

felixarntz · 2024-09-23T16:36:17Z

src/wp-includes/blocks.php

-	$is_core_block = str_starts_with( $file_or_folder, ABSPATH . WPINC );
-	// If the block is not a core block, the metadata file must exist.
-	$metadata_file_exists = $is_core_block || file_exists( $metadata_file );
-	if ( ! $metadata_file_exists && empty( $args['name'] ) ) {
-		return false;
+	// Determine if we're dealing with a file/folder or a metadata source
+	if ( WP_Block_Metadata_Registry::has_metadata( $file_or_metadata_source ) ) {
+		$metadata_source = $file_or_metadata_source;
+		$file_or_folder = null;
+	} else {
+		$metadata_source = null;
+		$file_or_folder = $file_or_metadata_source;
 	}

-	// Try to get metadata from the static cache for core blocks.
-	$metadata = array();
+	$is_core_block = $file_or_folder && str_starts_with( $file_or_folder, ABSPATH . WPINC );
+
 	if ( $is_core_block ) {
-		$core_block_name = str_replace( ABSPATH . WPINC . '/blocks/', '', $file_or_folder );
-		if ( ! empty( $core_blocks_meta[ $core_block_name ] ) ) {
-			$metadata = $core_blocks_meta[ $core_block_name ];
+		$core_block_name = 'core/' . str_replace( ABSPATH . WPINC . '/blocks/', '', $file_or_folder );
+		$metadata = WP_Block_Metadata_Registry::get_metadata( $core_block_name );
+
+		if ( null === $metadata ) {
+			// Load core metadata if not already registered.
+			$core_blocks_meta = require ABSPATH . WPINC . '/blocks/blocks-json.php';
+			foreach ( $core_blocks_meta as $block_name => $block_meta ) {
+				$block_name = 'core/' . $block_name;
+				wp_register_block_metadata( $block_name, $block_meta );
+			}
+			$metadata = WP_Block_Metadata_Registry::get_metadata( $core_block_name );
 		}
+	} elseif ( $metadata_source ) {
+		$metadata = WP_Block_Metadata_Registry::get_metadata( $metadata_source );
 	}


Based on what I suggested above, all of this could be replaced with a simple:

$metadata = WP_Block_Metadata_Registry::get_metadata( $file_or_folder );

If it finds a collection for the $file_or_folder, it would read its PHP file (only if it had not been read before) and then return the relevant metadata. If it doesn't find one, the function would continue to pass through to the logic below to directly read from the block JSON file.

felixarntz · 2024-09-23T16:38:04Z

src/wp-includes/blocks.php

-	if ( is_string( $block_type ) && file_exists( $block_type ) ) {
+	$passed_metadata_source = false;
+	if ( WP_Block_Metadata_Registry::has_metadata( $block_type ) && ( empty( $args ) || is_array( $args ) ) ) {
+		$passed_metadata_source = true;
+	}
+
+	if ( is_string( $block_type ) &&
+		 ( $passed_metadata_source || file_exists( $block_type ) ) ) {
 		return register_block_type_from_metadata( $block_type, $args );
 	}


See above, I think this change would be unnecessary if we simplified the logic to continue to rely on $file_or_folder for a $block_type registered from block.json metadata.

mreishus · 2024-09-24T22:18:09Z

A lot of changes here from the initial implementation! The register_block_type_from_metadata() changes are looking a lot cleaner now. The core specific code is removed and now uses the new registry. I was also able to get WooCommerce working with it (draft PR here).

I tried to beef up the comments to explain what's happening, but essentially you first register the file with:

WP_Block_Metadata_Registry::register_collection(
  '/srv/http/wordpress/wp-content/plugins/myplugin/blocks/',
  '/srv/http/wordpress/wp-content/plugins/myplugin/blocks/blocks-json.php'
)

That php file ("manifest file" in comments - should this change?) should contain something like:

<?php return array(
  'timeline' => array(
    'name' => 'myplugin/timeline',
    'title' => 'Timeline',
    // ..properties omitted..
  ),
  'profile-card' => array(
    'name' => 'myplugin/profile-card',
    'title' => 'Profile Card',
    // ..properties omitted..
  ),
  // ..other unique blocks..
);

Now, if you

register_block_type_from_metadata( '/srv/http/wordpress/wp-content/plugins/myplugin/blocks/timeline' );

or

register_block_type_from_metadata( '/srv/http/wordpress/wp-content/plugins/myplugin/blocks/timeline/block.json' )

(the same interface as before)

It will look in the timeline key of the registered data to find the metadata for that block. The mapping we do from $file_or_folder to manifest key is done by taking the directory name of where the block.json file lives. Right now, you can provide an optional callback to change this mapping, but I didn't need it for WooCommerce. I wonder if other large plugins like Gutenberg or Jetpack would need this customization available or not? I haven't researched that yet.

src/wp-includes/blocks.php

felixarntz

@mreishus This looks excellent! A few minor comments, but I think it's close.

src/wp-includes/blocks.php

src/wp-includes/class-wp-block-metadata-registry.php

felixarntz · 2024-09-25T16:58:17Z

@gziolo Would you be able to give this another review as well? Is there anything that Gutenberg would need to change to cater for this once it lands in Core?

mreishus · 2024-09-25T20:10:47Z

One other idea I had was to automatically unset() any metadata after fetching it from the registry, since presumably it would only be needed once, and it would free up a small amount of memory. Not sure about this though.

felixarntz · 2024-09-25T20:37:29Z

One other idea I had was to automatically unset() any metadata after fetching it from the registry, since presumably it would only be needed once, and it would free up a small amount of memory. Not sure about this though.

Probably overkill :)

Also you never know how plugins will use this function, so best to keep the data in place.

felixarntz

@mreishus This looks great. My comments at this point are mostly nit-picks to polish this for commit. :)

Great job on the thorough documentation, I think that's super helpful!

src/wp-includes/blocks.php

felixarntz · 2024-09-25T20:47:53Z

src/wp-includes/blocks.php

+	$registry_metadata = WP_Block_Metadata_Registry::get_metadata( $file_or_folder );
+	// If the block is not registered in the metadata registry, the metadata file must exist.
+	$metadata_file_exists = $registry_metadata || file_exists( $metadata_file );
 	if ( ! $metadata_file_exists && empty( $args['name'] ) ) {
 		return false;
 	}

-	// Try to get metadata from the static cache for core blocks.
-	$metadata = array();
-	if ( $is_core_block ) {
-		$core_block_name = str_replace( ABSPATH . WPINC . '/blocks/', '', $file_or_folder );
-		if ( ! empty( $core_blocks_meta[ $core_block_name ] ) ) {
-			$metadata = $core_blocks_meta[ $core_block_name ];
-		}
-	}
+	// Try to get metadata from the metadata registry.
+	$metadata = $registry_metadata ?? array();

-	// If metadata is not found in the static cache, read it from the file.
+	// If metadata is not found in the metadata registry, read it from the file.
 	if ( $metadata_file_exists && empty( $metadata ) ) {
 		$metadata = wp_json_file_decode( $metadata_file, array( 'associative' => true ) );
 	}


I think we can make this code easier to follow: Right now this is an if else (either use the registry or read from the metadata file) disguised as something else, which makes it harder to understand.

Another note: The $metadata_file_exists variable should continue to rely on whether the file actually exists. We don't know that from whether the $registry_metadata is available. This was different for the previous $is_core_block condition, since for Core we could safely assume that the file is present, but for a plugin we don't know.

Something like:

$metadata_file_exists = file_exists( $metadata_file ); $metadata = WP_Block_Metadata_Registry::get_metadata( $file_or_folder ); if ( ! $metadata ) { if ( ! $metadata_file_exists ) { return false; } $metadata = wp_json_file_decode( $metadata_file, array( 'associative' => true ) ); }

So I think there are three main ways to use the function, via block.json file, via the registry we added, and via $args (can be useful to check cc2133f where this one was added).

I reorganized it in a way that makes sense to me. What do you think? (There aren't comments, but maybe I should add some to make it more clear).

Ideally, I'd like to avoid the file_exists call when using this new method, but $metadata_file_exists does seem to be used later in the function, which I missed previously - good find. My reasoning is I'd like the plugins to be able to register their 50-100 blocks without disk access at all, but this may be acceptable since an existing file_exists() is cached by PHP in a way that the operations done by wp_json_file_decode() are not.

I think we can still reduce some of this code, but logically your current iteration looks good to me. I think we should avoid multiple assignments in if clauses though for $metadata_file_exists, as it can be assigned just one - no need to try to avoid file_exists() checks in the logic when it'll have to happen anyway. As you're mentioning, that shouldn't really be a performance concern.

Update: There's one logical caveat, which probably isn't very relevant, but it's a change from before, which is safer to avoid: We shouldn't assume that any $metadata read from the files is complete. So they check for whether $metadata_file['name'] is set, which is present in the current codebase, needs to be retained.

src/wp-includes/blocks.php

src/wp-includes/class-wp-block-metadata-registry.php

src/wp-includes/blocks.php

src/wp-includes/class-wp-block-metadata-registry.php

gziolo · 2024-09-26T16:34:56Z

Would you be able to give this another review as well? Is there anything that Gutenberg would need to change to cater for this once it lands in Core?

I’m looking at it right now. I need to read the communication to better understand how the metadata collection gets registered and consumed. I still need to see how that relates to Gutenberg and whether there is an easy path to override the metadata cache and avoid block deregistration and registration. However, that is advanced usage so it shouldn’t block this one.

felixarntz

@mreishus This looks great to me! Except one minor logic flaw that needs to be addressed.

src/wp-includes/blocks.php

felixarntz · 2024-09-26T17:15:48Z

I’m looking at it right now. I need to read the communication to better understand how the metadata collection gets registered and consumed. I still need to see how that relates to Gutenberg and whether there is an easy path to override the metadata cache and avoid block deregistration and registration. However, that is advanced usage so it shouldn’t block this one.

Looking at the gutenberg_reregister_core_block_types() function, I don't think this PR would really change anything. The Gutenberg code would work the same as before. So the core blocks would still need to be unregistered to then re-register the Gutenberg equivalents, but as you're saying it's advanced usage anyway.

The one enhancement this would bring is that Gutenberg could now register its own collection file, to get the same performance optimization as Core when re-registering its own blocks. So at least there's that benefit even for this scenario. Still, it's optional, so while we should probably add this to Gutenberg, it's not urgent, and there shouldn't be any breakage from landing this PR in Core.

felixarntz

@mreishus Great work, I think this is practically ready to commit.

If you can, it would be great if you could add 1-2 tests that cover the new logic in register_block_type_from_metadata(). For example, you could register a theoretical block collection and then register a theoretical block from that collection without actually having the block.json file exist, but ensure the block gets still registered because the function relies on the data from the block collection registry.

gziolo · 2024-09-26T18:42:36Z

@felixarntz, the test case you described is what I was contemplating, as we could remove many file checks that aren’t necessary in the case we have everything in the metadata cache. There is some nuance to it with assets that define paths as they are relative to block.json for developers convienience.

I’m writing from mobile, so I can explain that further later.

gziolo · 2024-09-26T19:17:53Z

src/wp-includes/blocks.php

-			$metadata = $core_blocks_meta[ $core_block_name ];
-		}
-	}
+	$metadata_file_exists = file_exists( $metadata_file );


This one is tricky because when reading the metadata from the registry, putting a file on the disk is rather redundant. However, for some reason we still keep these files for core blocks. So, we can keep it as a requirement for now, but I would look further later how to refactor code so it wasn't necessary.

Old code:

// If the block is not a core block, the metadata file must exist. $metadata_file_exists = $is_core_block || file_exists( $metadata_file );

Core block folder:

I found that the existing code wants to know if the file exists so it can support the render and variation keys. I think this is mostly for themes?

$metadata['file'] = $metadata_file_exists ? wp_normalize_path( realpath( $metadata_file ) ) : null;

then:

if ( ! empty( $metadata['render'] ) ) { $template_path = wp_normalize_path( realpath( dirname( $metadata['file'] ) . '/' . // <--- metadata file remove_block_asset_path_prefix( $metadata['render'] ) ) );

and:

if ( ! empty( $metadata['variations'] ) && is_string( $metadata['variations'] ) ) { $variations_path = wp_normalize_path( realpath( dirname( $metadata['file'] ) . '/' . // <--- metadata file remove_block_asset_path_prefix( $metadata['variations'] ) ) );

It's also checked when doing translations:

if ( $metadata_file_exists && $textdomain && isset( $i18n_schema->$key ) ) { $settings[ $mapped_key ] = translate_settings_using_i18n_schema( $i18n_schema->$key, $settings[ $key ], $textdomain ); }

Let's leave it for later, but I think we could see if we can use the folder name instead. If the block needs render which is a file like render.php or variations which is a file like variations.php they need to live in some folder and the path is relative, example:

wordpress-develop/tests/phpunit/tests/blocks/register.php

Lines 967 to 977 in ad1fabe

public function test_register_block_type_from_metadata_with_variations_php_file() {

$filter_metadata_registration = static function ( $metadata ) {

$metadata['variations'] = 'file:./variations.php';

return $metadata;

};

add_filter( 'block_type_metadata', $filter_metadata_registration, 10, 2 );

$result = register_block_type_from_metadata(

DIR_TESTDATA . '/blocks/notice'

);

remove_filter( 'block_type_metadata', $filter_metadata_registration );

wordpress-develop/tests/phpunit/data/blocks/notice/block.json

Line 75 in bbec266

"render": "file:./render.php"

Essentially file:render.php means, relative to where block.json lives, so that could be checked against the physical folder to make all the PHP function calls work if there is no block.json on disk.

gziolo · 2024-09-26T19:23:09Z

src/wp-includes/blocks/index.php

+ * @since 6.7.0
+ */
+function wp_register_core_block_metadata_collection() {
+	wp_register_block_metadata_collection(


So my hope for the future is the following to be possible in Gutenberg:

function gutenberg_register_core_block_metadata_collection() { wp_register_block_metadata_collection( BLOCKS_PATH, GUTENBERG_BLOCKS_PATH . 'blocks-json.php' ); } remove_action( 'init', 'wp_register_core_block_metadata_collection' ); add_action( 'init', 'gutenberg_register_core_block_metadata_collection', 9 );

Oh right! I think technically that already should be possible once this PR lands? 🤔

Co-authored-by: Felix Arntz <[email protected]>

Run with ./vendor/bin/phpunit --filter Tests_Blocks_RegisterBlockTypeFromMetadataWithRegistry

felixarntz · 2024-09-27T21:53:27Z

Seeing mysql Error Head "https://registry-1.docker.io/v2/library/mysql/manifests/8.0": unauthorized: incorrect username or password. I don't think that's related to this PR, I can try rebasing..

Edit: Doesn't seem to have worked :(

Definitely seems unrelated. I wonder if something else in Core is causing this, or it's just some temporary outage or instability somewhere?

This reverts commit 31874e7.

spacedmonkey · 2024-09-27T22:46:03Z

src/wp-includes/class-wp-block-metadata-registry.php

+		$path = wp_normalize_path( rtrim( $path, '/' ) );
+
+		// Check if the path is valid:
+		if ( str_starts_with( $path, wp_normalize_path( ABSPATH . WPINC ) ) ) {


Calling wp_normallze_path is expensive. Can we cache it like this.

wordpress-develop/src/wp-includes/blocks.php

Lines 96 to 99 in a51f2e4

static $wpinc_path_norm = '';

if ( ! $wpinc_path_norm ) {

$wpinc_path_norm = wp_normalize_path( realpath( ABSPATH . WPINC ) );

}

Great idea, addressed in 899bd6f

spacedmonkey · 2024-09-27T22:46:47Z

src/wp-includes/class-wp-block-metadata-registry.php

+		} elseif ( str_starts_with( $path, wp_normalize_path( WP_PLUGIN_DIR ) ) ) {
+			// For plugins, ensure the path is within a specific plugin directory and not the base plugin directory.
+			$plugin_dir    = wp_normalize_path( WP_PLUGIN_DIR );


Repeated called to wp_normalize_path( WP_PLUGIN_DIR ). Cache as variable.

Fixed in 899bd6f

…lize_path

azaozz reviewed Sep 12, 2024

View reviewed changes

mreishus force-pushed the add/metadata-register branch from c60583c to 9f08849 Compare September 13, 2024 21:53

felixarntz reviewed Sep 23, 2024

View reviewed changes

mreishus force-pushed the add/metadata-register branch 2 times, most recently from b50ee43 to 904a156 Compare September 24, 2024 14:36

mreishus requested review from azaozz and felixarntz September 24, 2024 22:18

mukeshpanchal27 reviewed Sep 25, 2024

View reviewed changes

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

felixarntz reviewed Sep 25, 2024

View reviewed changes

mreishus force-pushed the add/metadata-register branch from 7fdc082 to c8156a1 Compare September 25, 2024 18:51

felixarntz reviewed Sep 25, 2024

View reviewed changes

mukeshpanchal27 reviewed Sep 26, 2024

View reviewed changes

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

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

src/wp-includes/class-wp-block-metadata-registry.php Outdated Show resolved Hide resolved

mreishus force-pushed the add/metadata-register branch from 95a0f08 to 8379a8c Compare September 26, 2024 15:43

felixarntz reviewed Sep 26, 2024

View reviewed changes

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

felixarntz approved these changes Sep 26, 2024

View reviewed changes

gziolo reviewed Sep 26, 2024

View reviewed changes

mreishus and others added 23 commits September 27, 2024 16:00

add $last_matched_collection

d7c61c5

align equals

3c489dc

update comments

e48dd39

variable rename

3420e0b

update comment

043ef98

remove private constructor

a07bb1b

add _doing_it_wrong calls

39b1fa4

fix broken tests

dc22cd5

Remove $identifier_callback option

0093108

comment updates

13bab14

make default_identifier_callback private

a3b1277

standardize _doing_it_wrong messages

5ac0229

reorganize top of register_block_type_from_metadata

6f5bf93

fix one more since tag

edab04b

update comment

19b2a41

fix elseif

d006011

default_identifier_callback: Use str_ends_with and return early if empty

86f7ab2

Update src/wp-includes/blocks.php

013bb7d

Co-authored-by: Felix Arntz <[email protected]>

Add Tests_Blocks_RegisterBlockTypeFromMetadataWithRegistry

aa14c49

Run with ./vendor/bin/phpunit --filter Tests_Blocks_RegisterBlockTypeFromMetadataWithRegistry

retab

3c8cea3

add trailing commas

c0537ee

align arrows and equals

4f54092

let core blocks skip the file_exists check

e53dc25

mreishus force-pushed the add/metadata-register branch from 6d75a59 to e53dc25 Compare September 27, 2024 21:00

mreishus added 2 commits September 27, 2024 22:16

minor change to force test rerun

31874e7

Revert "minor change to force test rerun"

a10de78

This reverts commit 31874e7.

mreishus requested a review from spacedmonkey September 27, 2024 22:32

spacedmonkey reviewed Sep 27, 2024

View reviewed changes

register_collection: Use static variables to reduce calls to wp_norma…

899bd6f

…lize_path

	public function test_register_block_type_from_metadata_with_variations_php_file() {
	$filter_metadata_registration = static function ( $metadata ) {
	$metadata['variations'] = 'file:./variations.php';
	return $metadata;
	};

	add_filter( 'block_type_metadata', $filter_metadata_registration, 10, 2 );
	$result = register_block_type_from_metadata(
	DIR_TESTDATA . '/blocks/notice'
	);
	remove_filter( 'block_type_metadata', $filter_metadata_registration );

	static $wpinc_path_norm = '';
	if ( ! $wpinc_path_norm ) {
	$wpinc_path_norm = wp_normalize_path( realpath( ABSPATH . WPINC ) );
	}

Extend Block Metadata PHP Cache to Third-Party Blocks #7303

Are you sure you want to change the base?

Extend Block Metadata PHP Cache to Third-Party Blocks #7303

Conversation

mreishus commented Sep 6, 2024 • edited Loading

github-actions bot commented Sep 6, 2024 • edited Loading

github-actions bot commented Sep 6, 2024

Test using WordPress Playground

Some things to be aware of

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

gziolo commented Sep 13, 2024

mreishus commented Sep 13, 2024

gziolo commented Sep 16, 2024

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Sep 23, 2024 • 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

Choose a reason for hiding this comment

mreishus commented Sep 24, 2024 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented Sep 25, 2024

mreishus commented Sep 25, 2024

felixarntz commented Sep 25, 2024

felixarntz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreishus Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

felixarntz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

gziolo commented Sep 26, 2024

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented Sep 26, 2024 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

gziolo commented Sep 26, 2024 • 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

felixarntz commented Sep 27, 2024

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

mreishus commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

felixarntz Sep 23, 2024 •

edited

Loading

felixarntz Sep 23, 2024 •

edited

Loading

mreishus commented Sep 24, 2024 •

edited

Loading

felixarntz left a comment •

edited

Loading

mreishus Sep 26, 2024 •

edited

Loading

felixarntz Sep 26, 2024 •

edited

Loading

felixarntz commented Sep 26, 2024 •

edited

Loading

gziolo commented Sep 26, 2024 •

edited

Loading