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

Consolidate WP_Theme_JSON_Resolver changes in both lib/compat/wordpress-6.0 and lib/experimental #40140

Merged
merged 4 commits into from
Apr 7, 2022

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Apr 7, 2022

What?

This PR does two things:

  • makes sure the lib/compat/6.0/wp-class-theme-json-resolver-6.0.php has all the changes to be backported.
  • makes sure the lib/experimental/wp-class-theme-json-resolver-gutenberg.php has all the changes to be backported and the experimental ones.

Why?

By the process we have, we add changes to lib/compat/6.0 when they are supposed to land in WordPress 6.0 and we add them to lib/experimental when they need more iteration and should not be backported. This is problematic when the same method contains both experimental changes and changes to be backported.

This is the case of WP_Theme_JSON_Resolver_*::get_theme_data method, that has changes to be backported and others that should not.

How?

The get_theme_data method in experimental is the one that will run, so it needs to have all the changes necessary to work well.

On the other hand, we also need all those changes (but the experimental ones) in the get_theme_data method of lib/compat/6.0, as to make sure they're backported. Though this method is not called.

Alternatives:

  1. Continue the separation. Identifying changes to be backported requires looking at lib/experimental, git history and understand what everything does. Every change to be backported for a method that needs experimental changes needs be ported at experimental.
  2. Add a hook _lib_experimental_changes_description that code at lib/experimental could use. We'll still need to mark the hook as not to port, so I thought why not doing it directly given it's a oneliner.
  3. Merge both classes into a single one (living in lib/compat for example) and mark the changes not to backport via comments, as in:
/* NOT TO MERGE IN CORE (START) */
changes_go_here();
/* NOT TO MERGE IN CORE (END) */

Future

This PR tries to work with the model we have now to make sure everything is contained (we know what to backport) and it works (the experimental method has all the changes).

The existing approach has some pros:

  • we still have the experimental changes marked in lib/experimental

It also has some cons

  • requires porting every change to this method to both the lib/experimental and lib/compat/wordpress-6.0

For now, given how close we're to the WordPress 6.0 cutoff, it seems easier to run with what we have. For the 6.1 cycle this may require further thinking and/or changing the approach to avoid changes not being backported.

@oandregal oandregal self-assigned this Apr 7, 2022
@oandregal oandregal changed the title Merge 6.0 and Experimental resolvers Consolidate 6.0 and experimental changes into a single resolver Apr 7, 2022
@oandregal
Copy link
Member Author

To be clear: I think we need to keep the separation between lib/experimental and lib/compat. Just in this specific case we can't easily.

@oandregal oandregal changed the title Consolidate 6.0 and experimental changes into a single resolver Consolidate 6.0 and experimental changes Apr 7, 2022
@oandregal oandregal changed the title Consolidate 6.0 and experimental changes Consolidate WP_Theme_JSON_Resolver changes in both lib/compat/wordpress-6.0 and lib/experimental Apr 7, 2022
@oandregal
Copy link
Member Author

Initially, I thought merging both resolvers was easier. But it also has cons: we no longer have the experimental changes in lib/experimental but we still need them for the 6.1 cycle. I've modified the direction of this PR to just have both methods to contain all the feature changes to be backported but only the experimental method has the experimental changes.

@oandregal oandregal added the [Type] Bug An existing feature does not function as intended label Apr 7, 2022
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@oandregal oandregal merged commit 508037f into trunk Apr 7, 2022
@oandregal oandregal deleted the update/theme-json-resolvers branch April 7, 2022 12:42
@github-actions github-actions bot added this to the Gutenberg 13.0 milestone Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants