Backport: Duotone changes, refactor, enhancements and fixes

[onemaggie]

Ticket to handle backports of the changes made to the duotone related PHP files in Gutenberg for 6.3.

From the PR: 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.

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
Part of ​https://github.com/WordPress/gutenberg/issues/45171
Backports:

• [x] ​https://github.com/WordPress/gutenberg/pull/49103
• [x] ​https://github.com/WordPress/gutenberg/pull/49232
• [x] ​https://github.com/WordPress/gutenberg/pull/49212
• [x] ​https://github.com/WordPress/gutenberg/pull/49239
• [x] ​https://github.com/WordPress/gutenberg/pull/49393 I only backported the changes to class-wp-duotone-gutenberg.php because the rest seemed dependent on other unrelated changes
• [x] ​https://github.com/WordPress/gutenberg/pull/49423 I only backported the changes to class-wp-duotone-gutenberg.php because the rest seemed dependent on other unrelated changes
• [x] ​https://github.com/WordPress/gutenberg/pull/49700
• [x] ​https://github.com/WordPress/gutenberg/pull/49702
• [x] ​https://github.com/WordPress/gutenberg/pull/49705
• [x] ​https://github.com/WordPress/gutenberg/pull/49706
• [x] ​https://github.com/WordPress/gutenberg/pull/50714
• [x] ​https://github.com/WordPress/gutenberg/pull/51397

​Some of the duotone code depends on ​https://github.com/WordPress/gutenberg/pull/46496 getting backported first.

Backport PR that include ​https://github.com/WordPress/wordpress-develop/pull/4655 over here: ​https://github.com/WordPress/wordpress-develop/pull/4655

It also includes changes from ​https://github.com/WordPress/gutenberg/pull/49427

Would this PR also need backporting for duotone to work?

• ​https://github.com/WordPress/gutenberg/pull/50678

cc @ajlende

Would this PR also need backporting for duotone to work?

• ​Fix custom duotone filters in frontend gutenberg#50678

cc @ajlende

Apparently that was already ported to core ​here

[kirasong]

Adding to the 6.3 milestone, since it's listed here:
​https://github.com/WordPress/gutenberg/issues/51077

@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?

[kirasong]

Changing this to a task because it's a backport of existing Gutenberg changes into core for 6.3.

Also adding some of the PR's description to the ticket, to make it a bit clearer at a glance.

Personally, I'd love if we could do an initial commit before beta, even if this continues to have work/fixes done during beta.

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 ​https://github.com/WordPress/wordpress-develop/pull/4655 is committed and this PR rebased (?)

I just rebased this after ​https://github.com/WordPress/wordpress-develop/pull/4655 got merged

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

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.

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 ​https://github.com/WordPress/wordpress-develop/pull/4619#issuecomment-1608024130, would you be able to clarify?

I've rebased this onto trunk (locally) and tested in editor again.

Looking good functionality-wise! Duotone being output as expected on the frontend.

Thanks for all the hard work on this.

Is the @private annotation question the only thing holding this up code-wise @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?

Sorry, I'm unclear of the context for this ​https://github.com/WordPress/wordpress-develop/pull/4619#issuecomment-1608024130, would you be able to clarify?

@peterwilsoncc The exposed public methods should only be called once in a very particular order defined in ​block-supports/duotone.php—they shouldn't really be called otherwise.

This is one of the hooks for layout that has @access private that I was talking about—it should only get called during the render_block_core/image filter which happens later in the same file. ​https://github.com/WordPress/wordpress-develop/blob/80e424ad55e0ef4d48959a9c91b226eae45b6a77/src/wp-includes/block-supports/layout.php#L802-L813

[isabel_brison]

In 56101:

Editor: update duotone support.

Updates duotone support after stabilisation of selectors API and adds a few small code quality and UI improvements.

Props onemaggie, peterwilsoncc, ajlende, audrasjb, mikeschroder, ramonopoly.
Fixes #58555.

committed in r56101 / ​d966798.

[isabel_brison]

In 56103:

Editor: delete test file from update duotone support.

Deleting the test file that should have been removed in [56101].

See #58555.

[flixos90]

In 56110:

General: Add missing parentheses to functions referenced in _deprecated_function() calls added in 6.3.

See #58235, #58301, #58555.

[swissspidy]

