Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#59732 closed defect (bug) (fixed)

Theme live preview is broken

Reported by: karl94's profile karl94 Owned by: flixos90's profile flixos90
Milestone: 6.4 Priority: normal
Severity: normal Version: 5.8
Component: Themes Keywords: has-patch has-testing-info dev-reviewed commit
Focuses: Cc:

Description

The newly added memoization in 6.4 for get_stylesheet_directory() and get_template_directory() breaks themes live preview.

https://github.com/WordPress/wordpress-develop/commit/ec21b604e0527cff54b774adcb9631036d6b57ec#diff-b9f1810ad43acfa11ba58a9f21eb0ee3a8063c80aa155de9b210323252534716R190-R223

How to reproduce:

Attachments (4)

WP-6.4 get_stylesheet_directory issue.png (350.6 KB) - added by karl94 7 months ago.
test-report-theme-live-preview.png (527.4 KB) - added by hellofromTonya 7 months ago.
Test Report: After applying PR 5574 - resolves the bug ✅
test-report-block-theme-in-customizer.png (1.1 MB) - added by hellofromTonya 7 months ago.
Test Report: After applying PR 5574, with block theme TT4 activated, Menus does not show and live preview (for classic theme) still works ✅
test-report-6.3.2v6.4-rc2.png (415.8 KB) - added by hellofromTonya 7 months ago.
Test Report: Bug before the patch: Live Preview in WP 6.3.2 (left) ✅ vs 6.4 RC (right) ❌

Change History (22)

#1 @karl94
7 months ago

@flixos90 can you give a look at this?

It appears that WP is invoking get_stylesheet_directory() before that WP_Customize_Manager attaches its filters for previewing the theme.

WP_Customize_Manager adds its preview filters in WP_Customize_Manager::start_previewing_theme(), which is called from WP_Customize_Manager::setup_theme(), which is run on action setup_theme at default priority (10).

