Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Script loader: remove 6.1 wp actions #44519

Merged
merged 3 commits into from
Oct 3, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Sep 28, 2022

What?

Now that 6.1-beta is running in development, we should remove the backported hook callbacks for wp_enqueue_stored_styles.

Why?

This prevents the callback running twice.

How?

remove_action YO!

Testing Instructions

In trunk, you can test the before state by creating a post with some layout blocks.

Here, kind reviewer, is some test HTML

Example
<!-- wp:group {"style":{"spacing":{"blockGap":"var:preset|spacing|50"}},"layout":{"type":"constrained","justifyContent":"left","contentSize":"577px"}} -->
<div class="wp-block-group"><!-- wp:site-title {"style":{"color":{"background":"#b92c2c"},"typography":{"lineHeight":5.7,"letterSpacing":"41px"}},"fontSize":"large"} /-->

<!-- wp:site-title {"style":{"color":{"background":"#88c7b4"},"typography":{"lineHeight":5.7,"letterSpacing":"41px"}},"fontSize":"large"} /--></div>
<!-- /wp:group -->

In the published post's frontend HTML source, you'll see that the block supports inline CSS has two comments Core styles: block-supports:

<style id='core-block-supports-inline-css'>
/**
 * Core styles: block-supports
 */

/**
 * Core styles: block-supports
 */

This means that the callback has been called twice.

Apply this PR and see that the duplicate comment is GONE BABY

@ramonjd ramonjd self-assigned this Sep 28, 2022
@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended CSS Styling Related to editor and front end styles, CSS-specific issues. labels Sep 28, 2022
@tellthemachines
Copy link
Contributor

Don't we still need these somewhere in order for the plugin to be compatible with previous versions of WP?

@ramonjd
Copy link
Member Author

ramonjd commented Sep 28, 2022

Don't we still need these somewhere in order for the plugin to be compatible with previous versions of WP?

Thanks for taking a look @tellthemachines!

Sorry, I'm not sure I follow 🤔

Do you mean the wp hooks removed by remove_action?

wp_enqueue_stored_styles will be introduced with 6.1 so no previous WP version will have it.

Also, I was going on the assumption that the plugin will always want to use gutenberg_enqueue_stored_styles to allow continuous development (if any). That's also the case with wp_enqueue_global_styles etc

Am I understanding things right?

@tellthemachines
Copy link
Contributor

Do you mean the wp hooks removed by remove_action?

wp_enqueue_stored_styles will be introduced with 6.1 so no previous WP version will have it.

Yes, sorry, I think my yesterday evening brain was processing it the other way around 😂

So the assumption is every WP version > 6.0 will have wp_enqueue_stored_styles, and we'll still want to use gutenberg_enqueue_stored_styles in the plugin for the foreseeable future, right?

If that's the case, we should move this to somewhere outside the compat/wordpress-6.1/ folder because that will be removed once the plugin no longer supports 6.1 (we only support the latest 2 versions of WP), lib/load.php might be a good place?

@ramonjd
Copy link
Member Author

ramonjd commented Sep 29, 2022

If that's the case, we should move this to somewhere outside the compat/wordpress-6.1/ folder because that will be removed once the plugin no longer supports 6.1 (we only support the latest 2 versions of WP

OMG, the folder name totally went over my head. Thanks for pointing that out. I'm still living in the past 🤣

I'll update

…kported hook callbacks for wp_enqueue_stored_styles.

This prevents the callback running twice.
@ramonjd
Copy link
Member Author

ramonjd commented Sep 30, 2022

The following is just a brain dump in case we need to come back to it later, and it's not necessarily coherent :)

TL;DR We don't need this PR as it is right now, but there might be case to remove the WP hooks later.


So the assumption is every WP version > 6.0 will have wp_enqueue_stored_styles, and we'll still want to use gutenberg_enqueue_stored_styles in the plugin for the foreseeable future, right?

Yes. In fact, because, like gutenberg_enqueue_global_styles, gutenberg_enqueue_stored_styles calls internal gutenberg_ function.

Furthermore, it's envisaged that Gutenberg will only use the style engine's gutenberg_* functions and _Gutenberg classes to aid continuous development of the Style Engine for the foreseeable future.