FYI this change seems to cause test errors when running PHPUnit tests from a plugin, see #58673

If y'all could take a look at this and comment on #58673, that would be great.

[swissspidy]

Reopening for visibility because of #58673

[spacedmonkey]

I am seeing Duotone broken in classic themes. See attached screenshots.

[spacedmonkey]

WordPress 6.3 beta 4 - duotone image.

[spacedmonkey]

WordPres 6.2.2 - Duotone image.

[spacedmonkey]

Related: #58673.

[kirasong]

Replying to spacedmonkey:

I am seeing Duotone broken in classic themes. See attached screenshots.

Do the changes that have been made since your comment resolve the issue for you?

I tested on a classic theme, and duotone seems to be working as expected (in trunk) so far.

[swissspidy]

I think [56226] resolved that issue

[spacedmonkey]

I have a number of questions issues with the code as it stands. I hope someone can answer.

1. Why are their three deprecated methods in this new class? get_filter_svg_from_preset, get_filter_css_property_value_from_preset and get_filter_id_from_preset. It is very strange to me to introduce and then deprecated a method in the same release. This should be removed no?
2. Why were functions like wp_register_duotone_support deprecated? As a developer API, it seems much cleaner to have a function.
3. Why does the WP_Duotone use all static methods? I feel like having all static methods, basically makes it pointless of it being a class. It is just a namespaced function at that point. Either go back to functions and make this use methods. It should be simple enough to convert to none static with no downsides. Example

$duotone = new WP_Duotone();
// Add classnames to blocks using duotone support.
add_filter( 'render_block', array( $duotone, 'render_duotone_support' ), 10, 2 );

// Enqueue styles.
add_action( 'wp_enqueue_scripts', array( $duotone, 'output_block_styles' ), 9 );
add_action( 'wp_enqueue_scripts', array( $duotone, 'output_global_styles' ), 11 );

Static properties and methods have a number of downsides.

• There are harder to test, as they are stateful. In tests, simple create a new instance.
• Different from the rest of the code base. I know there are other instances of static methods in core, but for the most part these were done for a reason. I don't understand the reasoning here.
• Hard to follow and understand the code.

I would love the feedback of the committers here. @isabel_brison @mikeschroder @peterwilsoncc

[kirasong]

I don't feel like I have enough knowledge of the history to have a strong opinion at the moment.

cc: @ajlende @scruffian who I noticed wrote + reviewed a lot of the changes (+ @onemaggie who I think probably is already aware, since she is the ticket reporter).

Would any of y'all mind weighing in re: the above feedback?

For folks' reference, part one of the process of porting + refactoring is here:
​https://github.com/WordPress/gutenberg/pull/49700

[ajlende]

Why are their three deprecated methods in this new class? get_filter_svg_from_preset, get_filter_css_property_value_from_preset and get_filter_id_from_preset. It is very strange to me to introduce and then deprecated a method in the same release. This should be removed no?

​This was discussed on GitHub.

TL;DR While refactoring, pieces of the implementation were moved into private methods, so in order to keep the deprecated functions available they were added as deprecated public methods for the class.

Why were functions like wp_register_duotone_support deprecated? As a developer API, it seems much cleaner to have a function.

wp_register_duotone_support and the other functions were never meant to be part of the developer API and should only have been called via hooks that were replaced.

Why does the WP_Duotone use all static methods? I feel like having all static methods, basically makes it pointless of it being a class. It is just a namespaced function at that point. Either go back to functions and make this use methods. It should be simple enough to convert to none static with no downsides.

I had a discussion with @hellofromTonya and @azaozz about this earlier.

TL;DR Many of the static methods are private to hide implementation details, and we have a couple static properties for caching some computation. Doing it this way was simpler than the alternatives when the public methods are intended to only be used with the hooks.

[swissspidy]

While refactoring, pieces of the implementation were moved into private methods, so in order to keep the deprecated functions available they were added as deprecated public methods for the class.

Hmm maybe in that case better to mark them as internal or access private rather than deprecated. Otherwise a bit confusing.

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

[spacedmonkey]

TL;DR Many of the static methods are private to hide implementation details, and we have a couple static properties for caching some computation. Doing it this way was simpler than the alternatives when the public methods are intended to only be used with the hooks.

I am not sure why these need to be static to do this. You can have private method that are not static.

