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

Increment theme.json version to 3 #40232

Closed
wants to merge 5 commits into from
Closed

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Apr 11, 2022

What?

This PR increments the theme.json version to 3.

Why?

We've added some new things to the theme.json schema, so we need to publish a new one to document in which WordPress version a specific setting can be found.

There's no modifications or removals in the existing schema, only additions:

  • title top-level key
  • patterns top-level key
  • settings.color.defaultDuotone setting key

The behavior is the same as before, this serves to semantically mark the new files. This version change will be documented in the block editor handbook as well with a PR sent to the Gutenberg repository.

How?

Updates the version in the theme.json file bundled with Gutenberg and in the theme.json class.

Related PR to core at WordPress/wordpress-develop#2565

Testing Instructions

Does not have any effect at runtime.

@oandregal oandregal self-assigned this Apr 11, 2022
@oandregal oandregal requested review from a team and gziolo and removed request for a team April 11, 2022 15:31
@oandregal oandregal added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Apr 11, 2022
@oandregal oandregal changed the title Update theme.json version from 2 to 3 Increment theme.json version to 3 Apr 11, 2022
@oandregal
Copy link
Member Author

A related task I'll address shortly is to generate the v3 docs for the block editor handbook https://developer.wordpress.org/block-editor/reference-guides/theme-json-reference/

@gziolo
Copy link
Member

gziolo commented Apr 11, 2022

@oandregal, is it necessary for the WP 6.0 Beta 1 release tomorrow?

@oandregal oandregal requested review from Mamaduka and removed request for jorgefilipecosta April 11, 2022 15:53
@oandregal oandregal requested review from jorgefilipecosta and removed request for Mamaduka April 11, 2022 15:55
@oandregal
Copy link
Member Author

@gziolo this needs to be part of the WordPress 6.0 release. If we could have it in he Beta 1 tomorrow it'll be ideal so we don't have to worry for this anymore. Everything will work fine without these changes, so if we want to punt it for after the beta 1, that'd be fine as well.

@oandregal
Copy link
Member Author

oandregal commented Apr 11, 2022

Captura de ecrã de 2022-04-11 17-59-17

When I add a new reviewer another one is removed? I presume there's some kind of limit to the number of reviewers.

The pings in this PR are mostly for awareness for people that worked in this area: when the theme.json behaviour and/or the keys change we need to create a new version. In this case (2->3) there's only additions and not incompatible changes, so everything would work fine. Though it's important for matching a given schema change to the particular WordPress release that introduced it.

@oandregal
Copy link
Member Author

I see there's an unit test failure for Block_Library_Comment_Template_Test::test_build_comment_query_vars_from_block_with_context_no_pagination, although not sure how is related to this PR.

@gziolo
Copy link
Member

gziolo commented Apr 11, 2022

I see there's an unit test failure for Block_Library_Comment_Template_Test::test_build_comment_query_vars_from_block_with_context_no_pagination, although not sure how is related to this PR.

@adamziel noticed that, too. I guess it's related to the core commit WordPress/wordpress-develop@067d166. I'm investigating whether there are any differences in the implementaion in Gutenberg.

@adamziel
Copy link
Contributor

@gziolo fixed that problem in WordPress/wordpress-develop@cab6fec

@youknowriad
Copy link
Contributor

Do we really need to update the version for non-breaking additions?

@gziolo gziolo added the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 11, 2022
@gziolo
Copy link
Member

gziolo commented Apr 11, 2022

Do we really need to update the version for non-breaking additions?

I'm looking forward to the final decision. We are going to update release/13.0 on Tuesday morning (CEST) and use it to create wp/6.0 that's going to be used for the final version of npm packages for WordPress 6.0 Beta 1.

@youknowriad
Copy link
Contributor

For block.json for instance, It evolved a lot (new keys...) since the v2 and we never needed to update the version number.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @oandregal 👍

For block.json for instance, It evolved a lot (new keys...) since the v2 and we never needed to update the version number.

