Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for merging plugin theme.json into active block theme.json #42573

Closed
wants to merge 4 commits into from

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Jul 20, 2022

What?

This is an initial working proof of concept solving #41707 with a goal of serving as a basis for further discussion on what we want to land as the initial iteration in Gutenberg.

Note: I'm not 100% clear on the strategy for what classes (compat classes) the code should be in for easier potential merge to WP core so reviewers please let me know if changes are needed related to that.

Why?

See #41707 for details. However, in summary, with the advent of theme.json becoming the primary entry point for much of the defaults around a site's customization and personalization, there is a need for a mechanism for plugins to:

  • provide default styles or settings for their blocks that can be overridden by themes.
  • expose custom properties that can be overridden by themes.
  • provide and expose a set of custom templates for post types.
  • indicate what patterns to register from the Patterns directory.

Preliminary discussion in #41707 suggests that accepting a theme.json provided by a plugin might be a suitable mechanism for this. This PR provides a working POC that can serves as a reference for further evaluation and discussion.

How?

In this PR:

  • Any active plugin with a theme.json in its root directory will have that file merged with the properties of the active theme.json on the site.
  • In the priority chain, the plugins theme.json will sit between blocks and themes (so the theme's theme.json will override that provided by plugins).
  • Currently, the merge strategy for multiple plugins is kept simple and simply follows the order returned from calling the wp_get_active_and_valid_plugins function (I think currently alphabetically?).

Some remaining open questions that are not addressed in this PR

Caching/Performance

There is caching per request (in keeping with the current behaviour for other sources) that exists for retrieving plugin-provided theme.json files. It should be fairly performant already, but there may be potential that sites with a lot of plugins might be impacted given the discovery part of the process. There is an open question around whether more persistent caching across requests might be needed. I have some ideas there (including resetting the cache on plugin activation and deactivation) but I think that's something that could be done in a subsequent iteration if it's deemed needed. Any persistent caching would likely have to consider network vs site activated plugins.

Pattern retrieval

Via _register_remote_theme_patterns, the patterns property is returned only from the theme.json provided by the active theme (not the merged theme.json). If we want pattern slugs provided by plugins theme.json to be included here then this would need to be modified.

i18n

Currently, I think the same method of translation for available properties is handled in the code submitted. However, I only looked at this shallowly so it'd need a good review by someone familiar with how that works. I know this definitely won't handle any custom properties that plugins might extend on theme.json.

Allowing custom properties

Speaking of which, plugins will not be able to enhance theme.json with custom properties with this current PR. It'd require work in the WP_Theme_JSON (or equivalent compat class) I'm on the fence whether, this should be allowed or not given:

  • theme.json has a specific schema.
  • the use case for custom properties still isn't very clear to me yet.

I think if this is a demonstrable need, we should address this separately. However, I mention it here because it could impact the evaluation of this PR as a viable approach overall.

Merging strategy for multiple plugins with a theme.json file

While there are some comments indicating places we could potentially implement a different merge strategy for plugins theme.json files, it's not clear what that strategy should be (or even if it's necessary). I'm leaning more towards this mechanism being provided as a method for plugins to add defaults specific to their exposed blocks/patterns/templates to the active theme and not a mechanism for overriding other plugins defaults (which should be the purview of a theme). However, I know practically this doesn't leave room for the nuances of other use-cases where that may be desired.

Hence, this is very much an open question. I do wonder though if it'd be sufficient to roll out this POC as is and deal with the merging strategy in subsequent iterations once those use-cases are clearer?

Restrict which properties plugins can define?

Somewhat related to the merging strategy question, while this PR is liberal in what plugins can define in their theme.json file, I do wonder if it's too liberal. With the current merge strategy, if plugins define every property then there will always be one plugin that "wins" in the final rendered "plugin provided theme.json file" which could lead to unexpected behaviour. So this opens the question of whether we should either restrict which properties plugins can define or potentially have plugins only provide a custom .json file that only merges with a subset of theme.json properties. The latter aligns mostly with what Luis suggested here.

Testing Instructions

There are some PHPUnit tests that cover the expected behaviour viewable in this PR, but it's also testable using the following instructions.

  1. All you need for testing is a plugin with a defined theme.json file. You can use what is implemented for PHPUnit tests in this PR.
  2. Activate the plugin.
  3. Take note of what properties are new and not overridden by the active theme's theme.json and verify that those are utilized by Gutenberg in the editor (for instance a custom block styles property defined in the plugin provided theme.json).
  4. Take note of what properties are also present in the active theme's theme.json and ensure that when viewed in the editor that the active theme's implementation is what is preserved.

@nerrad nerrad self-assigned this Jul 20, 2022
@nerrad nerrad added [Feature] Extensibility The ability to extend blocks or the editing experience Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jul 20, 2022
@nerrad nerrad force-pushed the add/support-for-plugin-theme-json branch from 5ffa617 to 7bdb674 Compare July 26, 2022 18:40
@nerrad nerrad marked this pull request as ready for review July 26, 2022 19:25
@nerrad nerrad requested a review from spacedmonkey as a code owner July 26, 2022 19:25
@nerrad nerrad linked an issue Jul 26, 2022 that may be closed by this pull request
@nerrad
Copy link
Contributor Author

nerrad commented Sep 14, 2022

Closing in favor of #44015.

@nerrad nerrad closed this Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow plugins to include a theme.json file in the same way themes do
1 participant