search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.34k stars 4.13k forks source link

Code quality check for `theme.json` related APIs #45171

Open oandregal opened 1 year ago

oandregal commented 1 year ago

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:

Performance:

Sync WordPress core <=> Gutenberg:

WordPress 6.3: shipped

Public API

WordPress 6.4: ongoing

Performance:

Public API

Next

Algorithm:

Public API:

Performance

oandregal commented 1 year ago

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

oandregal commented 1 year ago

https://github.com/WordPress/gutenberg/pull/45168 is ready for review.

oandregal commented 1 year ago

Added a section about performance.

bph commented 1 year ago

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

spacedmonkey commented 1 year ago

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

spacedmonkey commented 1 year ago

Key goals of this refactor.

oandregal commented 1 year ago

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 commented 1 year ago

https://github.com/WordPress/wordpress-develop/pull/3556 backports to core the wp_theme_has_theme_json method.

spacedmonkey commented 1 year ago

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 commented 1 year ago

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

felixarntz commented 1 year ago

@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 https://github.com/WordPress/wordpress-develop/pull/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 commented 1 year ago

Added a new item in the performance category:

Consider updating the transients cache in wp_get_global_styles_svg_filters and wp_get_global_stylesheet by an object cache. See issues: https://github.com/WordPress/gutenberg/pull/41201 and https://github.com/WordPress/wordpress-develop/pull/2732

oandregal commented 1 year ago

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 https://github.com/WordPress/wordpress-develop/pull/3536 (it's a core PR, but may affect Gutenberg as well).

Yes, that'd be lovely. Please, share here any related work, so people interested has a central place to track how this evolves, and we can coordinate among the editor & performance folks working in this area (a bunch!).

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 :)

To keep busy work to a minimum and increase the test base for the changes to the maximum, the ideal flow would be:

  1. Create and merge PRs in Gutenberg.
  2. Backport them to core at some point.

Ideally, no (major) changes are necessary in 2 because code was already reviewed and iterated in 1. The key here is iterated. I personally want to favor small PRs that do one thing, so they can land faster. By being small & focused, they can also be reverted easily, should we need. This means that, sometimes, a PR lands before someone is able to review. That's fine. A post-review and creating follow-ups is great. It keeps us moving. Once a bunch of PRs in a particular area has landed and code feels stable, we can backport ahead of the beta cycle.

oandregal commented 1 year ago

Trying using the object cache for wp_theme_has_theme_json at https://github.com/WordPress/gutenberg/pull/45543

mmtr commented 1 year ago

@oandregal are you also planning to backport https://github.com/WordPress/wordpress-develop/pull/3517 into Gutenberg as part of the performance effort?

oandregal commented 1 year ago

Added a section about syncing Gutenberg and WordPress core. https://github.com/WordPress/gutenberg/pull/45634 landed today.

There's been activity in core that has not been ported to Gutenberg, it needs an audit. I also like to suggest a different organization for the theme.json related APIs, so backports are more straightforward.

oandregal commented 1 year ago

Updated the section about performance. https://github.com/WordPress/gutenberg/pull/45679 is migrating the gutenberg_get_global_stylesheet method from transients (1 minute expiration) to object cache and manage invalidation.

spacedmonkey commented 1 year ago

Created a related ticket - https://core.trac.wordpress.org/ticket/57057.

Improve performance by reducing the usage of wp_get_theme.

spacedmonkey commented 1 year ago

@oandregal @felixarntz I have created a PR, that does a number of this. This add a new method to WP_Theme for detecting theme json exists.

spacedmonkey commented 1 year ago

Created a new PR #45831

spacedmonkey commented 1 year ago

Created a PR for allowing developers to pass stylesheet to the wp_theme_has_theme_json function. This is a needed changed, as their are already multiple places in core where this is required. Sadly, this required a refactor of cache invalidation. But I think I got it working well.

oandregal commented 1 year ago

I want to give visibility to this conversation about performance. Whether/When or not to use wp_get_theme. I need more clarity about direction: we are removing it from some places because it's an expensive operation, and propose to add it to a few hotpaths.

spacedmonkey commented 1 year ago

hotpaths.

I am not sure what you mean here by hotpath. This is not a common term. I am guessing what you mean here, is a path to a code that is executed a lot.

To be clear, spinning up a whole WP_Theme object on a "hotpath", is wasteful. 99% of the time, you only need the stylesheet, which you can get from get_stylesheet. So removing it in those instances, saves compute, speeding the page up.