I expect we will have a larger number of 3rd parties leveraging theme.json. Would that increase the need to accurately match schema changes to WP releases that introduce them?

This probably also sets a good standard for maintaining versioning around theme.json as well.


✅ Theme.json migration doc update looks fine.
✅ Unit tests pass

  • class-wp-theme-json-test.php
  • class-wp-theme-json-schema-gutenberg-test.php
  • class-gutenberg-rest-global-styles-controller-test.php

Comment on lines +80 to +82
if ( 2 === $theme_json['version'] ) {
$theme_json['version'] = 3;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate this change is trivial but for consistency with the other theme.json classes and 6.0 related changes should we override this class in the same manner? I'm also ok with leaving it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you're right, should this land, we need to fix it.

@ajlende
Copy link
Contributor

ajlende commented Apr 12, 2022

The JSON schema is loaded from the wp/x.x branch on GitHub, so once wp/6.0 is created, the new JSON schema will be available at https://schemas.wp.org/wp/6.0/theme.json. Updating the version becomes a breaking change, so theme authors will have to update that even though nothing else needs to change. So from the JSON schema perspective, I'd say that we should keep "version": 2 and not increment the value.

@oandregal
Copy link
Member Author

We don't need this for anything to work and themes don't need to be adapted either. Though, I was under the impression that we wanted a new version for each theme.json schema with changes, so we could say, for example, patterns or defaultDuotone was added in version 3, so there was clarity for theme authors.

How would we provide this clarity for theme authors if we don't do this? 🤔

The keys/sections that are not supported in a given runtime are ignored. For example, a theme.json for WordPress 5.8 needs to use settings.border.customRadius (v1); for WordPress 5.9 and above, the v1 still works but the v2 uses settings.border.radius. If someone uses the v2 names in 5.8 they'll do nothing. Same for the patterns key in WordPress 5.9: it doesn't work.

I'm struggling to see how to clarify this for theme authors if we don't have versions. An alternative is to deemphasize versions in the block editor theme.json schema docs and follow instead the model of the block API docs: sections per key. For each key, we need to document which runtime (WordPress version) supports it.

Is this any better?

@youknowriad
Copy link
Contributor

For me the apiVersion is not about additions, it's only about breaking changes. Whenever we introduce a feature that is likely to introduce breaking change, we introduce a new version that way we don't break anything for existing themes unless they opt-in (update version) for their theme.

The keys/sections that are not supported in a given runtime are ignored. For example, a theme.json for WordPress 5.8 needs to use settings.border.customRadius (v1); for WordPress 5.9 and above, the v1 still works but the v2 uses settings.border.radius. If someone uses the v2 names in 5.8 they'll do nothing. Same for the patterns key in WordPress 5.9: it doesn't work.

This is a valid use-case because it's a breaking change, if they don't change the version, the old key won't work.

Changing the apiVersion on each addition likely means that themes will have to play catchup with core and update their themes with each new version even if they don't make any changes. The alternative is them not updating the version but the new key still works (If I'm not wrong, even if my version is 2, the new keys will continue to work, so it's incoherent)

@gziolo gziolo added Needs Decision Needs a decision to be actionable or relevant and removed Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Apr 12, 2022
@oandregal
Copy link
Member Author

The alternative is them not updating the version but the new key still works (If I'm not wrong, even if my version is 2, the new keys will continue to work, so it's incoherent)

Good point. And this actually diminishes the value of using the version as documentation: v1, v2 and v3 themes could use patterns, but that setting would only work in the WordPress 6.0 runtime or above.

Let's try to address the documentation concerns in a different way: de-emphasize version and focus on documenting which setting works in a given runtime.

@oandregal oandregal closed this Apr 12, 2022
@oandregal oandregal deleted the update/theme-json-v2-to-v3 branch April 12, 2022 10:15
@oandregal oandregal removed the Needs Decision Needs a decision to be actionable or relevant label Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

6 participants