Style engine: migrate functions, classes and tests #3199

ramonjd · 2022-09-06T04:48:57Z

This PR migrates the Style Engine PHP functions, classes and tests into Core for 6.1.

It reflects the latest changes from WordPress/gutenberg#43840

See tracking issue: WordPress/gutenberg#43440

Backport PRs blocked by this changeset (that I know of)

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

costdev

As this review is becoming quite large and the comments are quite similar, I'll submit it now.

The feedback on the tests applies to all other test files, as do documentation comments in source files.

A couple of general things:

Test class names should have the format Tests_Folder_Name_ThingInTitleCase.
Ensure all test methods have the @ticket annotation.
Ensure all test methods have the @covers (or @coversNothing if it doesn't cover something) annotations, as this will allow us all to see where coverage may be great/need improvement.
As shown in the review, @coversDefaultClass can be used in the class docblock for a file that tests a single class, which allows for shorter covers annotations using simply the method name like ::method_name.
Where possible, try to start test methods with test_should_ / test_should_not. Using test_method_name or test_function_name is typically only useful for small source functions, or for test methods that use a data provider.

tests/phpunit/tests/style-engine/wpStyleEngineCssDeclarations.php

src/wp-includes/style-engine/class-wp-style-engine-processor.php

src/wp-includes/style-engine.php

tests/phpunit/tests/style-engine/styleEngine.php

ramonjd · 2022-09-08T04:54:49Z

Thanks @costdev for volunteering and taking the time to review the code, and also for explaining the required changes 🙇

I think I've covered just about everything, but will take another sweep over later to spot if I've missed something.

The final failing test should pass once https://core.trac.wordpress.org/ticket/55966 has been committed.

costdev

Thanks for the updates @ramonjd! I've reviewed the other files as well in this one.

src/wp-includes/style-engine.php

costdev · 2022-09-09T04:04:54Z

src/wp-includes/style-engine/class-wp-style-engine-css-declarations.php

+ * @access private
+ * @since 6.1.0
+ */
+class WP_Style_Engine_CSS_Declarations {


While this is intended to be private, it's open not just for instantiation, but extension.

Some thoughts:

Should this be considered private? Why?

If so, should this be a final class to at least block extension, or do we extend/plan to extend this class?

What else could be done? (I appreciate that something like an allowlist using an optimized debug_backtrace for the caller of the constructor is essentially the only way to "properly" lock it down though, so not suggesting that).

Same applies to all instances in the PR.

@peterwilsoncc do you have any thoughts here?

or do we extend/plan to extend this class

I'm not sure of the strategy right now.

I expect that there is no plan to extend. Since this comes from a Gutenberg package, we expect to version it.

Backwards compat will foil that I guess. 🤷

Happy to take any advice onboard!

@azaozz Do you have any thoughts on this?

@costdev thanks for the ping.

If this is intended to be private, "core use only", it will perhaps have to be locked in a closure like the current Webfonts code is. That will ensure it can be updated in the future without concerns about backwards compatibility.

If not locked, back-compat will have to be maintained, even if the class is final.

Thanks @azaozz! If it's not locked and back-compat has to be maintained, I don't think there would be any benefit in having @access private. Would you agree?

it will perhaps have to be locked in a closure like the current Webfonts code is.

Thanks for the advice here.

I think the class in question (WP_Style_Engine_CSS_Declarations), and the following, other, new classes:

WP_Style_Engine_CSS_Rule

WP_Style_Engine_CSS_Rules_Store

WP_Style_Engine_Processor

should be fairly stable. Maintaining backwards compatibility will be expected and, I hope, not overly tortuous.

I'll remove @access private from these.

WP_Style_Engine, on the other hand, is the primary integration point and provides the bulk of the functionality.

I expect it will have to be updated more frequently, especially given that this is a new API.

In that case, do folks think it's safer to refactor WP_Style_Engine to something similar to what's happening in webfonts? https://github.com/ramonjd/wordpress-develop/blob/bb0cbe3f557d16714ca109a28c2877d178adc94f/src/wp-includes/script-loader.php#L3059-L3058

cc @aristath and @andrewserong for thoughts

WP_Style_Engine, on the other hand, is the primary integration point and provides the bulk of the functionality.

I think even this one is pretty stable, and good for us to commit to maintaining for backwards compatibility. The protected methods can be added to or rearranged safely in future versions, but the public methods appear to be ones that have been pretty stable in Gutenberg lately, and I think would be good to commit to in the long-term. I'd lean toward keeping this PR as-is.

src/wp-includes/style-engine/class-wp-style-engine-css-declarations.php

tests/phpunit/tests/style-engine/styleEngine.php

tests/phpunit/tests/style-engine/wpStyleEngineCssRulesStore.php

mukeshpanchal27

Left some nit pick reviews.

src/wp-includes/style-engine/class-wp-style-engine-css-declarations.php

tests/phpunit/tests/style-engine/styleEngine.php

ramonjd · 2022-09-09T06:35:09Z

I'll fix up those failing tests later. Last commit was a bit rushed.

Thanks for the reviews, folks.

costdev

Thanks for the updates! I think these are the last round of changes from me. 🙂

As mentioned in the comments below, watch out for the array<string, string> to string[] An associative array changes causing alignment issues for @param annotations.

Also, the GHA failures don't seem to be test failures, but workflow failures, so it looks like your last commit was clean 🙂

src/wp-includes/style-engine/class-wp-style-engine.php

- Migration of the style engine classes and unit tests from Gutenberg to Core.

Updating test function names and annotations.

Updating php doc params

…is used in a test method.

Testing for WP_Style_Engine_CSS_Rules_Store in WP_Style_Engine_Processor->add_store

Counting calls to mock filters executed in safecss_filter_attr() (kses.php)

Removing duplicated test (we were already testing context below)

ramonjd · 2022-09-09T09:37:50Z

Also, the GHA failures don't seem to be test failures, but workflow failures, so it looks like your last commit was clean

Good to know! I was on my phone so wasn't sure. I see red and 😱

ramonjd · 2022-09-09T11:43:04Z

I might not be around next week. cc @andrewserong in case this PR and related PRs need any attention, though I think we should be sound 👍

costdev

LGTM pending the result of this discussion. 👍 Thanks @ramonjd!

New style engine does not emit spaces between attributes and values. See WordPress#3199.

ockham · 2022-09-13T14:40:01Z

LGTM pending the result of this discussion. 👍 Thanks @ramonjd!

That link doesn't seem to be working for me 😕 @costdev -- did you mean this discussion (about @access private and classes being final)? If so, has that been sufficiently answered by @ramonjd?

I'd love if we could land this PR soon, as it blocks a number of other Style Engine related PRs 😊
@andrewserong maybe you can help out a bit? 🙏

costdev · 2022-09-13T15:04:13Z

That's right @ockham, not sure why the original link isn't working.

I think we're waiting on feedback from @aristath and @andrewserong on that topic. Not sure if @azaozz has anything more to add as well.

ramonjd · 2022-09-13T21:59:20Z

I think we're waiting on feedback from @aristath and @AndrewSong on that topic.

For my part, I've been thinking about this, and I'm pretty confident that the function signatures and method names are fine for now. The classes have been running in Gutenberg for some time.

Ultimately the purpose of all this code is to accept block style input and output compiled CSS. I expect there'll be updates to the body of some functions, and probably unforeseen future conditions that will require new methods, and later, deprecations, but that's the story of WordPress.

The unknowns will remain unknowns :D

I'd lean on @aristath's and @AndrewSong's opinion though.

andrewserong · 2022-09-13T23:25:25Z

Thanks for pings!

For my part, I've been thinking about this, and I'm pretty confident that the function signatures and method names are fine for now. The classes have been running in Gutenberg for some time.

I agree, and like you mention in the thread, maintaining backwards compatibility is expected for the style engine classes. One of the things I believe @aristath had in mind with these classes was the eventual use cases of plugins and themes, so we ultimately do want these to be stable and well-supported classes with consistent public methods. The structure and naming of the public methods went through lots of iterations in the Gutenberg plugin and appears to be pretty stable now from my perspective.

I think this PR looks in a good shape to land, personally.

aristath · 2022-09-14T06:40:48Z

As mentioned above, the Style Engine classes went through a lot of iterations and discussion. What we ended up with is a hierarchy and structure that is solid, stable, makes sense, and I feel it can withstand the test of time 👍

audrasjb · 2022-09-14T12:47:33Z

Committed in https://core.trac.wordpress.org/changeset/54156

New style engine does not emit spaces between attributes and values. See WordPress#3199.

Spaces are stripped between properties and values since WordPress#3199.

ramonjd mentioned this pull request Sep 6, 2022

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

Closed

37 tasks

ramonjd force-pushed the add/style-engine-classes-migrate-6-1 branch from 97019a1 to d88d4a0 Compare September 6, 2022 05:30

ramonjd mentioned this pull request Sep 6, 2022

Style engine: pass CSS formatting options from global functions WordPress/gutenberg#43886

Merged

ockham mentioned this pull request Sep 6, 2022

Sync Gutenberg packages #3154

Closed

ramonjd force-pushed the add/style-engine-classes-migrate-6-1 branch from e8e00c6 to c0c64de Compare September 7, 2022 02:49

This was referenced Sep 7, 2022

Backport typography block supports for WP 6.1 #3203

Closed

Backporting block supports [border, color, elements, spacing] from Gutenberg to WP 6.1 #3204

Closed

Style engine: pre-core migration documentation and house keeping WordPress/gutenberg#43840

Merged

costdev requested changes Sep 7, 2022

View reviewed changes

ramonjd force-pushed the add/style-engine-classes-migrate-6-1 branch 4 times, most recently from 0166739 to 5116e16 Compare September 8, 2022 02:07

ramonjd force-pushed the add/style-engine-classes-migrate-6-1 branch 2 times, most recently from dadfd4d to e2e6323 Compare September 9, 2022 03:16

ramonjd mentioned this pull request Sep 9, 2022

Backport script loader: enqueue stored block supports styles #3218

Closed

costdev requested changes Sep 9, 2022

View reviewed changes

mukeshpanchal27 reviewed Sep 9, 2022

View reviewed changes

src/wp-includes/style-engine/class-wp-style-engine-css-declarations.php Outdated Show resolved Hide resolved

tests/phpunit/tests/style-engine/styleEngine.php Outdated Show resolved Hide resolved

costdev requested changes Sep 9, 2022

View reviewed changes

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

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

ramonjd added 9 commits September 9, 2022 19:37

Initial commit:

11a9aac

- Migration of the style engine classes and unit tests from Gutenberg to Core.

Adding @Covers annotations

8d87cd1

Changes from WordPress/gutenberg#43886

05b1903

Formatting PHP doc comments and params that describe associative arrays

d4fa390

Updating test function names and annotations.

formatting

5543539

Combining tests for get_declarations_string

f044bf4

Updating php doc params

Adding message parameter to assertions where more than one assertion …

73288d1

…is used in a test method.

Updating PHP docs

aa291de

Testing for WP_Style_Engine_CSS_Rules_Store in WP_Style_Engine_Processor->add_store

Refactoring sanitization tests for CSS declarations

cfcc425

Counting calls to mock filters executed in safecss_filter_attr() (kses.php)

ramonjd added 5 commits September 9, 2022 19:37

Formatting styleEngine tests

4b2128e

Removing duplicated test (we were already testing context below)

My blod is typo

5da7cf4

Imperative < active voice

d0de610

Remove Gutenberg suffix

3d2f156

renaming test group suite to kebab case

26cd739

ramonjd force-pushed the add/style-engine-classes-migrate-6-1 branch 2 times, most recently from bb87fe4 to 0ecd3a0 Compare September 9, 2022 09:58

Grammar, formatting, removing class_exists checks

57d81bc

ramonjd force-pushed the add/style-engine-classes-migrate-6-1 branch from 8c54963 to 57d81bc Compare September 9, 2022 10:30

costdev approved these changes Sep 9, 2022

View reviewed changes

Removing @access private from Style Engine utility classes

8e3266d

ironprogrammer added a commit to ironprogrammer/wordpress-develop that referenced this pull request Sep 12, 2022

Remove space between attribute and value

245a6d9

New style engine does not emit spaces between attributes and values. See WordPress#3199.

ironprogrammer mentioned this pull request Sep 13, 2022

Backport typography block supports for WP 6.1 (refresh) #3237

Closed

andrewserong mentioned this pull request Sep 14, 2022

Backport foundation for Layout block support refactor (part 1) #3205

Closed

hellofromtonya assigned audrasjb Sep 14, 2022

audrasjb closed this Sep 14, 2022

ironprogrammer added a commit to ironprogrammer/wordpress-develop that referenced this pull request Sep 14, 2022

Remove space between attribute and value

7fa1602

New style engine does not emit spaces between attributes and values. See WordPress#3199.

ockham mentioned this pull request Sep 15, 2022

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

Closed

ironprogrammer added a commit to ironprogrammer/wordpress-develop that referenced this pull request Sep 15, 2022

Remove spaces for typography styles

5ff82f6

Spaces are stripped between properties and values since WordPress#3199.

jrfnl mentioned this pull request Sep 28, 2022

PHP 8.2 | Gutenberg is introducing deprecated callables into WP Core WordPress/gutenberg#44536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style engine: migrate functions, classes and tests #3199

Style engine: migrate functions, classes and tests #3199

ramonjd commented Sep 6, 2022 •

edited

Loading

costdev left a comment •

edited

Loading

ramonjd commented Sep 8, 2022

costdev left a comment

costdev Sep 9, 2022 •

edited

Loading

ramonjd Sep 9, 2022

costdev Sep 9, 2022

azaozz Sep 9, 2022

costdev Sep 9, 2022

ramonjd Sep 9, 2022 •

edited

Loading

andrewserong Sep 13, 2022

mukeshpanchal27 left a comment

ramonjd commented Sep 9, 2022

costdev left a comment •

edited

Loading

ramonjd commented Sep 9, 2022

ramonjd commented Sep 9, 2022

costdev left a comment

ockham commented Sep 13, 2022

costdev commented Sep 13, 2022 •

edited

Loading

ramonjd commented Sep 13, 2022

andrewserong commented Sep 13, 2022

aristath commented Sep 14, 2022

audrasjb commented Sep 14, 2022

Style engine: migrate functions, classes and tests #3199

Style engine: migrate functions, classes and tests #3199

Conversation

ramonjd commented Sep 6, 2022 • edited Loading

Backport PRs blocked by this changeset (that I know of)

costdev left a comment • edited Loading

Choose a reason for hiding this comment

ramonjd commented Sep 8, 2022

costdev left a comment

Choose a reason for hiding this comment

costdev Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

ramonjd Sep 9, 2022

Choose a reason for hiding this comment

costdev Sep 9, 2022

Choose a reason for hiding this comment

azaozz Sep 9, 2022

Choose a reason for hiding this comment

costdev Sep 9, 2022

Choose a reason for hiding this comment

ramonjd Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

andrewserong Sep 13, 2022

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

ramonjd commented Sep 9, 2022

costdev left a comment • edited Loading

Choose a reason for hiding this comment

ramonjd commented Sep 9, 2022

ramonjd commented Sep 9, 2022

costdev left a comment

Choose a reason for hiding this comment

ockham commented Sep 13, 2022

costdev commented Sep 13, 2022 • edited Loading

ramonjd commented Sep 13, 2022

andrewserong commented Sep 13, 2022

aristath commented Sep 14, 2022

audrasjb commented Sep 14, 2022

ramonjd commented Sep 6, 2022 •

edited

Loading

costdev left a comment •

edited

Loading

costdev Sep 9, 2022 •

edited

Loading

ramonjd Sep 9, 2022 •

edited

Loading

costdev left a comment •

edited

Loading

costdev commented Sep 13, 2022 •

edited

Loading