Backport: Duotone changes #4619

MaggieCabrera · 2023-06-15T08:44:18Z

This PR backports the changes made to the duotone related PHP files. I grouped them together instead of having one PR for each change because they would all depend on each other, which made more sense. This PR depends on the stabilized selectors API to be backported for the changes to work.

Trac ticket: https://core.trac.wordpress.org/ticket/58555
Backports:

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.

tests/phpunit/tests/block-supports/duotone.php

ajlende · 2023-06-15T19:23:43Z

Some of the duotone code depends on WordPress/gutenberg#46496 getting backported first.

tellthemachines · 2023-06-16T06:11:06Z

@MaggieCabrera is there a Trac ticket for these changes? If not, we'll need to create one and add it in the PR description.

MaggieCabrera · 2023-06-16T07:50:11Z

@MaggieCabrera is there a Trac ticket for these changes? If not, we'll need to create one and add it in the PR description.

No, I was planning on doing that today

audrasjb

Thanks for the PR!
I left some comments concerning this changeset :)

src/wp-includes/block-supports/duotone.php

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

tests/phpunit/tests/block-supports/duotone.php

ramonjd · 2023-06-21T05:41:02Z

Some of the duotone code depends on WordPress/gutenberg#46496 getting backported first.

Backport PR that includes #4619 over here:

Block.json: Refactor and stabilize selectors API #4655

It also includes changes from WordPress/gutenberg#49427

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

ramonjd · 2023-06-23T01:44:44Z

Would this PR also need backporting for duotone to work?

Fix custom duotone filters in frontend gutenberg#50678

cc @ajlende

peterwilsoncc

I've added a few notes relating to the docblocks and locations.

I've used 🔢 to indicate that the item applies in multiple locations.

tests/phpunit/tests/block-supports/duotone.php

peterwilsoncc · 2023-06-23T05:26:10Z

src/wp-includes/block-supports/duotone.php

+		'register_attribute' => array( 'WP_Duotone', 'register_duotone_support' ),
+	)
+);
+


This block ought to be in the default-filters file.

All of the other block supports (layout and position) have their hooks co-located with the support registration in block-supports. Should those be moved too? Or is it okay having these here?

@ajlende let's leave it as is for now (including in this PR). They probably all should be in default-filters but probably a discussion for another time.

src/wp-includes/block-supports/duotone.php

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

peterwilsoncc · 2023-06-23T05:43:39Z

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

+	 * Migrate the old experimental duotone support flag to its stabilized location
+	 * under `supports.filter.duotone` and sets.


Again this needs to be split in to a summary and longer description/

When the developer docs are generated the first line is a short summary displayed below the title, followed by a longer description that goes in to detail. See https://developer.wordpress.org/reference/classes/wp_query/get_posts/ fpr example.

🔢 This chnage is needed in numerous places so I'll get you to reeveluate them rather than continue adding noise to the PR

@ajlende can you help with this one?

Updated in 0b06740.

tests/phpunit/tests/block-supports/duotone.php

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

MaggieCabrera · 2023-06-26T11:38:58Z

