Code quality check for theme.json related APIs

[oandregal]

This issue tracks the current state of theme.json related APIs from a quality point of view, so we can bring up, discuss and address the issues.

WordPress 6.2: shipped

Public API:

• Add new method wp_theme_has_theme_json. #45168 #45380

Performance:

• wp_theme_has_theme_json: cache output #45543
• wp_get_global_settings: cache output #45372 #45971 #45969
• wp_get_global_stylesheet: migrate from transient to object cache #45679
• wp_get_global_styles_svg_filters : migrate from transient to object cache #47460 (done in core as the function was removed from Gutenberg)
• While initially the cache was implemented as persistent across requests for the above methods, concerns were brought up to make it non-persistent and keep testing: #46150

Sync WordPress core <=> Gutenberg:

• Try: bundle WP_Theme_JSON class instead of inheriting per WordPress version #46579
• Try: bundle the WP_Theme_JSON_Resolver class instead of inheriting per WordPress version #46750

WordPress 6.3: shipped

Public API

• Add gutenberg_get_remote_theme_patterns function #49307
• block.json API: consider graduating __experimentalSelector and __experimentalDuotone from experimental to stable #45194
• wp_get_global_styles: for custom props, it should return the CSS format not the shortened internal one #49693
• wp_get_global_styles: return values instead of CSS Custom Properties #49712

WordPress 6.4: ongoing

Performance:

• get_root_layout_rules: remove unnecessary call to sanitize_title #53568
• Compute presets from theme.json: skip those without classes or variables #53574
• Font Face API: use gutenberg_get_global_settings instead of private API #53805
• Optimize compute_style_properties by reducing the number of iterations it performs #51983
• Try/improve get stylesheet performance #53351
• webfonts handler: use wp_get_global_settings instead of private API wordpress-develop#4980
• Try: reduce number of WP_Theme_JSON instances #53357
• Try: get_style_nodes: only return those that will be used #53575

Public API

• Add wp_get_theme_data_template_parts: create public API to access templateParts from theme.json wordpress-develop#4971
• Add wp_get_theme_data_custom_templates wordpress-develop#5031

Next

Algorithm:

• Iterate on absorbing block styles as part of the global styles mechanism #45198

Public API:

• Make sure all current uses cases of WP_Theme_JSON or WP_Theme_JSON_Resolver are covered by the addition of public methods.
• Collocate customTemplates and templateAreas metadata with the actual data they represent (templates and template parts provided by the theme as HTML) and remove them from theme.json. #42732 In testing #38984 we ran into an issue with valid HTML comments that don't belong to a block #47212
• Deprecate/Remove the wp_theme_json_get_style_nodes filter #45172 Filters, together with methods or the theme.json datum, are all part of the public API. We should aim to not expose private implementation details in the public API that prevent us from evolving it.
• wp_get_global_styles: resolver the reference to the value #49715

Performance

• Make theme.json caches persistent across requests: Control whether theme (e.g. theme.json) cache usage is enabled through a dedicated function #45912
• wp_get_global_styles: add object cache.
• Try caching stylesheets generated for the editor https://github.com/WordPress/gutenberg/pull/45679/files#r1024084613
• wp_add_global_styles_for_blocks: review status.
• Styles generation: there are two points for styles, get_stylesheet (renders the top-level styles) and get_styles_for_block (renders the block-level styles). Potential optimizations:
  • top-level styles: remove any operation that is not relevant for top-level.
  • block-level styles: look for ways to only process the theme.json nodes of the blocks in use. At the moment all are processed (see wp_add_global_styles_for_blocks), though only the blocks in use get enqueued.

[oandregal]

Added a section on experimental API that covers __experimentalSelector, __experimentalDuotone, and __experimentalStyle.

[oandregal]

#45168 is ready for review.

[oandregal]

Added a section about performance.

[bph]

Added the 'needs-dev-note` to this tracking issue, to make sure we cover all relevant changes for the WordPress 6.2 release.

[spacedmonkey]

Related: https://core.trac.wordpress.org/ticket/56945 . CC @felixarntz

[spacedmonkey]

Key goals of this refactor.

• Do no run logic on themes that do not support theme.json ( have this file ).
• Only run the logic once.
• Add set of developer focus functions that stop them from running code again.
• Add unit tests.
• Make code standard with rest of core.
• Make code easier to test.

[oandregal]

Key goals of this refactor.

Hey @spacedmonkey your comment may induce people to think this is about a refactor, that code does not have unit test, etc.
That's not true.

The issue description should be clear enough, though reiterating never hurts for expectations to be clear for everyone. This is about coordinating work among the dozens of people working on this area by highlighting actionable issues that will have an impact on user experience and/or code longevity.