I have put together a PR for removing static. ​https://github.com/WordPress/wordpress-develop/pull/4860

I don't see any reason for this change. It was simpler before.

[isabel_brison]

@spacedmonkey what you suggest is a code quality change, and given that we're hours away from RC1 let's leave that for the next release. We can open a new ticket to discuss it separately.

I'm going to go ahead and close this one for now.

I don't see any reason for this change. It was simpler before, and this introduces an unnecessary global.

The code as is stands, it is harder to maintain, does not follow the design pattern of other class / code. There is no new global added here, just a local variable.

[spacedmonkey]

@isabel_brison As you know, once code is in core is can not be changed. Either it changed now and it will never get changed. I am strongly against the use of static in this class. My PR proves the fact it is unneeded. Code quality is important, as this code has be maintained. Static code like this, is hard to maintain as it does follow the pattern of other core classes.

[hellofromTonya]

I am strongly against the use of static in this class.

While I know you are against static class wrapper design pattern @spacedmonkey, I respectfully disagree.

This ​design pattern has been discussed previously in Gutenberg with ​support from @azaozz.

My PR proves the fact it is unneeded.

How does your PR prove it?

Your PR converts it into another design pattern, i.e. OOP. IMO one design pattern is not better than the other as both are valid and acceptable within WordPress Core.

One of the disadvantages of OOP is the need of a global way to access the object(s) for usage within Core, hooks and extender customization. Globals can introduce issues such as added costs of it adds a global variable to gain access to the object, which includes the cost of maintenance (as the global must be maintained forever within the project) and the potential instability or unexpected behavior if the global is overwritten.

IMO I do not see compelling reasons to warrant changing the architectural design pattern from static class wrapper to OOP, especially within Core at this late stage. I think the request to change to OOP is personal preference (I mean no disrespect).

I think the time to have discussed this was during the development of this class in Gutenberg's development cycle. At this stage, the class has already been through Gutenberg's development, release, feedback, and testing cycles. Without compelling reasons, I think it's too late to change it now. In doing so could introduce risks to (a) the release as unintended and possibly unknown issues or bugs could be introduced and (b) extenders who may already being customizing or using the class.

My vote is to reclose this ticket as fixed.

I respectfully disagree with the changes in this PR. I do not see compelling reasons to convert this class from a static class wrapper design pattern to OOP.

I've previously shared ​why I believe and support the usage of static class wrappers and when to use them vs global functions and OOP.

Class wrappers (using all static properties and methods) are an architectural design pattern and do indeed provide many benefits.

I left my feedback and opinions on the Trac ticket https://core.trac.wordpress.org/ticket/58555#comment:38.

The code as is stands, it is harder to maintain

Why is harder to maintain than OOP within the WordPress open source project?

I disagree. I think the code is not harder to maintain.

IMO OOP's requirement of a global way to access the object (i.e. via global function or variable) makes the code harder to maintain because of BC and the risk of overwriting the global object concerns.

[kirasong]

Given @isabel_brison's existing decision on a direction forward, along with the history + details shared and timeline, I'm going to go ahead and re-close this ticket as fixed.

As was suggested, please feel free to open another ticket to discuss this separately.

[hellofromTonya]

Following up with a way forward for the architectural design pattern change being requested:

@spacedmonkey I think this ​kind of change needs to brought up at the project-level. It's an interesting discussion that warrants a more broad, project-level consideration.

How to move it forward?

An actionable and well argued Make/Core Proposal can be an effective way to advocate for an architectural design change, especially for a change that requires a particular design pattern not be used within the project. If adopted, then all new classes being introduced in Gutenberg and/or Core would need to comply to the new coding standards.

[swissspidy]

I think the time to have discussed this was during the development of this class in Gutenberg's development cycle. At this stage, the class has already been through Gutenberg's development, release, feedback, and testing cycles.

Just a general observation / food for thought: I don't necessarily agree with this. Gutenberg development is so independent and different from core that only few core contributors keep track of what's going on there. And when we learn about a change (like with this ticket), it's often too late to make substantial changes. Then we have to deal with the consequences of doing last minute bug fixes and reverts because of a lack of code quality.

this kind of change needs to brought up at the project-level. It's an interesting discussion that warrants a more broad, project-level consideration.

I do 100% agree with this though. I hope the community summit helps with this.