Make WordPress Core

Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#58555 closed task (blessed) (fixed)

Backport: Duotone changes, refactor, enhancements and fixes

Reported by: onemaggie's profile onemaggie Owned by: isabel_brison's profile isabel_brison
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-unit-tests gutenberg-merge has-patch
Focuses: Cc:

Description (last modified by kirasong)

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.

Attachments (2)

Screenshot 2023-07-11 at 22.15.25.png (1.1 MB) - added by spacedmonkey 9 months ago.
WordPress 6.3 beta 4 - duotone image.
Screenshot 2023-07-11 at 22.15.39.png (593.3 KB) - added by spacedmonkey 9 months ago.
WordPres 6.2.2 - Duotone image.

Download all attachments as: .zip

Change History (44)

This ticket was mentioned in PR #4619 on WordPress/wordpress-develop by @onemaggie.


10 months ago
#1

  • Keywords has-patch has-unit-tests added

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:

#2 @kebbet
10 months ago

  • Keywords gutenberg-merge added

@ramonopoly commented on PR #4619:


10 months ago
#4

Would this PR also need backporting for duotone to work?

cc @ajlende

@onemaggie commented on PR #4619:


10 months ago
#5

Would this PR also need backporting for duotone to work?

cc @ajlende

Apparently that was already ported to core here

#6 @kirasong
10 months ago

  • Milestone changed from Awaiting Review to 6.3

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

#7 @kirasong
10 months ago

  • Keywords changes-requested added

@ajlende commented on PR #4619:


10 months ago
#8

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

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


10 months ago

#10 @kirasong
10 months ago

  • Component changed from General to Editor
  • Description modified (diff)
  • Type changed from enhancement to task (blessed)

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.

Last edited 10 months ago by kirasong (previous) (diff)

@ramonopoly commented on PR #4619:


10 months ago
#11

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 (?)

@mikeschroder commented on PR #4619:


10 months ago
#13

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 commented on PR #4619:


10 months ago
#14

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 commented on PR #4619:


10 months ago
#15

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?

@ramonopoly commented on PR #4619:


10 months ago
#16

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 ?

@ajlende commented on PR #4619:


10 months ago
#17

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

#18 @isabel_brison
10 months ago

  • Owner set to isabel_brison
  • Resolution set to fixed
  • Status changed from new to closed

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.

@isabel_brison commented on PR #4619:


10 months ago
#19

committed in r56101 / d966798.

#20 @isabel_brison
10 months ago

In 56103:

Editor: delete test file from update duotone support.

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

See #58555.

#21 @flixos90
10 months ago

In 56110:

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

See #58235, #58301, #58555.

#22 @swissspidy
9 months ago

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.

#23 @swissspidy
9 months ago

  • Keywords needs-patch added; has-patch changes-requested removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for visibility because of #58673

#24 follow-up: @spacedmonkey
9 months ago

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

@spacedmonkey
9 months ago

WordPress 6.3 beta 4 - duotone image.

@spacedmonkey
9 months ago

WordPres 6.2.2 - Duotone image.

#26 in reply to: ↑ 24 @kirasong
9 months ago

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.

#27 @swissspidy
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I think [56226] resolved that issue

#28 @spacedmonkey
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#29 @kirasong
9 months ago

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

#30 follow-up: @ajlende
9 months ago

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.

#31 @swissspidy
9 months ago

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.

This ticket was mentioned in PR #4860 on WordPress/wordpress-develop by @spacedmonkey.


9 months ago
#32

  • Keywords has-patch added; needs-patch removed

#33 in reply to: ↑ 30 @spacedmonkey
9 months ago

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

@ajlende commented on PR #4860:


9 months ago
#34

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

#35 @isabel_brison
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

@spacedmonkey commented on PR #4860:


9 months ago
#36

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.

#37 @spacedmonkey
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#38 @hellofromTonya
9 months ago

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.

@hellofromTonya commented on PR #4860:


9 months ago
#39

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.

#40 @kirasong
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#41 @hellofromTonya
9 months ago

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.

#42 @swissspidy
9 months ago

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.

Note: See TracTickets for help on using tickets.