Conditional loading `build_template_part_block_variations` for performance improvement

[thekt12]

Raised in ​Lazy initialization of block variants.

Profiling of if TT4 homepage showed register_block_core_template_part accounts for ~7% of load time ~80ms. The function calls
build_template_part_block_variations, which accounts for 99% of the function time. This function is called in almost every page. The root reason for this is deep down in the call stack, it calls WP_Theme_JSON_Resolver::get_merged_data, which calls WP_Theme_JSON::__construct for parsing theme JSON.

The core issue of improving JSON loading will be addressed in another ​Improve template loading and rendering activity.

However, it was found that by disabling build_template_part_block_variations, for pages that don’t require it, we found there is an improvement of ~80ms which aligns with the total time needed for register_block_core_template_part.
Disabling change that was made - [ ​https://github.com/WordPress/gutenberg/issues/45601#issuecomment-1516214955 WordPress/gutenberg#45601] (comment) tested on the default homepage of TT4
Before Change - ​https://blackfire.io/profiles/933e16a5-0932-41e3-8fb7-a21c10dc3811/graph
After Change- ​https://blackfire.io/profiles/c225c202-974c-4a9c-886f-ef731273461c/graph.

By removing the call to build_template_part_block_variations on pages that don't have variation could improve the performance.

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

[thekt12]

Before applying PR

[thekt12]

After PR changes

[thekt12]

register_block_core_template_part

register_block_core_template_part registers template parts from block.json and the custom template created from the editor side.
You can find the list of templates on the local instance via the following link ->
​http://localhost:8889/wp-admin/site-editor.php?path=%2Fwp_template_part%2Fall (change the base domain asper your env)

This function is mostly needed to register header and footer templates for the page and is in use by both frontend and backend.

The function makes a call to register_block_type_from_metadata which registers the settings( to brief it is a function that combines data obtained from JSON files with the settings provided during its call and registers it as a WP_Block_Type which is used at many places).

Specifically, In register_block_core_template_part we are passing on the instance and area variation(detailed in the next section) obtained from build_template_part_block_variations (which is an expensive function)

build_template_part_block_variations()

build_template_part_block_variations is a combination of two functions summarized below:
$instance_variations = build_template_part_block_instance_variations();

• It gets all the wp_template_parts (check the link above for template listing local)

$area_variations = build_template_part_block_area_variations( $instance_variations );

• It gets the area which has some variation
• Some improvements and bugs were found in this function ( noted towards last )

Where the registered block type variation is used (Improvement)?

Searching for places where variations in block type were used it was found only inside ->
​get_block_editor_server_block_settings() on ​line 2318 which is an editor-centric function and is not needed in the frontend.

This aligns with the fact that it will have no effect on the frontend if removed -
​build block variation only for editor

Opened a PR for the same - ​PR#5718
Other improvements found during research

• We must remove variables each time we hit one in the loop - ​line 183
• Also, the whole looping is to get the area which has some variation. I feel there should be a better way than looping.

Possible chance of bug

^{(I am not 100% sure here, just speaking as per what the code says)}
The scope attribute in the final outcome is highly dependent on the first variation that we encounter - ​line 189. However, the variations can be added from the editor and there maybe two variations in an area with different scope and this can lead to ambiguity as it takes the scope of first variation it encounters.

[thekt12]

@joemcgill one of the reasons to stick to the current PR instead of lazy loading version @spacedmonkey tried to implement​here is

• 'block_type_metadata_settings' filter from register_block_type_from_metadata() ​line 579 won't get variation information any more.

(But, I am not sure what the impact would be)

[joemcgill]

Thanks @thekt12! If I'm following all of this correctly (which your detailed writeup has been super helpful for), it seems to me that we can basically always assume that variations with the scope set to inserter are not needed on the front end, and since build_template_part_block_instance_variations() sets the scope to all of these variations to inserter, then they're basically only needed in the editor (or in REST API requests that are scope agnostic).

Ensuring variations are available to the block_type_metadata_settings filter makes sense to me, but they still won't be available with the other PR unless the user has edit_theme_options capabilities.

From what I understand, registering the core template block without variations when the user doesn't have the right capabilities might be a good workaround, but I'd love to get feedback from someone like @Mamaduka, who was involved in the original Gutenberg discussion about this, to be sure.

[Mamaduka]

I'm reposting here for visibility, as code comments and replies aren't shared on the trunk.

I still think a declarative approach for lazy-loading would be best suited here. There are other blocks that generate variations on the server where the capabilities approach might not be applicable.

@spacedmonkey, I know you're taking a well-deserved break from contributing, but if you could share your notes for #4857, maybe someone could pick it up again.

[spacedmonkey]

Basically in my ​PR, I use magic getter, to only load block variations, when the property on the class is needed. This property is only need in the admin context and not on the front end. Loading block variations in the context of template parts, results in a very expensive load WP_Query that results in many database calls.

I went with magic getter, this is bad pattern in PHP development, but it is the most backwards compatible way of making this change.

Thanks for the latest update @kt-12. Going to break out of the inline thread and respond to ​your last update here.

This approach is a bit better, because it allows variations to only be loaded in get_block_editor_server_block_settings(), but it still seems like a workaround and not a true API, since this will cause the registered block properties to be different based on whether that filter happens to be called, which also makes registered blocks mutable and thus non-cacheable (or at least more challenging). I do think that ensuring variations show up in the API responses would be a requirement, even if we did temporarily move variant registration for this block to only get_block_editor_server_block_settings() and the API, though.

What would you think about updating the block registration schema so that you could pass a callable as the variations property, which would be called only when variations are needed? @Mamaduka suggested something similar in the original Gutenberg thread.

Additionally, I think for consistency, we should apply whatever changes we're making for template part variations to other core blocks that register variations dynamically on the server, e.g. ​Navigation Link, and ​Post Terms.

BTW, this change does seem to have a measurable impact on server response time on the front end, in my local benchmarking, trunk is loading in 290.79 ms (p50) while this PR is 242.19 ms (p50), which is a sizable improvement.

@kt-12 Some unit tests going fails, The wp-includes/blocks/template-part.php needs to update in GB first then the changes needs to backport.

@joemcgill Implemented the same for Navigation-Link and Post-Term and then took a performance.

The following command was run thrice for both Trunk and PR and then Mean (Trunk) and Mean (PR) was obtained

npm run research -- benchmark-server-timing --url ​http://localhost:8889/ -n 500 -c 2

### Performance Improvement for TT4 (Tested locally on default homepage)

Metric	Mean (Trunk)	Mean (PR)	Diff
Response Time (median)	397.66 ms	362.16 ms	-35.5 ms
wp-load-alloptions-query (median)	3.23 ms	2.96 ms	-0.27 ms
wp-before-template (median)	173.53 ms	143.51 ms	-30.02 ms

@Mamaduka This PR is purely based on our findings-based interpretation that the dynamically registered variations were only used inside get_block_editor_server_block_settings and block-types rest API. We want someone core to Gutenberg to check and confirm if these changes aren't breaking anywhere else.

Furthermore, I have some improvements found on the editor side that could lead to similar performance upgrades on the editor side. However, I don't have the expertise to confirm my findings.

### Remove inline block schema and replace it with prefetch
We have used the following code at ​site-editor, ​widgets-form-blocks, ​edit-form, ​wp-customize-widgets
A 5-year-old code is mostly introduced to handle some edge cases related to third-party blocks and dynamic variations.

wp_add_inline_script(
	'wp-blocks',
	'wp.blocks.unstable__bootstrapServerSideBlockDefinitions(' . wp_json_encode( get_block_editor_server_block_settings() ) . ');'
);

The above code adds the preloads of the schema for all block types, in the default setup, I found it to add 92 block types.
I could confirm this by running wp.blocks.getBlockTypes(). However, if I comment on the above code (I tried on edit-form), reload the editor and then run wp.blocks.getBlockTypes() it still shows to load all the block types, just some of the blocks have dynamic information missing in this case. This means that information in inlining is being used but most of the data is just a repeat of what is already available to the editor.

Also, the information in the inline script is only used when a person searches for a block or some similar option and is not needed when the page loads (just my understanding based on what I discovered - this could defer). So we don't need all the information in the first load.

#### Proposal

1. Replace unstablebootstrapServerSideBlockDefinitions with a call to block type REST API ​#22812 The information in /wp/v2/block-types is similar to what we are inlining right now. However, it seems that we are not making use of this API endpoint anywhere in the core (not 100% sure). Instead of inlining, we can shift the load to preloading this way we free up the original doc from expensive computation

2. Only load what is missing instead of looping through all 92 blocks.

Most of the information is getting loaded in the editor even without inlining. So we could avoid looping through all the registered block types and inline it entirely. Instead, only inline those blocks that had some dynamic information. Note: This method would yield lesser improvement than the first.

@kt-12 Some unit tests going fails, The wp-includes/blocks/template-part.php needs to update in GB first then the changes needs to backport.

Thanks @mukeshpanchal27. Once I have confirmation on the code here I'll raise another PR for GB. At the moment I am confused as the which will go in GB and which will stay here, so focusing more on the problem to solve.

[gaambo]

@joemcgill Implemented the same for Navigation-Link and Post-Term and then took a performance.

In #59906 (based on ​https://github.com/WordPress/gutenberg/pull/54801, ​https://github.com/WordPress/gutenberg/pull/56100 and ​https://github.com/WordPress/gutenberg/pull/56080) I added variations for the post-term and navigation link block. Both were mentioned in comment:11

Should we create/register these variations conditionally (eg only on admin) as well? I see the current PR in this ticket still works with the code currently in core, which does not have the additional code from my GitHub PRs where additional variations are created on post-type/taxonomy registration.

[joemcgill]

Replying to gaambo:

In #59906 (based on ​https://github.com/WordPress/gutenberg/pull/54801, ​https://github.com/WordPress/gutenberg/pull/56100 and ​https://github.com/WordPress/gutenberg/pull/56080) I added variations for the post-term and navigation link block. Both were mentioned in comment:11

Should we create/register these variations conditionally (eg only on admin) as well? I see the current PR in this ticket still works with the code currently in core, which does not have the additional code from my GitHub PRs where additional variations are created on post-type/taxonomy registration.

Thanks for the heads up, @gaambo. I think we'll need to keep these types of changes in mind when implementing any changes to the way variations can be registered. The problem that you were solving for — adding additional variations after the block is initially registered — could also be solved by creating the variations when they are consumed, rather than than when the block is initially registered.

[spacedmonkey]

@thekt12 @joemcgill @Mamaduka Can someone explain why my PR has not been expanded apon. I didn't get an answer to my last reply. My PR considered backwards compablitiy where as other changes feel like backwards compat breaks. Can someone please explore my PR or explain to me why it does dropped?

[joemcgill]

Hi @spacedmonkey! Thanks for checking in here. Your ​last PR and initial explorations in ​the related Gutenberg ticket have been the foundations of the continuing iteration that @thekt12 has been doing in his PR, as I see it.

The main differences in the approaches so far are that we wanted to avoid relying on a filter to register callbacks for variations and experiment with allowing the $variations property to be a callable as well as an array of variation objects in order to get to the same result. I'm not confident that handling that kind of type juggling is going to work, since we can't be sure what third party code is expecting that property to only include an array of objects, so overloading that property with a magic getter or introducing a new method for hydrating variation data entirely might be necessary.

I assume changing the $variations property to allow it to either be an array of objects or a callable is the main back-compat concern you have? If that's not correct, could you elaborate?

Improve on ​https://github.com/WordPress/wordpress-develop/pull/5718, with improved B/C.

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

[spacedmonkey]

@joemcgill @Mamaduka @thekt12 I put together my own ​PR. This is based on the existing PR, but has more thinking about B/C. I feel like my PR is getting close. My PR uses magic getter and issets, to maintain B/C. It basically solf deprecated accessing the variations property directly and replaces it with get_variations method.

Please take a look in the new year and see what you think.

I have created my own PR ​https://github.com/WordPress/wordpress-develop/pull/5825 based on this one. It is very similar but has some improves.

1. This implementation prefers that the variation callback be run each time the property is needed. However, this means that there is potential of an expensive operation to be run multiple times during the same run. It's maybe unlikely that this will actually occur, but is something worth considering. We could rely on the magic getter to call the get_variations() callback only if the variations property isn't already set (which is what PHP will do naturally) rather than trying to prefer the callback each time you need to access variations.

I did consider this. Calling the callback once and then setting a flag saying it has be called, then "caching" the value in the varianets property. This is do-able. But I wonder if the callback might return dynamitic value, making it not cachable.

2. I'm curious if we can avoid loading variations in get_block_editor_server_block_settings entirely? This would speed up the initial load of the editor if we can hydrate those values from the REST API instead.

Also considered. Personally I think it is above my paid grade when it comes to JS changes, I would need someone from gutenberg team to help. I have had a ticket ​https://github.com/WordPress/gutenberg/issues/22812 since 2020, on this very topic. I wonder if @Mamaduka could support on this one.

Thanks for your feedback @joemcgill . I don't have enough time to get this one across the line myself. Please take this code example and fork / apply to your own PRs.

I'm happy to help out with the JS part of the code for ​https://github.com/WordPress/gutenberg/issues/22812, but I don't think it's a blocker for lazy loading variation improvements.

The implementation-wise ​`block_editor_rest_api_preload()` isn't much different from what we're already doing, so I'm wondering if there will be any performance benefits.

The above code preloads the schema for all block types. In the default setup, I found it to add 92 block types with it's variations.

I could confirm this by running wp.blocks.getBlockTypes() in the browser console for the editor. However, if I comment the above code (I tried on edit-form), reload the editor and then run wp.blocks.getBlockTypes() it still loads all the block types, except that some blocks have dynamic information missing in this case. This means that information in inlining is being used but most of the data is just a repeat of what is already available at the editor.

We could also remove the duplicated information on the client for core blocks, given that the server is the source of truth and it always takes precedence. We need to keep in mind that everything is filterable in WordPress core so every block type can get modified by the site before exposing the final shape to the client. That's why I would prefer to replace wp.blocks.unstable__bootstrapServerSideBlockDefinitions with the REST API endpoint call for block type and if necessary preload the response on the server. In reality, the won't be much of a difference for the user as the simply need all the information from the server to have the correct state in the block editor.

That said, the subset of changes for lazy loading block variations that are also discussed in ​https://github.com/WordPress/wordpress-develop/pull/5825 seems like a nice optimization. If we were to add a new setting like variations_callback then we will have to polyfill it in the Gutenberg plugin so it works with all supported versions of WordPress. There is an existing filter that should make it possible:

​https://github.com/WordPress/wordpress-develop/blob/e43275b61c6e3a8d5657b6d21eb99bf1bafd52b2/src/wp-includes/class-wp-block-type.php#L524

Update this PR for B/C mainly based on @spacedmonkey's new PR update.

Below are some changes that were made to the original PR and the reasoning behind it ->

1. In this PR, '$variations is set to private instead of protected as both yield different results (added a short snippet below to explain this). At the moment (trunk) final built variations are available to the child class, so we must respect that for B/C, hence private`.
2. In the current trunk code, variations are only built once during block registration. The same can be assumed for the updates that we make. So, I have updated the original $variations variable once the callback is made, this will prevent expensive callback from being called again and again if accessed multiple times in the same request.
3. I believe that even though $variations is set as private by nature it is not truly private, so we could avoid issuing a warning for anyone who tries to access them directly instead of via the get_variations() method.

<?php

class a {
    private $a  =  'b'; 
    
    public function __get( $name ) {
                if ( 'a' === $name ) {
                        return 'c';
                }
    }
}

class c extends a {
    public function test() {
        print_r( $this->a );
    }
}

$f = new c();
$f->test();  // this gives output as 'c', however, if we use `protected $a` you get the result as 'b'

cc: @joemcgill @spacedmonkey

[gaambo]

For ​block styles, ​patterns and ​pattern categories a registry is used. I know these are kind of different, because block styles can be registered outside from register_block_type, but that might be interesting for variations in the future as well. Also variations are kind of "bound" more to a single block type.

So maybe that approach - to use a singleton registry - could work for variations as well? That does not necessarily solve the performance aspect of this issue, but wanted to give it as an input for the architecture.

It's my understanding, that the original issue is a performance issue but we're getting into API design territory here.

@kt-12 @joemcgill We should add a backwards compablitiy shim in the gutenberg plugin. Something like this should work.

function gutenberg_register_block_type_args_shim( $args ) {
    if( isset( $args['variation_callback'] ) && is_callable( $args['variation_callback'] ) ) {
           $args['variations'] = call_user_func( $args['variation_callback'] );
           unset( $args['variation_callback'] );
    } 
    return $args;
}
add_filter( 'register_block_type_args', 'gutenberg_register_block_type_args_shim' );

Let's revert the changes to the block files. These changes along with a backwards compatiblity shim, should be made in gutenberg project. Can we also add a unit test for the get_variations method?

Otherwise, this is good to go.

Yes, I am working on the unit test and Gutenberg changes. I'll add an update shortly.

@joemcgill @spacedmonkey I had to replace __isset logic to always return true for variations to solve a BC issue - ​https://github.com/WordPress/wordpress-develop/actions/runs/7461317691/job/20301216384?pr=5718#step:14:340
If we think about it variations will never be returned as null due to the logic inside get_variations. Any call to isset inside class WP_Block_Type will never go through the magic function so it doesn't affect isset logic inside get_variations

To Note: With the current change, we introduce a BC issue (with a lower chance of re-surfacing); we can no longer set variations to null.

Yes, I am working on the unit test and Gutenberg changes. I'll add an update shortly.

Is this still being worked on?

Yes, I am working on the unit test and Gutenberg changes. I'll add an update shortly.

Is this still being worked on?

@spacedmonkey I did add a couple of test cases here in this PR and also updated GB PR ​https://github.com/WordPress/gutenberg/pull/56952

Excellent work, everyone!

[joemcgill]

In 57315:

Editor: Support deferred block variation initialization on the server.

When registering blocks on the server using register_block_type() or similar functions, a set of block type variations can also be registered. However, in some cases building this variation data during block registration can be an expensive process, which is not needed in most contexts.

To address this problem, this adds support to the WP_Block_Type object for a new property, variation_callback, which can be used to register a callback for building variation data only when the block variations data is needed. The WP_Block_Type::variations property has been changed to a private property that is now accessed through the magic __get() method. The magic getter makes use of a new public method, WP_Block_Type::get_variations which will build variations from a registered callback if variations have not already been built.

Props spacedmonkey, thekt12, Mamaduka, gaambo, gziolo, mukesh27, joemcgill.
Fixes #59969.

[joemcgill]

Adding a the needs-dev-note keyword for this release.

Merged in 57315

[ellatrix]

🚨 PHP unit tests are currently failing in the GB repository. It started happening around the time this PR was merged.

Indirect modification of overloaded property WP_Block_Type::$variations has no effect

Example run: ​https://github.com/WordPress/gutenberg/actions/runs/7590355231/job/20676711107?pr=58031

🚨 PHP unit tests are currently failing in the GB repository. It started happening around the time this PR was merged.

Indirect modification of overloaded property WP_Block_Type::$variations has no effect

Example run: ​https://github.com/WordPress/gutenberg/actions/runs/7590355231/job/20676711107?pr=58031

@ellatrix Thank you for flagging this. I have found the issue and have a fix for it. I'll push it once I test it out for other scenarios.

Thanks!

@ellatrix Thank you for flagging this. I have found the issue and have a fix for it. I'll push it once I test it out for other scenarios.

I have added a new ticket for the bug with a detailed description - https://core.trac.wordpress.org/ticket/60309#ticket

I have added an initial fix ​https://github.com/WordPress/wordpress-develop/pull/5915. However, I need to check how my changes affect some operations in __isset

[gaambo]

Replying to gaambo:

In #59906 (based on ​https://github.com/WordPress/gutenberg/pull/54801, ​https://github.com/WordPress/gutenberg/pull/56100 and ​https://github.com/WordPress/gutenberg/pull/56080) I added variations for the post-term and navigation link block. Both were mentioned in comment:11

Noting again that the code from my PRs in Gutenberg - once GB is merged into core - will probably fail as well, since I'm adding variations manually (via ->variations[]) as well.
I don't think using the variations_callback is viable in these cases, since when the callback is called, I still don't know if all taxonomies and post types are registered.
How should we handle this - in #59906 for merging the PHP changes into core, should I open a seperate issue in Gutenberg as a follow up - what's the best way here?

[spacedmonkey]

Thanks for the heads up @gaambo.

Can I recommend using the register_block_type_args filter if possible, instead of how you have added those variations in.

On the core merge, there I recommend using the get_block_type_variations filter.

[gaambo]

Replying to spacedmonkey:

Thanks for the heads up @gaambo.

Can I recommend using the register_block_type_args filter if possible, instead of how you have added those variations in.

On the core merge, there I recommend using the get_block_type_variations filter.

No, unfortunately using register_block_type_args is not possible. The whole point of the PRs was to add variations for custom post types and taxonomies that are registered _AFTER_ the block is registered. And since there is no server-side API for registering block variations I had to directly set the variations attribute.

I will have a look at the get_block_type_variations filter, that could work. But would be a rewrite of the existing (already merged into GB) PRs.
So should I open a new Gutenberg issue for this change, should this happen in the core merge? I'm kind of confused how to handle this 😅

[thekt12]

@gaambo variations_callback execution happens only when you try to access $block_type->variations (which should be way later than any other post-term, navigation queries), meaning using it should solve your problem.

I would highly recommend trying variation_callback first. If not then go for the get_block_type_variations filter.

[youknowriad]

Since we're in a time sensitive moment (package updates, merge with core...). Do you think it makes sense to revert the commit that introduce the test failures to avoid blocking Gutenberg work?

[thekt12]

@youknowriad I have added a fix ​https://github.com/WordPress/gutenberg/pull/58072, waiting for the tests to execute on PR.

[thekt12]

@youknowriad @ellatrix We might have to revert this for a while, It looks like we need some more time to get a fix.

[mukesh27]

The safer option is to revert it from the core for now, update the code accordingly, and add new unit tests to test that behaviour. Once confirmed, we can proceed with the merge.

[spacedmonkey]

@mukesh27 I disagree. I think this issue should be fixed in the gutenberg repo. Core is point of truth. Gutenberg integrates into that.

[flixos90]

I generally agree with @spacedmonkey that this is something that needs to be addressed in Gutenberg. The core change is reasonable and the only reason that there are test failures, as far as I can tell, is that the Gutenberg code needs to be updated to be compatible with the core change.

The situation is less than ideal, but the reality is that with the two codebases these problems will come up, and we can't expect WP core to not make changes like this. I do however acknowledge that we should do a better job in anticipating potential Gutenberg breakage from WP core changes and proactively communicate them between teams or even open a Gutenberg PR in advance, to avoid situations like the current one.

@youknowriad What would be the effort needed to update the Gutenberg blocks that make use of the (now outdated) way? If this is indeed blocking a lot of other crucial work right now ahead of the 6.5 Beta, I think it would be okay to revert. But if we revert, we should commit to addressing this in the near future on both sides.

[youknowriad]

I'm personally not familiar with this code so I can't speak about the time or the amount of work needed to address this on Gutenberg. I'm only speaking as editor release lead and trying to be mindful that most developers are working in the next few days getting their features on time for 6.5 and that it's not great to block them as it may have impact on deadlines...

[ellatrix]

Correct me if I'm wrong, but didn't this change a public API? What happened to back compat?

[joemcgill]

Replying to ellatrix:

Correct me if I'm wrong, but didn't this change a public API? What happened to back compat?

As I've looked into the failures during the first part of my day today, it does seem to me like the PHPUnit issues in the Gutenberg repo are a result of the way those tests are written and not with a true back-compat break caused by the changes here. I'm working on a fix for the GB tests now, but we may need to temporarily disable some tests in the Block_Navigation_Link_Variations_Test class to unblock others.

I do think there is a legitimate question about what kinds of modifications to variations should be supported for registered blocks once the variations data has already been accessed, but that seems like a separate issue to what this issue is trying to solve.

[joemcgill]

#60309 was marked as a duplicate.

[joemcgill]

The PHPUnit tests in the Gutenberg plugin for the Nav Link block has been fixed, but this did expose an issue where indirectly modifying the overloaded variations property no longer works. Copying the description from @thekt12 on #60309 here so we can keep discussion of this issue all in one place:

The setting of WP_Block_Type::$variations from the outside in a normal way like $block_type-> variations = array( array(...), ... ) works fine. However if we try to set it especially $block_type->variations[] = array( ... ); it throws Indirect modification of overloaded property WP_Block_Type::$variations has no effect.

The reason is WP_Block_Type::$variations is only accessible via the magic get, set function. get always returns a copy of the original attribute variable and not the reference of the actual attribute variable.

So, when we do $block_type->variations = array( array(...), ... ) we are only setting the temporary variable which is then processed by set to set the original attribute.

But this fails when we try to set the variable in a way where it's original state is checked like in ->$block_type->variations[] = array( ... );; Here, we try to add to the original variable array.

The way to fix this is by trying to get the reference of the original variable from get instead of copy of variable

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

This ticket fixes - ​https://github.com/WordPress/wordpress-develop/pull/5718#issuecomment-1901940214

[joemcgill]

After reviewing ​PR #5915, I'm leaning towards calling indirect modification of the variations property unsupported. Instead, it would be better to either use the get_block_type_variations filter to modify block variations at runtime or to replace the whole array.

As a convenience, it would make sense to implement a proper PHP API for registering/unregistering block variations (​see related Gutenberg issue), but doing so is out of scope for this ticket.

Marking this to close, pending additional feedback on the indirect modification question above.

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

[westonruter]

Replying to joemcgill:

The PHPUnit tests in the Gutenberg plugin for the Nav Link block has been fixed, but this did expose an issue where indirectly modifying the overloaded variations property no longer works. Copying the description from @thekt12 on #60309 here so we can keep discussion of this issue all in one place:

The setting of WP_Block_Type::$variations from the outside in a normal way like $block_type-> variations = array( array(...), ... ) works fine. However if we try to set it especially $block_type->variations[] = array( ... ); it throws Indirect modification of overloaded property WP_Block_Type::$variations has no effect.

The reason is WP_Block_Type::$variations is only accessible via the magic get, set function. get always returns a copy of the original attribute variable and not the reference of the actual attribute variable.

So, when we do $block_type->variations = array( array(...), ... ) we are only setting the temporary variable which is then processed by set to set the original attribute.

But this fails when we try to set the variable in a way where it's original state is checked like in ->$block_type->variations[] = array( ... );; Here, we try to add to the original variable array.

The way to fix this is by trying to get the reference of the original variable from get instead of copy of variable

I don't think this is necessarily the case. A magic getter can return by reference as I see from ​https://stackoverflow.com/a/13186679/93579

Test case: ​https://3v4l.org/TT2UD

Here's a patch to fix the back-compat issue: ​https://github.com/WordPress/wordpress-develop/pull/6030

Update: Hah, I see you drafted the same here: ​https://github.com/WordPress/wordpress-develop/pull/5915 and you discussed it in the previous comment.

[joemcgill]

Replying to westonruter:

I don't think this is necessarily the case. A magic getter can return by reference as I see from ​https://stackoverflow.com/a/13186679/93579

This is correct, and is something we had considered. Thanks for the feedback on the ticket. If we find an approach that avoids the direct access limitation without making this more complex and/or brittle, I'd be happy to reconsider, but don't see it as a blocker.

I'm also a bit unsure about the implications for other properties being accessed via this magic __get method, since I'm not seeing any other classes in WP that implement __get by reference.

[joemcgill]

Tried to capture some performance numbers for this change today, but due to the fact that the potential improvement is only measurable after the updates to the core blocks making use of this feature were synced in r57377, it's not easy to do a direct A/B comparison via benchmarks.

Using XHProf to do profiling locally, using the development environment included, I measured the cost of build_template_part_block_variations on the homepage of Twenty Twenty-four in the 6.4 branch compared with the same request in trunk.

	Calls	iWT	%
6.4	1	14,914 µs	5%
trunk	0	N/A	N/A

I'm going to leave this ticket open for the ongoing conversation about improvements happening here, but the primary concern of this ticket, avoiding the cost of build_template_part_block_variations on every pageload has been fixed.

[joemcgill]

Seeing no other reports of issues with this change, I'm going to go ahead and mark this as fixed. We can reopen or create follow-up issues if the lack of indirect modification support leads to bug reports. This should be addressed in the dev-note that @thekt12 is drafting as well.

[sabernhardt]

Dev note:
​https://make.wordpress.org/core/2024/02/29/performance-improvements-for-registering-block-variations-with-callbacks/