As for adding it in wp_theme_has_theme_json, that is not a "hotpath". It is cached, making it safe to call.

oandregal commented 1 year ago

I'm extracting small PRs from the PR to bring object cache to the settings object, so we can fast-track them and land changes iteratively:

spacedmonkey commented 1 year ago

Related: https://github.com/WordPress/wordpress-develop/pull/3624

oandregal commented 1 year ago

https://github.com/WordPress/gutenberg/pull/45679 has been merged.

spacedmonkey commented 1 year ago

Just want to flag that I am worried about cache invalidation and filtering here.

If we add caches to theme json data, it means that filters like this, this, this will not run as this data will be loaded from the object cache. If you have relay on these filters, they may stop working. This may mean we can not cache this data at all.

I also have worried about caching invalidation. The most worrying is this line. This loads get_default_block_editor_settings. This function contains a lot of things, including.

It is going to be hard to impossible to cache invalidation get_default_block_editor_settings. We may need to find a way to remove this data from the theme_json_resolver or otherwise cache it.

I discoveried the above while working on https://github.com/WordPress/wordpress-develop/pull/3624. Core unit tests failed when remove_theme_supports. This would be a problem with any chance that we add here.

Becuase of the above, we need to reconsider https://github.com/WordPress/gutenberg/pull/45679 and https://github.com/WordPress/gutenberg/pull/45372 until we can workout of how filter / cache invalidation will work.

oandregal commented 1 year ago

Just want to flag that I am worried about cache invalidation and filtering here.

I want to share that all of this is being discussed at https://github.com/WordPress/gutenberg/pull/45372

oandregal commented 1 year ago

https://github.com/WordPress/gutenberg/pull/46150 makes the caches introduced for wp_theme_has_theme_json and gutenberg_get_global_stylesheet non-persistent, as per some discussions at https://github.com/WordPress/gutenberg/pull/45372

oandregal commented 1 year ago

:wave: I want to step back a bit and take stock of the progress and next steps. The big things shipped in the past 6 weeks:

These have landed in many PRs. There are other smaller ones that either supported this progress or made other things faster as well.

As for next steps, what I find most impactful (hence would focus my efforts on them in the next weeks) is:

oandregal commented 1 year ago

I've been AFK the past week, but wanted to share current progress regarding backports:

oandregal commented 1 year ago

Current status:

Progress has been focused in syncing Gutenberg<=>Core:

Next steps for me: besides addressing any other core feedback on the backport PRs, my focus will be switched now to look for more performance gains and/or creating the remaining public methods (wp_get_remote_patterns_from_theme, wp_get_custom_templates_from_theme, wp_get_template_parts_from_theme).

oandregal commented 1 year ago

Current status:

Other than addressing feedback in the backports, I am not sure if I'll be able to make much progress on the other tasks listed here for WordPress 6.2 (feature freeze is 4 weeks away).

spacedmonkey commented 1 year ago

wp_get_global_styles_svg_filters: migrate transient to object cache. Needs PR.

There is a PR now https://github.com/WordPress/gutenberg/pull/47460

oandregal commented 1 year ago

Now that WordPress 6.2 beta is over, I've taken some time to reorganize the issue description so it's clear what has been shipped, what's ongoing, and what's next.

Shipped in WordPress 6.2

Public API:

Performance:

Sync WordPress core <=> Gutenberg:

It has seen some progress

Public API:

Performance:

Next

See issue description.

oandregal commented 1 year ago

Created a section for ongoing 6.3 work.

oandregal commented 1 year ago

Added a few tasks related to wp_get_global_styles:

jsnajdr commented 1 year ago

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 commented 1 year ago

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 https://github.com/WordPress/gutenberg/issues/49715#issuecomment-1508895208

oandregal commented 1 year ago

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

spacedmonkey commented 1 year ago

This PR is still awaiting review - https://github.com/WordPress/gutenberg/pull/45831

There is also https://github.com/WordPress/wordpress-develop/pull/3860 to look at as well.

dd32 commented 1 year ago

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 commented 1 year ago

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

I've prepared https://github.com/WordPress/wordpress-develop/pull/4640 to follow-up on this.

oandregal commented 1 year ago

PR for adding wp_get_theme_template_part_metadata https://github.com/WordPress/wordpress-develop/pull/4971

oandregal commented 1 year ago

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

felixarntz commented 1 year ago

@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 commented 1 year ago

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.