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

Clean theme data when switching themes in the customizer #34540

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Sep 3, 2021

Fixes #34531
Related #34704
Related https://meta.trac.wordpress.org/ticket/5818#comment:4

When we read the theme.json from the theme, we cache its data and only clean it upon certain actions, such as switch_theme. In the customizer, the user can dynamically switch themes, which appears not to use the switch_theme. We still have the setup_theme and specific ones such as start_previewing_theme.

@oandregal oandregal self-assigned this Sep 3, 2021
@oandregal oandregal added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Sep 3, 2021
@@ -531,3 +531,4 @@ public static function clean_cached_data() {
}

add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'setup_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if it'd be enough to only hook to setup_theme. In other words: is there any situation in which the switch_theme is fired but the setup_theme isn't?

The current fix works, though, and the worst-case scenario is that we read the data from the sources twice for a theme.

Copy link
Contributor

@ntsekouras ntsekouras Sep 7, 2021

Choose a reason for hiding this comment

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

I believe setup_theme alone could be enough. switch_theme is triggered when we deactivate a theme, which is not the case for the previews.

Maybe @aristath, @carolinan or @swissspidy could confirm? 😄

Copy link
Member

Choose a reason for hiding this comment

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

setup_theme is fired before the current theme is loaded when accessing WordPress (happens late in wp-settings.php). This only happens once per request. This is similar to the plugins_loaded hook.

switch_theme() (which fires the switch_theme action) can be called later on and potentially multiple times during a request, for example when publishing customizer changesets, upgrading the theme, or of course when switching themes.

For this reason, it's definitely needed to hook into switch_theme

Not super familiar with the Customizer, but WP_Customize_Manager hooks into setup_theme to preview different themes when accessing WordPress.

That makes sense, since the Customizer loads the WP site in an iframe and reloads that page when changing themes. So each time it's a new request, firing setup_theme anew, then kicking off the customizer logic.

Hooking into setup_theme to clean cached data seems a bit odd to do this.

In a regular request, there's no data to clear yet, and for customizer requests it just so happens that your code runs before the customizer.

It would be better to hook into the customizer logic itself.

tl;dr:

  1. Hook into switch_theme
  2. Hook into start_previewing_theme

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 for the explanation, Pascal! I've implemented what you suggested.

I agree using setup_theme is a bit odd (cleaning the cache before the theme data is even used, in most cases). However, I wonder if that's the hook that's most relevant to what we want to do here, instead of hooking into flow-specific ones. There may be flows that are not covered yet. I'll keep an eye on 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.

I've run into a meta trac ticket that may be related to this https://meta.trac.wordpress.org/ticket/5818#comment:4 (previews for themes with theme.json not working consistently in the theme directory).

@oandregal oandregal force-pushed the fix/customizer-theme-switching branch from 7a20d3a to 987a4a8 Compare September 7, 2021 08:13
@oandregal
Copy link
Member Author

Related PR at #34704

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

This works well! Thanks @oandregal and @swissspidy 💯

Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

I was able to reproduce the problem and the PR solves it for me.

@ntsekouras ntsekouras merged commit 4fc4a03 into trunk Sep 14, 2021
@ntsekouras ntsekouras deleted the fix/customizer-theme-switching branch September 14, 2021 13:03
@github-actions github-actions bot added this to the Gutenberg 11.6 milestone Sep 14, 2021
fullofcaffeine added a commit that referenced this pull request Sep 16, 2021
* trunk: (74 commits)
  Update docs for ClipboardButton component (#34711)
  Post Title Block: add typography formatting options (#31623)
  Bump plugin version to 11.5.0
  Navigation Screen: Adjust header toolbar icon styles (#34833)
  Fix the parent menu item field in REST API responses (#34835)
  Rewrite FocusableIframe as hook API (#26753)
  Create Block: Remove wp-cli callout since not recommended and outdated (#34821)
  Global Styles: Fix dimensions panel default controls display (#34828)
  [RNMobile][Embed block] Enable embed preview for Instagram and Vimeo providers (#34563)
  Increase Link UI search results to 10 on Nav Editor screen (#34808)
  Prevent welcome guide overflow x scroll (#34713)
  Enable open on click for Page List inside Navigation. (#34675)
  [RNMobile] [Embed block] -  Unavailable preview fallback bottom sheet title update  (#34674)
  Add missing field _invalid in menu item REST API (#34670)
  Fix Dropdown/DropdownMenu toggle closing in all UAs (#31170)
  Navigation submenu block: replace global shortcut event handlers with local ones (#34812)
  Navigation Screen: Consolidate menu name and switcher (#34786)
  Remove parent and position validation from menu item REST API endpoint (#34672)
  Clean theme data when switching themes in the customizer (#34540)
  Components: add reset timeout to ColorPicker's copy functionality. (#34601)
  ...
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 [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

theme.json styles are not loaded when previewing a FSE theme in the Customizer
5 participants