The first invocation of get_stylesheet_directory() appears to be from wp_theme_has_theme_json() (https://github.com/WordPress/wordpress-develop/blob/9352b2a/src/wp-includes/global-styles-and-settings.php#L399) which is called by wp_enable_block_templates() (https://github.com/WordPress/wordpress-develop/blob/9352b2a/src/wp-includes/theme-templates.php#L227) yet again during the hook setup_theme at priority 10 (https://github.com/WordPress/wordpress-develop/blob/9352b2a/src/wp-includes/default-filters.php#L721).

So, both wp_enable_block_templates() and WP_Customize_Manager::setup_theme() are attached to the same hook at the same priority, therefore they get invoked in the same order they were attached to the hook.

default-filters.php is executed before, then the callback order is: first wp_enable_block_templates(), then WP_Customize_Manager::setup_theme().

Because get_stylesheet_directory() is used inside wp_enable_block_templates(), the global variable $wp_stylesheet_path is set, caching $stylesheet_dir. Any following get_stylesheet_directory() call will always return the same value.

The cache-bypass logic doesn't help in this case since when get_stylesheet_directory() is first invoked (via wp_enable_block_templates()) the customizer hasn't added its filter yet.

get_template_directory() has the same issue.

Maybe a doing_it_wrong() should be added to both get_stylesheet_directory() and get_template_directory() to warn of every usage before or during the hook setup_theme? And bypass their cache until that hook completes?

#2 @flixos90
7 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.4
  • Owner set to flixos90
  • Status changed from new to assigned
  • Version changed from trunk to 5.8

Thank you for the report @karl94! I have investigated the problem and identified the root cause: There are two default filters (_add_default_theme_supports(), and wp_enable_block_templates() as you've mentioned), which are hooked into the setup_theme action to add_theme_support() for certain features.

This is incorrect, as theme support should only be added on the after_setup_theme hook. Using the setup_theme hook does not only introduce the above bug, but leads to other problems as well as the theme can only be expected to be completely set up after the setup_theme action. For example, at the moment these callbacks will add theme support based on the current theme which then would still be maintained even when previewing another theme in the Customizer.

It looks like these problems were introduced in [51199] and [52369] respectively, therefore pinging @jorgefilipecosta and @audrasjb for visibility.

This should be relatively straightforward to fix by relying on the appropriate action after_setup_theme instead. There is no relevant usage of these functions in the plugin repository which would be affected by this change, see https://wpdirectory.net/search/01HDKY6F7BJ58ZD7H6JQ8K4P3S and https://wpdirectory.net/search/01HDKY998M6WV6TRE2WR3ZCYG6.

This ticket was mentioned in PR #5574 on WordPress/wordpress-develop by @flixos90.


7 months ago
#3

  • Keywords has-patch added; needs-patch removed

This fixes the bug of using the setup_theme action for adding theme support, which is incorrect for such purposes, as at that point the current theme is not set up yet. For example, Customizer previewing, which may exchange the current theme at runtime only fires on setup_theme.

Therefore any code depending on the current theme must only run after that, i.e. on after_setup_theme. This is also the hook which theme authors have been encouraged for more than a decade to use to add theme supports.

See https://core.trac.wordpress.org/ticket/59732#comment:2 for additional context.

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

This ticket was mentioned in PR #5574 on WordPress/wordpress-develop by @flixos90.


7 months ago
#4

This fixes the bug of using the setup_theme action for adding theme support, which is incorrect for such purposes, as at that point the current theme is not set up yet. For example, Customizer previewing, which may exchange the current theme at runtime only fires on setup_theme.

Therefore any code depending on the current theme must only run after that, i.e. on after_setup_theme. This is also the hook which theme authors have been encouraged for more than a decade to use to add theme supports.

See https://core.trac.wordpress.org/ticket/59732#comment:2 for additional context.

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

This ticket was mentioned in Slack in #core by flixos90. View the logs.


7 months ago

#6 @flixos90
7 months ago

I have implemented https://github.com/WordPress/wordpress-develop/pull/5574 as a fix for this bug. It addresses the root problem and through that the concrete broken user experience reported: With the PR applied, the Customizer-previewed theme's data is correctly loaded, without any data from the original theme bleeding over.

@hellofromTonya commented on PR #5574:


7 months ago
#7

I share the concerns @joemcgill expressed:

could there be some unintentional side effects where some block data or settings get instantiated between setup_theme and after_setup_theme and assume that theme support has already been established?

While it fixes Customizer theme live preview, what might it do to block powered sites? Is something dependent upon on it? Not sure, but should be validated with careful testing within the editors and front-end.

@flixos90 commented on PR #5574:


7 months ago
#8

@hellofromtonya

While it fixes Customizer theme live preview, what might it do to block powered sites? Is something dependent upon on it? Not sure, but should be validated with careful testing within the editors and front-end.

I have checked the WordPress core codebase for further usage of the setup_theme action, and I couldn't find any relevant instances where delaying the execution of _add_default_theme_supports() and wp_enable_block_templates() could lead to a problem. In fact the hook is only used to register the available theme features and for the Customizer preview.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 months ago

@flixos90 commented on PR #5574:


7 months ago
#10

@hellofromtonya I've tested this PR using the Site Editor for a bit, with both Twenty Twenty-Three and Twenty Twenty-Four, and couldn't find any problems. It also fixes the original Customizer bug. I personally think this change is safe to make, though of course any additional testing would be appreciated.

#11 @hellofromTonya
7 months ago

  • Keywords needs-testing has-testing-info added

Patch: https://github.com/WordPress/wordpress-develop/pull/5574

The patch changes the "when" (to after the theme is set up) for:

  • adding block theme's theme supports (including 'block-templates'),
  • turning on 'should_load_separate_core_block_assets',
  • and removing Customizer's Menu panel.

Testing instructions need to test for:

  • the bug reported (i.e. Customizer themes live preview)
  • and the above changes for block themes, i.e. to ensure the patch does not change the behavior.

Test Instructions: Customizer themes live preview bug

Steps to Reproduce

Though listed in the ticket's description, repeating here to keep all testing instructions in one place:

  1. Load WordPress 6.4 RC2
  2. Install some classic themes e.g. twentytwenty and twentynineteen
  3. Activate twentynineteen
  4. Live preview twentytwenty 🐞 Bug occurs.

Expected Results

When reproducing a bug:

  • ❌ Before applying the patch, the live preview should not work.

When testing a patch to validate it works as expected:

  • ✅ After applying the patch, the live preview should work.

Test Instructions: Block themes not affected by the patch

Though listed in the ticket's description, repeating here to keep all testing instructions in one place:

  1. Load WordPress 6.4 RC2.
  2. With a classic theme, such as Twenty Twenty-One (TT1) activated, open Customizer in a different browser tab or ease of access in the next steps.
    • Notice: The "Menus" item in the navigation sidebar.
  3. Activate Twenty Twenty-Four (TT4).
  4. Refresh Customizer. Expected: The "Menus" item should not be in the navigation sidebar. ✅
  5. Apply the patch.
  6. Repeat step 4. Expected: No "Menus" item ✅
  7. Navigate to Posts > and open a post.
    • Expected: "Featured image" subpanel should render in the right sidebar and be functional. ✅

Expected Results

After applying the patch, the following should still work for block themes:

  • ✅ In Customizer, the "Menus" item should not in the navigation sidebar.
  • ✅ When adding or editing a post, the "Featured image" subpanel should render and be functional.

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

@hellofromTonya
7 months ago

Test Report: After applying PR 5574 - resolves the bug ✅

@hellofromTonya
7 months ago

Test Report: After applying PR 5574, with block theme TT4 activated, Menus does not show and live preview (for classic theme) still works ✅

#12 @hellofromTonya
7 months ago

  • Keywords needs-testing removed

Environment

  • OS: macOS
  • Localhost: wp-env Docker
  • PHP: 7.4
  • WordPress: trunk
  • Browser: Firefox
  • Theme: TT1 and TT0 (classic themes) and TT4 (block theme)
  • Active Plugins: none

Actual Results: For Live Theme Preview bug

✅ After applying the patch, the live preview works.

See test-report-theme-live-preview.png

Actual Results: For Block Theme

✅ In Customizer, the "Menus" item does not show in the navigation sidebar.
✅ In Customizer, selecting TT0, live preview works.

See test-report-block-theme-in-customizer.png

✅ When adding or editing a post, the "Featured image" subpanel still renders and is functional.

#13 @hellofromTonya
7 months ago

  • Keywords dev-reviewed commit added

Patch: https://github.com/WordPress/wordpress-develop/pull/5574

With multiple testing reports and the patch being approved by 2 committers, the patch is ready for commit.

To expedite, the patch is also okay to backport to the 6.4 branch.

@hellofromTonya
7 months ago

Test Report: Bug before the patch: Live Preview in WP 6.3.2 (left) ✅ vs 6.4 RC (right) ❌

#14 @hellofromTonya
7 months ago

Additional information why this regression should be included in 6.4 RC4:

Without the patch applied (i.e. before the patch), test-report-6.3.2v6.4-rc2.png shows theme live preview:

  • Works with WordPress 6.3.2 ✅
  • Broken with WordPress 6.4. RC2 ❌

With the live theme preview experience broken, a fix is indeed needed. As previously noted, the patch resolves the issue without impacting block themes.

#15 @flixos90
7 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57009:

Themes: Fix block theme supports being added too early, leading to Customizer live preview bugs in 6.4.

The Customizer live preview broke because of [56635], however the root cause for the bug was a lower-level problem that had been present since WordPress 5.8: The block theme specific functions _add_default_theme_supports() and wp_enable_block_templates() were being hooked into the setup_theme action, which fires too early to initialize theme features. Because of that, theme functionality would be initialized before the current theme setup being completed. In the case of the Customizer, that includes overriding which theme is the current theme entirely, thus leading to an inconsistent experience.

This changeset fixes the bug by moving those two callbacks to the after_setup_theme action, which is the appropriate action to initialize theme features.

Props karl94, hellofromTonya, joemcgill, flixos90.
Fixes #59732.
See #18298, #53397, #54597.

#17 @flixos90
7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#18 @flixos90
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57010:

Themes: Fix block theme supports being added too early, leading to Customizer live preview bugs in 6.4.

The Customizer live preview broke because of [56635], however the root cause for the bug was a lower-level problem that had been present since WordPress 5.8: The block theme specific functions _add_default_theme_supports() and wp_enable_block_templates() were being hooked into the setup_theme action, which fires too early to initialize theme features. Because of that, theme functionality would be initialized before the current theme setup being completed. In the case of the Customizer, that includes overriding which theme is the current theme entirely, thus leading to an inconsistent experience.

This changeset fixes the bug by moving those two callbacks to the after_setup_theme action, which is the appropriate action to initialize theme features.

Props karl94, hellofromTonya, joemcgill, flixos90.
Merges [57009] to the 6.4 branch.
Fixes #59732.
See #18298, #53397, #54597.

Note: See TracTickets for help on using tickets.