Fix incorrect usage of setup_theme action for theme supports

[felixarntz]

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 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.

[joemcgill]

This looks good to me, so am approving based on code alone. My only question is whether this hook was selected on purpose? E.g., 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?

I assume not, since all tests are still passing.

[hellofromtonya]

From what I remember (been a while), this hook was selected to align to the above hook that creates the theme features. Notice the priority order:

• 0 for creating the initial theme features
• 1 for adding the default theme supports

Here's the PR that added it #2029.

[felixarntz]

@hellofromtonya I understand that the _add_default_theme_supports() function must be called after create_initial_theme_features(), but I don't think it needs to be right after.

Looking at the PR #2029, I don't see any discussion or reasoning on why it was decided to use the setup_theme action immediately after the create_initial_theme_features() callback. Given the lack of an explanation, I assume it was simply chosen because that's what create_initial_theme_features() used, and because the _add_default_theme_supports() should fire after that.

The problem with that approach is: While the available theme features in general should be registered on setup_theme, actually adding theme supports based on the current theme needs to happen only on after_setup_theme, as outlined in the PR description.

[hellofromtonya]

While the available theme features in general should be registered on setup_theme, actually adding theme supports based on the current theme needs to happen only on after_setup_theme, as outlined in the PR description.

Why should Core itself register to 'after_theme_setup'? Shouldn't it set itself up before themes get loaded into memory?

Because without the activated theme loaded into memory, how would Core know what default supports to add? _add_default_theme_supports() checks if the activated theme is a block theme. Aha.

[felixarntz]

Why should Core itself register to 'after_theme_setup'? Shouldn't it set itself up before themes get loaded into memory?

Because before the after_setup_theme, the current theme is still being set up, including potentially overriding what the current theme is, as the Customizer does. So only after the setup_theme action (i.e. during the after_setup_theme action) we truly know what the current theme is.

It's also worth noting that all features that _add_default_theme_supports() adds are not block theme specific. These features are added in classic themes as well, including all classic default themes of the past 5+ years. The themes do it on after_setup_theme, so core should do it too. The reason that core even handles it for block themes is to avoid the need for block theme developers to implement a functions.php file just for these default features, which improves the developer experience.

[hellofromtonya]

It's also worth noting that all features that _add_default_theme_supports() adds are not block theme specific.

For clarity, I was referring to:

function _add_default_theme_supports() {
	if ( ! wp_is_block_theme() ) {
		return;
	}

where the function bails out if the theme is not a block theme. More specifically, I was mentally wrestling with: what is the state of that check when hooked into setup_theme?

Was it working as intended?

Backtracing the logic, yes, it does work, because wp_get_theme() can be populated by the activated theme in the 'stylesheet' option. Thus, the theme did not need to be set up for this to work.

That part of the logic working though is not necessarily relevant to the bug being reported.

[felixarntz]

Let me clarify with an example of how this being hooked into setup_theme does not work as expected:

1. Your site uses a block theme (e.g. Twenty Twenty-Three).
2. On Appearance > Themes, you click "Live Preview" for a classic theme, e.g. Twenty Twenty-One.
3. In the live preview, Twenty Twenty-One will now have all the "theme supports" from _add_default_theme_supports(), which is a bug, as those theme supports should only be added when the theme is a block theme. Now the theme features added based on the assumption that the current theme is a block theme are present, even though the previewed theme is a classic theme.

The reason for that is that at the setup_theme action, WordPress still "thinks" the current theme is a block theme and thus adds the theme supports. The Customizer however overrides the current theme for the preview to be the classic theme only after that, and this logic has been present in WordPress core for many many years, far longer than when this bug was introduced. Therefore the after_setup_theme action must be used for this.

The problem with the existing code is not that there is no theme set up. The problem is that the incorrect theme is set up at that point when considering the live preview.

[felixarntz]

To my previous points, please also see https://developer.wordpress.org/reference/hooks/after_setup_theme/:

[This action] is generally used to perform basic setup, registration, and init actions for a theme.

add_theme_support() is part of this basic setup, and should not be used on setup_theme. Themes are not even able to hook into setup_theme as it runs before the current theme files are being loaded.

Also https://developer.wordpress.org/reference/hooks/setup_theme/:

Fires before the theme is loaded.

Note the "before". This action must not be used to initialize specific logic that is part of the theme.

[hellofromtonya]

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.

[felixarntz]

@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.

[felixarntz]

@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.

[hellofromtonya]

Confirmed this patch does fix the reported bug as live theme previews are restored in Customer.

Also confirmed, the patch does not change the behavior for block themes.

See my testing report in the Trac ticket.

LGTM 👍 Ready for commit.

[felixarntz]

Committed in https://core.trac.wordpress.org/changeset/57009