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

PHP: Backport changes from core theme resolver. #46250

Merged
merged 7 commits into from
Dec 6, 2022

Conversation

spacedmonkey
Copy link
Member

What?

Backport changes from WordPress/wordpress-develop@8368eef by @felixarntz

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @spacedmonkey!

@@ -34,11 +34,15 @@ public static function get_theme_data( $deprecated = array(), $settings = array(
}

// When backporting to core, remove the instanceof Gutenberg class check, as it is only required for the Gutenberg plugin.
if ( null === static::$theme || ! static::$theme instanceof WP_Theme_JSON_Gutenberg ) {
if ( null === static::$theme || ! static::has_same_registered_blocks( 'theme' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why there is the check for static::$theme instanceof WP_Theme_JSON_Gutenberg, though we should aim to understand that before removing it. What was the intention behind it? Is it safe to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is this way in core. This just a back port.

Copy link
Member

Choose a reason for hiding this comment

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

@spacedmonkey I've looked up git history and that check has been added in this PR #42756

As my understanding goes, without that check, there's an issue with some data not being recalculated in the plugin, so this PR has introduced a regression.

I am working on something that will fix the root issue (re: reorganizing theem.json code in the plugin), though, in the meantime, this is just such a small change that is worth adding it back until that lands.

@spacedmonkey spacedmonkey merged commit d6aaef0 into trunk Dec 6, 2022
@spacedmonkey spacedmonkey deleted the feature/backport-json branch December 6, 2022 16:17
@github-actions github-actions bot added this to the Gutenberg 14.8 milestone Dec 6, 2022
@mirka
Copy link
Member

mirka commented Dec 7, 2022

Hi 👋 This may be a stupid question, but my wp-env is broken after this commit. Is there something I need to do to make it work?

For example, when I run npm run wp-env start -- --update on this commit, it fails with this error:

error
[07-Dec-2022 12:52:51 UTC] PHP Fatal error:  Uncaught Error: Call to undefined method WP_Theme_JSON_Resolver_Gutenberg::has_same_registered_blocks() in /var/www/html/wp-content/plugins/gutenberg/lib/experimental/class-wp-theme-json-resolver-gutenberg.php:37
Stack trace:
#0 /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php(161): WP_Theme_JSON_Resolver_Gutenberg::get_theme_data()
#1 /var/www/html/wp-content/plugins/gutenberg/lib/experimental/register-webfonts-from-theme-json.php(13): WP_Theme_JSON_Resolver_6_2::get_merged_data()
#2 /var/www/html/wp-includes/class-wp-hook.php(307): gutenberg_register_webfonts_from_theme_json('')
#3 /var/www/html/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters(NULL, Array)
#4 /var/www/html/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#5 /var/www/html/wp-settings.php(598): do_action('init')
#6 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1336): require('/var/www/html/w...')
#7 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1254): WP_CLI\Runner->load_wordpress()
#8 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start()
#9 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/bootstrap.php(78): WP_CLI\Bootstrap\LaunchRunner->process(Object(WP_CLI\Bootstrap\BootstrapState))
#10 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php(32): WP_CLI\bootstrap()
#11 phar:///usr/local/bin/wp/php/boot-phar.php(11): include('phar:///usr/loc...')
#12 /usr/local/bin/wp(4): include('phar:///usr/loc...')
#13 {main}
  thrown in /var/www/html/wp-content/plugins/gutenberg/lib/experimental/class-wp-theme-json-resolver-gutenberg.php on line 37
Fatal error: Uncaught Error: Call to undefined method WP_Theme_JSON_Resolver_Gutenberg::has_same_registered_blocks() in /var/www/html/wp-content/plugins/gutenberg/lib/experimental/class-wp-theme-json-resolver-gutenberg.php:37
Stack trace:
#0 /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php(161): WP_Theme_JSON_Resolver_Gutenberg::get_theme_data()
#1 /var/www/html/wp-content/plugins/gutenberg/lib/experimental/register-webfonts-from-theme-json.php(13): WP_Theme_JSON_Resolver_6_2::get_merged_data()
#2 /var/www/html/wp-includes/class-wp-hook.php(307): gutenberg_register_webfonts_from_theme_json('')
#3 /var/www/html/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters(NULL, Array)
#4 /var/www/html/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#5 /var/www/html/wp-settings.php(598): do_action('init')
#6 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1336): require('/var/www/html/w...')
#7 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1254): WP_CLI\Runner->load_wordpress()
#8 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start()
#9 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/bootstrap.php(78): WP_CLI\Bootstrap\LaunchRunner->process(Object(WP_CLI\Bootstrap\BootstrapState))
#10 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php(32): WP_CLI\bootstrap()
#11 phar:///usr/local/bin/wp/php/boot-phar.php(11): include('phar:///usr/loc...')
#12 /usr/local/bin/wp(4): include('phar:///usr/loc...')
#13 {main}
  thrown in /var/www/html/wp-content/plugins/gutenberg/lib/experimental/class-wp-theme-json-resolver-gutenberg.php on line 37
Error: There has been a critical error on this website.Learn more about troubleshooting WordPress. There has been a critical error on this website.

With the previous commit, it works fine.

@mirka
Copy link
Member

mirka commented Dec 7, 2022

Never mind, a complete wipe fixed it (npm run wp-env destroy && npm run wp-env start). Maybe it was just my env!

@spacedmonkey
Copy link
Member Author

Hi 👋 This may be a stupid question, but my wp-env is broken after this commit. Is there something I need to do to make it work?

For example, when I run npm run wp-env start -- --update on this commit, it fails with this error:

error
With the previous commit, it works fine.

There is a fix here - #46363 CC @mirka

mpkelly pushed a commit to mpkelly/gutenberg that referenced this pull request Dec 7, 2022
* Backport changes from core.

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Add line back.

* Fix lint
@noahtallen
Copy link
Member

Never mind, a complete wipe fixed it (npm run wp-env destroy && npm run wp-env start). Maybe it was just my env!

Btw, I think this fixed it because it would have updated the WordPress source code, which introduces the missing properties that caused the crash! :)

You can also run npx wp-env start --update to do that -- just another debug step that can be a bit faster than starting from scratch!

@mirka
Copy link
Member

mirka commented Dec 13, 2022

Btw, I think this fixed it because it would have updated the WordPress source code, which introduces the missing properties that caused the crash! :)

You can also run npx wp-env start --update to do that -- just another debug step that can be a bit faster than starting from scratch!

I have to think there was something else involved because the --update flag wasn't fixing it. Given that nobody has commented here with the same problem, maybe there was something going on with my local files, I have no idea 🤷

@noahtallen
Copy link
Member

I just had the exact same error myself trying to run trunk on WordPress 6.0 😅 I ended up switching to the last plugin release branch, because I specifically needed to test it on the older WP version 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants