Backport script loader: enqueue stored block supports styles (refresh) #3259

ockham · 2022-09-15T12:18:26Z

🎼 So Fresh, So Clean 🎶

This is a fork of @ramonjd's #3218, to rebase it (now that #3199 has been merged), and to address feedback by @gziolo.

Original PR desc:

Backporting changes to script-loader that enqueue block support styles stored by the style engine/

See tracking issue: WordPress/gutenberg#43440

Backporting the following changes

Style engine: enqueue block supports styles in Gutenberg gutenberg#42880

(WordPress/gutenberg#4288 has testing instructions.)

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

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.

Backporting block supports filter callback and registrations

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php

ockham · 2022-09-15T12:33:24Z

Unit tests are failing 😕

1) Tests_Theme_wpGetGlobalStylesheet::test_should_enqueue_stored_styles
Registered styles with handle of "core-block-supports" do not match expected value from Style Engine store.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '.saruman{color:white;height:100px;border-style:solid;}'
+    0 => '/**\n
+     * Core styles: block-supports\n
+     */\n
+    .saruman {\n
+    	color: white;\n
+    	height: 100px;\n
+    	border-style: solid;\n
+    }\n
+    '

Edit: Looks like comments and whitespace aren't removed as expected 🤔

ockham · 2022-09-15T18:02:04Z

A similar issue seems to be present over at #3237 (comment) and #3204 (comment).

andrewserong · 2022-09-16T01:45:53Z

A similar issue seems to be present over at #3237 (comment) and #3204 (comment).

I believe the test failure with this one is slightly different to those other two PRs. For the other two PRs, the inline styles tests likely need to be updated to either expect no space between the colon and the value (since the style engine intentionally switched inline style output to remove the space), or for the tests to be more permissive of spaces (i.e. remove spaces between property + colon and value before doing the comparison).

For this one, I think the issue is that (inferring from the test failures) the tests in core appear to be run with SCRIPT_DEBUG set, which switches on prettify for the stored styles output on the followings lines: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/style-engine/class-wp-style-engine-processor.php#L104, and https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/script-loader.php#L2990

As far as I can tell, for these tests, the SCRIPT_DEBUG value is determined here: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/default-constants.php#L108 (if I switch that line to $develop_src = false; then the tests pass for me locally).

So, what's the best fix for these particular tests? Given that SCRIPT_DEBUG appears to be set in default-constants.php based on whether or not the current WP version has -src, I was wondering if we should make the tests factor in whether SCRIPT_DEBUG is set to determine which value to expect.

I've opened up (yet another 😅) fork of this changeset in #3262 to try that out. But feel free to close that / copy + paste anything from there if it's helpful.

ramonjd · 2022-09-18T22:50:49Z

So, what's the best fix for these particular tests? Given that SCRIPT_DEBUG appears to be set in default-constants.php based on whether or not the current WP version has -src, I was wondering if we should make the tests factor in whether SCRIPT_DEBUG is set to determine which value to expect.

Thanks for looking into this folks. Sorry I was out of action last week.

Yeah, while I was running these tests locally SCRIPT_DEBUG was activated and I didn't get time to go deeper.

There was a recent change to wp_style_engine_get_stylesheet_from_context however that adds an options arg, through which we can set prettify => true/false. Maybe we could update the function signature of wp_enqueue_stored_styles() to be able to pass that down?

Would that be kosher?

I've opened up (yet another 😅) fork of this changeset in #3262 to try that out. But feel free to close that / copy + paste anything from there if it's helpful.

Thank you @andrewserong !!!

andrewserong · 2022-09-18T23:56:34Z

Maybe we could update the function signature of wp_enqueue_stored_styles() to be able to pass that down?
Would that be kosher?

Passing options to that function sounds like a good idea to me, given that we're supporting options args for most of the public style engine methods, and it'd make the a bit more explicit (e.g. we could test both prettified and non-prettified if we wanted to) 👍

ramonjd · 2022-09-19T00:11:38Z

Passing options to that function sounds like a good idea to me, given that we're supporting options args for most of the public style engine methods, and it'd make the a bit more explicit (e.g. we could test both prettified and non-prettified if we wanted to)

Okey dokey! I'll update this PR, but let's keep #3262 just in case there are doubts. 🙇

ramonjd · 2022-09-19T02:36:16Z

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php