Would this PR also need backporting for duotone to work?
* [Fix custom duotone filters in frontend gutenberg#50678](https://github.com/WordPress/gutenberg/pull/50678)
cc @ajlende

Apparently that was already ported to core here

ajlende · 2023-06-26T18:38:28Z

@peterwilsoncc All of the public methods are to be used exclusively with the hooks that we have defined. The other block supports have @access private added to their hooks—can that be used for public methods on the WP_Duotone class or should the whole class be marked private somehow?

ramonjd

I've tested this manually on top of:

Block.json: Refactor and stabilize selectors API #4655

Functionality-wise things are working well for me.

I think it'll just need a final comb from a core committer?

ramonjd · 2023-06-27T06:46:59Z

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

+	 * @param string $slug The slug of the duotone preset.
+	 * @return string The CSS variable.
+	 */
+	private static function get_css_var( $slug ) {


Do all these new methods require

@since 6.3.0

?

Fixed in 817b916

ramonjd · 2023-06-27T06:49:17Z

src/wp-includes/class-wp-theme-json.php

@@ -143,8 +143,8 @@ class WP_Theme_JSON {
 			'path'              => array( 'color', 'duotone' ),
 			'prevent_override'  => array( 'color', 'defaultDuotone' ),
 			'use_default_names' => false,
-			'value_func'        => 'wp_get_duotone_filter_property',
-			'css_vars'          => '--wp--preset--duotone--$slug',
+			'value_func'        => null, // CSS Custom Properties for duotone are handled by block supports in class-wp-duotone.php.


Suggested annotation for docblock

* @since 6.3.0 Replaced value_func for duoone with null. Custom properties are handled by class-wp-duotone.php.

Fixed in 3265f93

ramonjd · 2023-06-27T07:08:41Z

Hmm some of the failing tests are real though.

I'm seeing -wp--preset--duotone--custom-duotone: url('#wp-duotone-custom-duotone'); not being output in Tests_Theme_wpThemeJson::test_get_stylesheet

🤷

I guess we can rerun and debug when and if #4655 is committed and this PR rebased (?)

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

MaggieCabrera · 2023-06-27T08:52:53Z

I just rebased this after #4655 got merged

getsource · 2023-06-27T15:53:24Z

This may be something you're already checking into, but in case it helps -- it looks like a few of the test failures may be PHPUnit tests that need to be updated:
https://github.com/WordPress/wordpress-develop/actions/runs/5387774760/jobs/9779547969#step:17:328

ajlende · 2023-06-27T21:53:45Z

Alright, I think the remaining failing tests are unrelated to these changes, and all the feedback has been addressed. I've also tested it now that it's been rebased with the selectors API changes. So this should be ready to go except for confirming one thing:

The other block supports have @access private added to their hooks—can that be used for public methods on the WP_Duotone class or should the whole class be marked private somehow?

I added @access private to the whole class. I hope that keeps docs from being generated for developer.wordpress.org. The documentation on docblock tags isn't clear about what prevents docs from being generated.

peterwilsoncc · 2023-06-28T04:39:23Z

I added @access private to the whole class. I hope that keeps docs from being generated for developer.wordpress.org. The documentation on docblock tags isn't clear about what prevents docs from being generated.

@ajlende Items with @access private have posts generated on the developer docs, see _cleanup_image_add_caption() as an example (chosen at random).

Sorry, I'm unclear of the context for this #4619 (comment), would you be able to clarify?

Co-authored-by: Peter Wilson <[email protected]>

…s_svg_filters

tellthemachines

LGTM! Duotone filters working well in editor and front end.

tellthemachines · 2023-06-29T06:21:09Z

committed in r56101 / d966798.

MaggieCabrera mentioned this pull request Jun 15, 2023

Plugin: Synchronize PHP changes for WordPress 6.3 release WordPress/gutenberg#51077

Closed

MaggieCabrera commented Jun 15, 2023

View reviewed changes

tests/phpunit/tests/block-supports/duotone.php Outdated Show resolved Hide resolved

audrasjb requested changes Jun 16, 2023

View reviewed changes

ramonjd reviewed Jun 21, 2023

View reviewed changes

tests/phpunit/tests/block-supports/duotone.php Show resolved Hide resolved

ramonjd reviewed Jun 21, 2023

View reviewed changes

tests/phpunit/tests/block-supports/duotone.php Show resolved Hide resolved

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jun 21, 2023

Updating classnames for wpGetBlockCssSelector.php tests

9fa4f93

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

ramonjd mentioned this pull request Jun 21, 2023

Block.json: Refactor and stabilize selectors API #4655

Closed

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jun 22, 2023

Updating classnames for wpGetBlockCssSelector.php tests

f004fc6

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

peterwilsoncc reviewed Jun 23, 2023

View reviewed changes

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jun 25, 2023

Updating classnames for wpGetBlockCssSelector.php tests

e765f17

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jun 26, 2023

Updating classnames for wpGetBlockCssSelector.php tests

e8221e6

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

MaggieCabrera force-pushed the duotone-backports branch from f137c68 to 2631e83 Compare June 26, 2023 14:27

ajlende force-pushed the duotone-backports branch 2 times, most recently from 9f04e3c to 15c4d35 Compare June 26, 2023 17:55

ramonjd reviewed Jun 27, 2023

View reviewed changes

tellthemachines pushed a commit to ramonjd/wordpress-develop that referenced this pull request Jun 27, 2023

Updating classnames for wpGetBlockCssSelector.php tests

e2c1297

Reverting changes to Tests_Theme_wpThemeJson that will need to be reinstated after WordPress#4619 merges

MaggieCabrera force-pushed the duotone-backports branch from 0b06740 to 386fd78 Compare June 27, 2023 08:52

ajlende force-pushed the duotone-backports branch from b7ffac2 to e2cf339 Compare June 27, 2023 16:48

MaggieCabrera and others added 24 commits June 29, 2023 11:23

Update tests/phpunit/tests/block-supports/duotone.php

f78d01b

Co-authored-by: Peter Wilson <[email protected]>

Update src/wp-includes/class-wp-duotone.php

7106f33

Co-authored-by: Peter Wilson <[email protected]>

Update src/wp-includes/class-wp-duotone.php

4530c53

Co-authored-by: Peter Wilson <[email protected]>

fix docblocks for tests

071e093

fix multiline comments

eb1c12e

Update src/wp-includes/class-wp-duotone.php

956e65c

Co-authored-by: Peter Wilson <[email protected]>

fix colord functions docblocks

6dad8db

Update src/wp-includes/class-wp-duotone.php

f1c26ea

Co-authored-by: Peter Wilson <[email protected]>

Update src/wp-includes/class-wp-duotone.php

49f0630

Co-authored-by: Peter Wilson <[email protected]>

Update src/wp-includes/class-wp-duotone.php

7d296b4

Co-authored-by: Peter Wilson <[email protected]>

moved deprecated functions to the correct file

e18f0ae

Mark private and new deprecated methods as internal

ae122da

Format line wrapping in comment

6a91d93

Update deprecated function docblock link and order

5141c6a

Fix trailing space

ec248c2

Add more description to the WP_Duotone methods

375ce41

Deprecate wp_global_styles_render_svg_filters and wp_get_global_style…

895363a

…s_svg_filters

Remove tests for deprecated functions

5347e2c

Move newly deprecated functions to deprecated.php

1476cff

Add changelog to PRESETS_METADATA

d74a917

Add @SInCE 6.3.0 to all duotone methods

710858d

Mark class WP_Duotone as @access private

a114db1

Fix theme json test string

105bd9e

Fix lint error

2541786

tellthemachines force-pushed the duotone-backports branch from c545054 to 2541786 Compare June 29, 2023 01:23

tellthemachines requested a review from audrasjb June 29, 2023 05:41

tellthemachines approved these changes Jun 29, 2023

View reviewed changes

tellthemachines closed this Jun 29, 2023

ajlende mentioned this pull request Jul 12, 2023

Lazily load Duotone settings only when needed. #4831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport: Duotone changes #4619

Backport: Duotone changes #4619

MaggieCabrera commented Jun 15, 2023 •

edited

Loading

ajlende commented Jun 15, 2023

tellthemachines commented Jun 16, 2023

MaggieCabrera commented Jun 16, 2023

audrasjb left a comment

ramonjd commented Jun 21, 2023 •

edited

Loading

ramonjd commented Jun 23, 2023

peterwilsoncc left a comment

peterwilsoncc Jun 23, 2023

ajlende Jun 26, 2023

peterwilsoncc Jun 28, 2023

peterwilsoncc Jun 23, 2023

MaggieCabrera Jun 26, 2023

ajlende Jun 26, 2023

MaggieCabrera commented Jun 26, 2023

ajlende commented Jun 26, 2023

ramonjd left a comment

ramonjd Jun 27, 2023

ajlende Jun 27, 2023

ramonjd Jun 27, 2023

ajlende Jun 27, 2023

ramonjd commented Jun 27, 2023

MaggieCabrera commented Jun 27, 2023

getsource commented Jun 27, 2023

ajlende commented Jun 27, 2023

peterwilsoncc commented Jun 28, 2023

tellthemachines left a comment

tellthemachines commented Jun 29, 2023

		* Migrate the old experimental duotone support flag to its stabilized location
		* under `supports.filter.duotone` and sets.

Backport: Duotone changes #4619

Backport: Duotone changes #4619

Conversation

MaggieCabrera commented Jun 15, 2023 • edited Loading

ajlende commented Jun 15, 2023

tellthemachines commented Jun 16, 2023

MaggieCabrera commented Jun 16, 2023

audrasjb left a comment

Choose a reason for hiding this comment

ramonjd commented Jun 21, 2023 • edited Loading

ramonjd commented Jun 23, 2023

peterwilsoncc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaggieCabrera commented Jun 26, 2023

ajlende commented Jun 26, 2023

ramonjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd commented Jun 27, 2023

MaggieCabrera commented Jun 27, 2023

getsource commented Jun 27, 2023

ajlende commented Jun 27, 2023

peterwilsoncc commented Jun 28, 2023

tellthemachines left a comment

Choose a reason for hiding this comment

tellthemachines commented Jun 29, 2023

MaggieCabrera commented Jun 15, 2023 •

edited

Loading

ramonjd commented Jun 21, 2023 •

edited

Loading