[oandregal]

WordPress/wordpress-develop#3556 backports to core the wp_theme_has_theme_json method.

[spacedmonkey]

Hey @spacedmonkey your comment may induce people to think this is about a refactor, that code does not have unit test, etc. That's not true.

Confused by this comment. I am just expanding on all this work should include refactoring to improve code quality and testability. A goal we should always keep in mind for any change to code.

[spacedmonkey]

Seems like a duplicate: https://core.trac.wordpress.org/ticket/56974 . CC @aristath

[felixarntz]

@oandregal Most of this issue appears to cover general code quality, however in the description you also mention performance. Would you like to use this issue as a "tracker" / "overview" issue for performance-related enhancements/fixes as well? If so, potentially we should add WordPress/wordpress-develop#3536 (it's a core PR, but may affect Gutenberg as well).

On that note, to be fully transparent here, I'd love to have some guidance for whether something like the above should rather be opened as a Gutenberg PR or core PR in the first place :)

[oandregal]

Created a section for ongoing 6.3 work.

[oandregal]

Added a few tasks related to wp_get_global_styles:

• wp_get_global_styles: for custom props, it should return the CSS format not the shortened internal one #49693
• wp_get_global_styles: return values instead of CSS Custom Properties #49712
• wp_get_global_styles: resolver the reference to the value #49715

[jsnajdr]

Looking at the three wp_get_global_styles issues, they all want to resolve something, or convert it to a "canonical" value, so maybe the best API is not to create separate wp_get_global_styles_resolve_x functions, but to add an options parameter:

wp_get_global_styles( $path, $context, $options )

where the options is an object with fields like resolve_refs, resolve_css_vars etc.

With the "resolve internal CSS var format to standard" and "resolve refs" options, it seems to me they should be resolved by default. Only the "resolve CSS variables into real values" should 100% be an option.

[oandregal]

With the "resolve internal CSS var format to standard" and "resolve refs" options, it seems to me they should be resolved by default. Only the "resolve CSS variables into real values" should 100% be an option.

Agreed with the "internal CSS var", it should always be resolved: this is the easiest and @samnajian is looking into that.

Also agreed on "resolve CSS variables" being not the default behavior. It's only a few setups (different rendering pipelines such as mobile, mails, RSS feed) that would need this one.

I'm not sure about the "resolve refs": as far as my understanding goes, this aims to "declare links" between different style tokens. It may or may not be important for consumers to know those links exist, depending on what they want to do. See conversation at #49715 (comment)

[oandregal]

Relevant thread for the conversation about "transforming/resolving" references and variables in style values https://github.com/WordPress/gutenberg/pull/50484/files#r1192502874

[spacedmonkey]

This PR is still awaiting review - #45831

There is also WordPress/wordpress-develop#3860 to look at as well.

[dd32]

Syncing my comment from https://core.trac.wordpress.org/ticket/58460 over here:

The name wp_get_remote_theme_patterns() feels a bit close to the wp_remote_*() and wp_safe_remote_*() methods to me. The inclusion of remote when it's not actually performing a remote-request seems weird too (I get that it's to say "This returns something that needs to be fetched remotely")

I would suggest that something along the lines of wp_theme_required_patterns() or ...pattern_slugs() is maybe a little more understandable as to what it means? ie. wp_ (it's a WP function) theme_ (It's related to the themes component) required_ (It might not be required, and rather suggested?) pattern_slugs() what's being queried for.

Looking at the rest of this PR, wp_get_... works as the next component is wp_get_global_..., but for wp_get_remote_theme_patterns() I still feel like the above comments make sense.

[oandregal]

Looking at the rest of this PR, wp_get_... works as the next component is wp_get_global_..., but for wp_get_remote_theme_patterns() I still feel like the above comments make sense.

I've prepared WordPress/wordpress-develop#4640 to follow-up on this.

[oandregal]

PR for adding wp_get_theme_template_part_metadata WordPress/wordpress-develop#4971

[oandregal]

I've also updated the issue description with what we shipped in 6.3 and ongoing work for 6.4.

[felixarntz]

@oandregal Just checking in here for whether the issue description is still accurate? Is there any other work related to this that you intend to get into 6.4? Happy to help where I can!

[oandregal]

Just checking in here for whether the issue description is still accurate? Is there any other work related to this that you intend to get into 6.4? Happy to help where I can!

@felixarntz yeah, it is. The two opened PRs related to performance are in draft at the moment, I don't think I'll be able to get them ready on time before beta. I'd like to revisit this work for the next cycles.