+
+		wp_enqueue_stored_styles();
+
+		$this->assertEquals(


This is a hangover from my original PR.

I think it's better to use assertSame here for string comparisons and assertEquals for objects etc

Suggested change

$this->assertEquals(

$this->assertSame(

ramonjd · 2022-09-19T02:36:23Z

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php

+			'Registered styles with handle of "core-block-supports" do not match expected value from Style Engine store.'
+		);
+
+		$this->assertEquals(


As above.

Suggested change

$this->assertEquals(

$this->assertSame(

ramonjd · 2022-09-19T02:37:36Z

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php

+			),
+		);
+
+		// Enqueue some other styles.


I forgot to change this verb tense from imperative to third person present

Suggested change

// Enqueue some other styles.

// Enqueues some other styles.

ramonjd · 2022-09-19T02:46:21Z

There was a recent change to wp_style_engine_get_stylesheet_from_context however that adds an options arg, through which we can set prettify => true/false. Maybe we could update the function signature of wp_enqueue_stored_styles() to be able to pass that down?

@ockham I've added a Gutenberg PR with the proposed changes mentioned in the comment above.

WordPress/gutenberg#44248

Happy to refork this PR with the changes if you don't have the bandwidth to port them across to this PR.

ramonjd · 2022-09-19T05:19:35Z

Happy to refork this PR with the changes if you don't have the bandwidth to port them across to this PR.

Update PR here: #3273

If you're happy with that, we can close this PR

ockham · 2022-09-19T08:38:31Z

Thank you @ramonjd! Closing in favor of #3273.

…sts to 6.1. This changeset backports the following changes: - Implement [WordPress/gutenberg#42880 gutenberg#42880]: Backport script loader: enqueue stored block supports styles - Allow a way to bypass `SCRIPT_DEBUG` in tests. See [WordPress/wordpress-develop#3259 (comment) comment] and the related [WordPress/gutenberg#44248 Gutenberg pull request] Props ramonopoly, gziolo, bernhard-reiter, audrasjb, costdev. See #56467. Built from https://develop.svn.wordpress.org/trunk@54214 git-svn-id: http://core.svn.wordpress.org/trunk@53773 1a063a9b-81f0-0310-95a4-ce76da25c4cd

…sts to 6.1. This changeset backports the following changes: - Implement [WordPress/gutenberg#42880 gutenberg#42880]: Backport script loader: enqueue stored block supports styles - Allow a way to bypass `SCRIPT_DEBUG` in tests. See [WordPress/wordpress-develop#3259 (comment) comment] and the related [WordPress/gutenberg#44248 Gutenberg pull request] Props ramonopoly, gziolo, bernhard-reiter, audrasjb, costdev. See #56467. Built from https://develop.svn.wordpress.org/trunk@54214 git-svn-id: https://core.svn.wordpress.org/trunk@53773 1a063a9b-81f0-0310-95a4-ce76da25c4cd

…sts to 6.1. This changeset backports the following changes: - Implement [WordPress/gutenberg#42880 gutenberg#42880]: Backport script loader: enqueue stored block supports styles - Allow a way to bypass `SCRIPT_DEBUG` in tests. See [WordPress#3259 (comment) comment] and the related [WordPress/gutenberg#44248 Gutenberg pull request] Props ramonopoly, gziolo, bernhard-reiter, audrasjb, costdev. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54214 602fd350-edb4-49c9-b593-d223f7449a82

ockham mentioned this pull request Sep 15, 2022

Backport script loader: enqueue stored block supports styles #3218

Closed

ramonjd and others added 5 commits September 15, 2022 14:25

Initial commit.

db90f69

Backporting block supports filter callback and registrations

Added test

8d10a6e

Remove unused global from test

311df00

Formatting

3c3a710

Move test

23a38f8

ockham force-pushed the add/enqueue-stored-block-supports-styles branch from 3f828d5 to 23a38f8 Compare September 15, 2022 12:25

gziolo reviewed Sep 15, 2022

View reviewed changes

tests/phpunit/tests/theme/wpGetGlobalStylesheet.php Outdated Show resolved Hide resolved

Remove obsolete PHPDoc

ca40bbb

This was referenced Sep 15, 2022

Plugin: Backport PHP changes for WordPress 6.1 release WordPress/gutenberg#43440

Closed

Backport Layout block support refactor part 2 #3254

Closed

andrewserong mentioned this pull request Sep 16, 2022

(Alternative) Backport script loader: enqueue stored block supports styles (3rd attempt) #3262

Closed

tellthemachines mentioned this pull request Sep 16, 2022

Sync Gutenberg packages #3154

Closed

ramonjd reviewed Sep 19, 2022

View reviewed changes

ramonjd mentioned this pull request Sep 19, 2022

Block supports: allow overriding prettify options for enqueued CSS WordPress/gutenberg#44248

Merged

This was referenced Sep 19, 2022

Add/enqueue stored block supports styles ramonjd/wordpress-develop#1

Closed

Backporting block supports filter callback and registrations and tests #3273

Closed

ockham closed this Sep 19, 2022

ockham deleted the add/enqueue-stored-block-supports-styles branch September 19, 2022 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport script loader: enqueue stored block supports styles (refresh) #3259

Backport script loader: enqueue stored block supports styles (refresh) #3259

ockham commented Sep 15, 2022

ockham commented Sep 15, 2022 •

edited

Loading

ockham commented Sep 15, 2022

andrewserong commented Sep 16, 2022

ramonjd commented Sep 18, 2022 •

edited

Loading

andrewserong commented Sep 18, 2022

ramonjd commented Sep 19, 2022

ramonjd Sep 19, 2022

ramonjd Sep 19, 2022

ramonjd Sep 19, 2022

ramonjd commented Sep 19, 2022

ramonjd commented Sep 19, 2022

ockham commented Sep 19, 2022

Backport script loader: enqueue stored block supports styles (refresh) #3259

Backport script loader: enqueue stored block supports styles (refresh) #3259

Conversation

ockham commented Sep 15, 2022

Backporting the following changes

ockham commented Sep 15, 2022 • edited Loading

ockham commented Sep 15, 2022

andrewserong commented Sep 16, 2022

ramonjd commented Sep 18, 2022 • edited Loading

andrewserong commented Sep 18, 2022

ramonjd commented Sep 19, 2022

ramonjd Sep 19, 2022

Choose a reason for hiding this comment

ramonjd Sep 19, 2022

Choose a reason for hiding this comment

ramonjd Sep 19, 2022

Choose a reason for hiding this comment

ramonjd commented Sep 19, 2022

ramonjd commented Sep 19, 2022

ockham commented Sep 19, 2022

ockham commented Sep 15, 2022 •

edited

Loading

ramonjd commented Sep 18, 2022 •

edited

Loading