For now however, I don't think there's much of an immediate problem because we really only care about block support styles in Gutenberg.

Gutenberg, when activated, re-reregisters blocks supports (overwriting Core's).

Styles for Gutenberg's blocks supports are currently being generated using the style engine's gutenberg_* functions and _Gutenberg classes.

Core block supports, therefore, are not registering any styles at all with the plugin activated.

The worst thing that is happening is that the Core styles: block-supports comment is being printed twice, and ONLY in development mode.

Catch 22

In fact I think we can't call remove_action on gutenberg_enqueue_stored_styles hooks at all because in it we've included functionality to enqueue any styles registered by third parties. If we removed the WP hooks, we would short-circuit any styles registered via the Core WP Style Engine, by, for example, a theme. 😄

The Catch 22 will be when we start enqueuing global styles, and there's the possibility of styles being printed twice or Core and Gutenberg styles clashing. To be safe, I think we have to remove the action hooks. 🙃

So in that respect we've painted ourselves into nice little corner.

Going forward

My proposal in this PR therefore is to move gutenberg_enqueue_stored_styles into client-assets.php, which is not a versioned compat file, to indicate that it's a permanent fixture of Gutenberg for now.

Then, continue to remove the wp hooks in that file:

remove_action( 'wp_enqueue_scripts', 'wp_enqueue_stored_styles' );
remove_action( 'wp_footer', 'wp_enqueue_stored_styles', 1 );

And, to ensure that we support printed out 3rd party styles registered with the Style Engine, we check for Core stores

	// If there are any other stores registered by themes etc., print them out.
	$additional_stores = WP_Style_Engine_CSS_Rules_Store_Gutenberg::get_stores();

	/*
	 * Since the corresponding action hook in Core is removed below,
	 * this function should still honour any styles stored using the Core Style Engine store.
	 */
	if ( class_exists( 'WP_Style_Engine_CSS_Rules_Store' ) ) {
		$additional_stores = array_merge( $additional_stores, WP_Style_Engine_CSS_Rules_Store_Gutenberg::get_stores() );
	}

Sorry if this sounds like rubbish. I've been thinking about it for too long.

cc @andrewserong for the obligatory sanity check 😄

I'm wondering if gutenberg_enqueue_global_styles will face a similar compat fork in the road too, or whether it'll continue to be copied into compat/wordpress-6.2/script-loader.php, then compat/wordpress-6.3/script-loader.php etc

…t should not be versioned

It is envisaged that Gutenberg will continue to use the Style Engine's `gutenberg_*` functions and `_Gutenberg` classes to aid continuou.s development.
@andrewserong
Copy link
Contributor

My proposal in this PR therefore is to move gutenberg_enqueue_stored_styles into client-assets.php, which is not a versioned compat file, to indicate that it's a permanent fixture of Gutenberg for now.

Thanks for laying all that out @ramonjd — the logic of doing that and then removing the core hooks sounds good to me!

@ramonjd ramonjd force-pushed the update/script-loader-remove-6-1-wp-filters branch from 880903f to 019e1ab Compare September 30, 2022 05:59
@ramonjd ramonjd added Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Style Engine /packages/style-engine and removed [Type] Bug An existing feature does not function as intended labels Sep 30, 2022
lib/client-assets.php Outdated Show resolved Hide resolved
Co-authored-by: Ari Stathopoulos <[email protected]>
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up @ramonjd! This is testing well for me:

✅ Still outputs styles correctly in 6.0 + Gutenberg plugin
✅ Confirmed that prior to this PR, 6.1 beta + Gutenberg plugin resulted in double "Core styles" string
✅ With this PR applied, there is only a single output 👍

Before After
image image

LGTM! ✨

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@ramonjd ramonjd merged commit e5e9986 into trunk Oct 3, 2022
@ramonjd ramonjd deleted the update/script-loader-remove-6-1-wp-filters branch October 3, 2022 21:40
@github-actions github-actions bot added this to the Gutenberg 14.3 milestone Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability CSS Styling Related to editor and front end styles, CSS-specific issues. [Package] Style Engine /packages/style-engine
Projects
Status: 🏆 Done
Development

Successfully merging this pull request may close these issues.

4 participants