Editor: Move Interactivity directives processing to WP_Block class #6331

gziolo · 2024-03-29T10:27:24Z

Trac ticket: https://core.trac.wordpress.org/ticket/61185#ticket

Testing performance with TT4 and homepage:

Before (trunk): wp-total (median) 55.59 ms
After (branch): wp-total (median) 55.18 ms

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-03-29T10:27:41Z

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 gziolo, cbravobernal.

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

github-actions · 2024-03-29T10:44:00Z

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.

cbravobernal · 2024-05-13T16:20:55Z

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

+				! empty( $this->block_type->supports['interactivity']['interactive'] )
+			)
+		) {
+			$root_interactive_block = $this;


Would be more efficient to add an extra attribute to parsed_block than to compare the entire block?

I remember that the problem comparing $parsed_block attribute in layout has been fixed.

But this change would require some testing, so better leave for a follow-up PR.

Objects are stored and compared by reference, right? How would be any alternative more efficient in that case?

Checking a object property is more performant than checking the entire object, right?

Can you share a code example to illustrate the idea?

@cbravobernal, I think you're confusing arrays with objects here in PHP.

The problem lies in arrays, as PHP doesn't compare them by reference, and instead compares the entire array. In this case, checking a property of the array is more performant than checking the entire array.

However, in the case of objects, they are instances of a class and are compared by reference. So there's no performance problem.

Is that correct, @gziolo?

@cbravobernal, I think you're confusing arrays with objects here in PHP.

Yep, as we were comparing arrays back then in $parsed_block if I remember correctly.

Yes, but that's no longer the case, right? So we no longer have that problem.

cbravobernal

Great work Greg.

Tested and works perfectly. It also is cleaner and easier to follow than the other approach.

We are also saving tons of filters on the process.

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

cbravobernal · 2024-05-28T16:15:45Z

Added a filter to be able to enable/disable server directives processing and an unit test for it.

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

tests/phpunit/tests/interactivity-api/wpInteractivityAPIFunctions.php

…ons.php

tests/phpunit/tests/interactivity-api/wpInteractivityAPIFunctions.php

…ons.php

tests/phpunit/tests/interactivity-api/wpInteractivityAPIFunctions.php

…ons.php

tests/phpunit/tests/interactivity-api/wpInteractivityAPIFunctions.php

…ons.php

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

Neccessary for the planned changes in WordPress core: WordPress/wordpress-develop#6331.

gziolo · 2024-05-29T11:35:11Z

I documented the new filter. I renamed it to interactivity_process_directives as wp_ prefix doesn't add too much value here. It's used sometimes with functions to ensure more general names don't conflict.

I also opened WordPress/gutenberg#62095 to have the fix in place to Gutenberg at the time of the commit so E2E tests continue to pass.

gziolo · 2024-05-29T11:56:17Z

Committed with https://core.trac.wordpress.org/changeset/58234.

Neccessary for the planned changes in WordPress core: WordPress/wordpress-develop#6331. Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>

luisherranz · 2024-06-06T11:41:30Z

I documented the new filter. I renamed it to interactivity_process_directives as wp_ prefix doesn't add too much value here. It's used sometimes with functions to ensure more general names don't conflict.

Since this filter is used to control whether the blocks are processed or not, wouldn't it be better to include the word "blocks" in the name of the filter? Otherwise, people can think it is related to the wp_interactivity_process_directives( $html ) function, that works for arbitrary HTML.

Like interactivity_process_directives_in_blocks or something similar...

gziolo · 2024-06-06T12:51:58Z

It also could be moved to the body of wp_interactivity_process_directives to cover all use cases with a single filter. This way we also won't need to change anything in the Gutenberg plugin.

luisherranz · 2024-06-06T14:25:03Z

What is the main use of this filter? Is it so that plugins can disable the Server Directive Processing for some reason, or is it so that Gutenberg can replace it with a more updated version?

cbravobernal · 2024-06-06T15:12:58Z

We have e2e tests that disable the Server Directive Processing. That's the only one that come to my mind right now. Gutenberg crashed and we had to choose between 2 options:

Remove those tests, or update them to take into account SSR.
Add a filter to allow developers to choose.

luisherranz · 2024-06-07T07:23:57Z

Ok, thanks Carlos 🙂

I don't have a strong opinion on whether we should rename the filter or also include the processing of the wp_interactivity_process_directives function, but I would do one of those.

cbravobernal · 2024-06-07T11:36:49Z

Ok, thanks Carlos 🙂

I don't have a strong opinion on whether we should rename the filter or also include the processing of the wp_interactivity_process_directives function, but I would do one of those.

I think disabling all SSR would make more sense than only in blocks, right?

gziolo added 2 commits May 10, 2024 10:42

Editor: Move Interactivity directives processing to WP_Block class

047171a

Use Yoda Condition checks, you must!

ff14587

gziolo force-pushed the update/wp-block-process-directives branch from 867ef75 to ff14587 Compare May 10, 2024 08:42

cbravobernal reviewed May 13, 2024

View reviewed changes

cbravobernal approved these changes May 13, 2024

View reviewed changes

cbravobernal reviewed May 13, 2024

View reviewed changes

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

cbravobernal added 2 commits May 28, 2024 17:47

Add filter to enable/disable server directives processing

0be6e0b

Rename filter

b9c6d52

cbravobernal mentioned this pull request May 28, 2024

Interactivity API: Iteration for WP 6.6 WordPress/gutenberg#60219

Open