#59969 closed defect (bug) (fixed)
Conditional loading `build_template_part_block_variations` for performance improvement
Reported by: | thekt12 | Owned by: | joemcgill |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-dev-note |
Focuses: | performance | Cc: |
Description
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.
Attachments (2)
Change History (81)
This ticket was mentioned in PR #5718 on WordPress/wordpress-develop by @thekt12.
10 months ago
#1
- Keywords has-patch added; needs-patch removed
#2
@
10 months ago
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.
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
10 months ago
#4
@
10 months ago
@joemcgill one of the reasons to stick to the current PR instead of lazy loading version @spacedmonkey tried to implementhere 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)
#5
@
10 months ago
- Keywords 2nd-opinion needs-unit-tests added
- Milestone changed from Awaiting Review to 6.5
- Owner set to thekt12
- Status changed from new to assigned
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.
#6
@
10 months ago
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.
#7
@
10 months ago
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.
@joemcgill commented on PR #5718:
10 months ago
#8
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.
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
10 months ago
@mukesh27 commented on PR #5718:
10 months ago
#10
@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.
10 months ago
#11
@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
- 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
- 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.
10 months ago
#12
@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.
#13
follow-ups:
↓ 15
↓ 47
@
10 months ago
@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.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
10 months ago
#15
in reply to:
↑ 13
@
10 months ago
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.
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
10 months ago
#17
@
10 months ago
@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?
#18
@
10 months ago
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?
This ticket was mentioned in PR #5825 on WordPress/wordpress-develop by @spacedmonkey.
9 months ago
#19
Improve on https://github.com/WordPress/wordpress-develop/pull/5718, with improved B/C.
Trac ticket: https://core.trac.wordpress.org/ticket/59969
#20
@
9 months ago
@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.
@spacedmonkey commented on PR #5718:
9 months ago
#21
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.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
9 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
9 months ago
@spacedmonkey commented on PR #5825:
9 months ago
#24
- 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 thevariations
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.
- 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.
@Mamaduka commented on PR #5825:
9 months ago
#25
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.
9 months ago
#26
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:
9 months ago
#27
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 ->
- In this PR, '$variations
is set to
privateinstead of
protectedas 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`. - 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. - 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 theget_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
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
9 months ago
#29
@
9 months ago
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.
@spacedmonkey commented on PR #5718:
9 months ago
#30
@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' );
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
9 months ago
9 months ago
#32
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.
9 months ago
#33
@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
.
@spacedmonkey commented on PR #5718:
9 months ago
#34
Yes, I am working on the unit test and Gutenberg changes. I'll add an update shortly.
Is this still being worked on?
9 months ago
#35
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
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
9 months ago
@Mamaduka commented on PR #5718:
9 months ago
#37
Excellent work, everyone!
#38
@
9 months ago
- Keywords commit added; 2nd-opinion needs-unit-tests removed
- Owner changed from thekt12 to joemcgill
#40
@
9 months ago
- Keywords needs-dev-note added; commit removed
Adding a the needs-dev-note
keyword for this release.
@joemcgill commented on PR #5718:
9 months ago
#41
Merged in 57315
#42
@
9 months ago
🚨 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 commented on PR #5718:
9 months ago
#43
🚨 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
9 months ago
#44
@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.
@ellatrix commented on PR #5718:
9 months ago
#45
Thanks!
9 months ago
#46
@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
#47
in reply to:
↑ 13
@
9 months ago
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?
#48
follow-up:
↓ 49
@
9 months ago
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.
#49
in reply to:
↑ 48
@
9 months ago
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 😅
#50
@
9 months ago
@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.
#51
@
9 months ago
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?
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
9 months ago
#53
@
9 months ago
@youknowriad I have added a fix https://github.com/WordPress/gutenberg/pull/58072, waiting for the tests to execute on PR.
#54
@
9 months ago
@youknowriad @ellatrix We might have to revert this for a while, It looks like we need some more time to get a fix.
#55
@
9 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
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.
#56
@
9 months ago
@mukesh27 I disagree. I think this issue should be fixed in the gutenberg repo. Core is point of truth. Gutenberg integrates into that.
#57
@
9 months ago
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.
#58
@
9 months ago
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...
#59
follow-up:
↓ 60
@
9 months ago
Correct me if I'm wrong, but didn't this change a public API? What happened to back compat?
#60
in reply to:
↑ 59
@
9 months ago
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.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
9 months ago
#63
follow-up:
↓ 68
@
9 months ago
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
This ticket was mentioned in PR #5915 on WordPress/wordpress-develop by @thekt12.
9 months ago
#64
- Keywords has-unit-tests added
Trac ticket: https://core.trac.wordpress.org/ticket/59969
This ticket fixes - https://github.com/WordPress/wordpress-develop/pull/5718#issuecomment-1901940214
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
9 months ago
#66
@
9 months ago
- Keywords close added; has-unit-tests removed
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.
This ticket was mentioned in PR #6030 on WordPress/wordpress-develop by @westonruter.
8 months ago
#67
Trac ticket: https://core.trac.wordpress.org/ticket/59969
#68
in reply to:
↑ 63
;
follow-up:
↓ 69
@
8 months ago
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.
#69
in reply to:
↑ 68
@
8 months ago
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.
#70
@
8 months ago
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.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
8 months ago
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
8 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
8 months ago
#74
@
8 months ago
- Resolution set to fixed
- Status changed from reopened to closed
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.
Trac ticket: https://core.trac.wordpress.org/ticket/59969