Fix Interactivity API setup#8593
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
|
I see there are still e2e tests failing. No more "500 (Internal Server Error)", though. |
Aljullu
left a comment
There was a problem hiding this comment.
@DAreRodz what do you think about not including this PR in the changelog? If you agree, could you add the skip-changelog label?
Besides that, I left one question below which I think needs to be addressed.
I see there are still e2e tests failing. No more "500 (Internal Server Error)", though.
Right, they feel unrelated. We still have some tests in trunk that we need to fix.
| * @param bool $enabled True if the Interactivity API is manually enabled. | ||
| * @return bool True if _also_ the Gutenberg plugin is enabled. | ||
| */ | ||
| function woocommerce_blocks_is_gutenberg_enabled( $enabled ) { |
There was a problem hiding this comment.
Heads-up that WC core has a policy of L-2 support for WP, so we won't be able to remove this even when WP 6.2 is released.
I wonder whether it would be better to check the WP and GB versions directly instead of simply checking if GB is enabled. What do you think?
I also wonder if we could make this function generic so it's not coupled to the Interactivity API. That might be useful for other blocks. For example, we have at least one other block that checks if GB is enabled, so we could try to make this util function usable in that case as well? (That last comment is not a blocker, just me thinking out loud)
There was a problem hiding this comment.
Heads-up that WC core has a policy of L-2 support for WP, so we won't be able to remove this even when WP 6.2 is released.
Oh, thanks for the heads-up. I guess the L-2 support means that this plugin will have to support the two previously released versions of WP as well, so this is not a temporary fix, right? 👍
I wonder whether it would be better to check the WP and GB versions directly instead of simply checking if GB is enabled. What do you think?
Good idea, although not sure how to best implement that. I saw a PHP function called version_compare. Do you know if there is a more reliable way to compare versions in WordPress, or is it OK to use that?
There was a problem hiding this comment.
Oh, thanks for the heads-up. I guess the L-2 support means that this plugin will have to support the two previously released versions of WP as well, so this is not a temporary fix, right? 👍
Yes, exactly. WC currently supports WP 5.9, 6.0 and 6.1. So it won't be until WP 6.4 is released that we will be able to remove support for WP 6.1.
Sidenote: the truth is that "officially" WC Blocks has an L-0 support policy but WC core has an L-2 support policy. Given that WC Blocks ends up being merged into WC core, in practice we have to operate as if WC Blocks also had an L-2 support policy. I know it's a bit confusing. 😅
Good idea, although not sure how to best implement that. I saw a PHP function called
version_compare. Do you know if there is a more reliable way to compare versions in WordPress, or is it OK to use that?
We use version_compare in several places in the codebase, so I think it's fine to use it.
There was a problem hiding this comment.
The Gutenberg plugin kind of tries to remove the complexity of version checks etc by simply checking if the WP_HTML_Tag_Processor class exists, and by loading it from its own compatibility layer otherwise.
Is that something we can imitate here? Something like
function woocommerce_blocks_has_wp_html_tag_processor( $enabled ) {
if ( ! function_exists( 'is_plugin_active' ) ) {
include_once ABSPATH . 'wp-admin/includes/plugin.php';
}
return $enabled && class_exists( 'WP_HTML_Tag_Processor' );
}
add_filter(
'woocommerce_blocks_enable_interactivity_api',
'woocommerce_blocks_has_wp_html_tag_processor',
999
);We'd need to make sure that the woocommerce_blocks_enable_interactivity_api filter runs after the GB plugin is activated (if present), but that seems feasible 🤔
Seems kinda preferable to me over having a version check and manual imports from GB's compat layer...
There was a problem hiding this comment.
We'd need to make sure that the
woocommerce_blocks_enable_interactivity_apifilter runs after the GB plugin is activated (if present), but that seems feasible 🤔
@ockham I'm applying that filter in the plugins_loaded hook. I guess that runs after GB plugin is activated?
There was a problem hiding this comment.
I'm applying that filter in the
plugins_loadedhook. I guess that runs after GB plugin is activated?
From what I understand, yeah: Gutenberg's code should be loaded by the time plugins_loaded is triggered.
(To be sure, you can probably try with WP 6.1; disable the plugin and verify that the interactivity code isn't loaded, then re-enable the plugin and verify that the interactivity code is loaded.)
What
Fixes how the Interactivity API files are loaded, which now requires two conditions to be true:
woocommerce_blocks_enable_interactivity_apishould be explicitly set totrueWP_HTML_Tag_Processorclass, either included by WP core or the Gutenberg plugin, should be defined.Why
All e2e tests were recently failing due to a 500 response error from the server.
It was a bug introduced in #8447, that was causing the Interactivity API files to be unnecessarily loaded, and also requiring missing files from the Gutenberg plugin.
How
Only requiring
/src/Interactivity/woo-directives.phpwhen those two conditions are true.