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

Add backward compat shim for WP_Theme_JSON_Data::get_theme_json #6626

Closed

Conversation

joemcgill
Copy link
Member

@joemcgill joemcgill commented May 24, 2024

This checks for cases where extenders are returning an object other than WP_Theme_JSON_Data from filters and regenerates the WP_Theme_JSON object if so to avoid issues like this one or #6271 (comment).

Trac ticket: https://core.trac.wordpress.org/ticket/61112


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

github-actions bot commented May 24, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props joemcgill, adamsilverstein, oandregal, ryelle, ocean90, pbearne.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@joemcgill joemcgill requested a review from ryelle May 24, 2024 15:31
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@joemcgill
Copy link
Member Author

Updated the PR to address merge conflicts

@joemcgill joemcgill self-assigned this Jun 4, 2024
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

nicely solved

@@ -172,8 +172,18 @@ public static function get_core_data() {
*
* @param WP_Theme_JSON_Data $theme_json Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_default', new WP_Theme_JSON_Data( $config, 'default' ) );
static::$core = $theme_json->get_theme_json();
$theme_json = apply_filters( 'wp_theme_json_data_default', new WP_Theme_JSON_Data( $config, 'default' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Hi, is anyone able to reproduce this error?

I cannot repro the issue reported using latest WordPress trunk and Gutenberg 14.8.1 (the last release prior to updating WP_Theme_JSON_Data_Gutenberg). I'd like to understand this better before jumping into implementation.

The only way I can think to reproduce the issue is by not using the provided structure to update the data as a consumer of this filter. This is, instead of doing:

add_filter( 'wp_theme_json_data_*', function( $theme) {
  $new_data = array( /* ... */ );

  return $theme->update_with( $new_data );
});

Your plugin does something like:

add_filter( 'wp_theme_json_data_*', function( $theme) {
  /* your plugin code */
  return  $your_custom_structure;
});

Note that this is not supported or recommended. The update_with method is a requirement to use these filters, as stated in the devnote, because it makes sure everything works correctly (data migration, sanitization, etc.). This is what happened to @ryelle, as she reported.

Is there any other use case?

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I'm not against doing it, but I'm reluctant to given that it only happens if a plugin provides their own structure, which is not supported or recommended.

Copy link
Contributor

@ryelle ryelle Jun 5, 2024

Choose a reason for hiding this comment

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

The issue can be reproduced using the Blocks Everywhere plugin, which does the following, basically:

add_filter( 'wp_theme_json_data_theme', function( $theme) {
	$theme = new \WP_Theme_JSON_Data_Gutenberg( /* ... */ );
	return $theme;
} );

(full code here)

Which I suppose is incorrect according to the dev note, but it did work before the recent change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oandregal I believe this was only reproducible prior to WP_Theme_JSON_Data_Gutenberg::get_theme_json being added in version 18.3.0, which shipped after the core change. If we don't need to worry about WP 6.6 compat with any Gutenberg versions prior to 18.3.0, then we can probably close this issue as wontfix.

However, I think this use case is instructive for us as we consider better ways of managing the WP_Theme_JSON_ classes between the GB repo and the WP-dev repo to avoid these types of conflicts.

@joemcgill
Copy link
Member Author

I've updated the approach here, based on conversation in this thread to ensure that when an object returned from these filters that are not WP_Theme_JSON_Data objects have their data revalidated as WP_Theme_JSON objects to avoid potential compatibility conflicts.

@joemcgill joemcgill requested a review from oandregal June 7, 2024 19:37
@pbearne
Copy link

pbearne commented Jun 18, 2024

This looks good :-)

* Backward compatibility for extenders returning a WP_Theme_JSON_Data
* compatible class that is not a WP_Theme_JSON_Data object.
*/
if ( is_a( $theme_json, 'WP_Theme_JSON_Data' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This should use the instanceof keyword, see https://core.trac.wordpress.org/changeset/56352.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I had a sense that we preferred instanceof but couldn't remember the context. Do we not have a sniff for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 544d762

@oandregal
Copy link
Member

I reviewed the new changes and checking for WP_Theme_JSON_Data is a good approach. In addition with concerns raised by how Gutenberg extends core, this may help with other cases as well: example, example.

@joemcgill
Copy link
Member Author

Committed in 58443.

@joemcgill joemcgill closed this Jun 19, 2024
@joemcgill joemcgill deleted the fix/61112-wp-theme-json-data-compat branch October 11, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 😃
Development

Successfully merging this pull request may close these issues.

6 participants