Add a check for wp_get_current_user to current_user_can

[scruffian]

This issue was raised in ​https://github.com/WordPress/gutenberg/issues/53284, but I think its exposing an issue with WordPress more generally.

It's possible to call current_user_can before wp_get_current_user has been defined, but we don't have any checks for wp_get_current_user inside current_user_can. I think we should check whether the function exists before we attempt to call it to avoid fatal errors that are possible as shown by ​https://github.com/WordPress/gutenberg/issues/53284.

I have a potential fix here: ​https://github.com/WordPress/wordpress-develop/pull/4959, but I'm open to other suggestions.

This is an attempt to fix ​https://github.com/WordPress/gutenberg/issues/53284.

The problem is that it's possible to call current_user_can before wp_get_current_user has been defined. We could guard against this by checking whether wp_get_current_user is defined before calling current_user_can each time, but adding the check here seems simpler.

I'm not sure if it makes sense to return false in this case, maybe we should return an error instead?

Fixes https://core.trac.wordpress.org/ticket/59000

[johnbillion]

I'd be very wary of changing this. If user capability checks are being performed before pluggable functions are defined then they are unreliable. Changing the function to return false if wp_get_current_user() isn't defined just papers over the incorrect usage.

What's the reason the capability checks are being called before pluggable functions are defined?

[scruffian]

This comes up because we have added a filter to get_stylesheet, which checks current_user_can. get_stylesheet can be called before pluggable functions are defined.

[SergeyBiryukov]

Replying to johnbillion:

Changing the function to return false if wp_get_current_user() isn't defined just papers over the incorrect usage.

Same thoughts here, it seems like this check should perhaps be in wp_get_theme_preview_path() instead.

Intended to fix https://core.trac.wordpress.org/ticket/59000

This PR fixes ​https://github.com/WordPress/gutenberg/issues/53284. ​https://github.com/WordPress/wordpress-develop/pull/4959 tried to fix the same issue, but there is a concern about its reliability, depending on timing.

Hence, this PR changed to load theme-previews.php, which is the cause of this issue, after the pluggable functions. The reason for this is that running capability checks before defining pluggable functions itself does not seem to be a good approach.

Fixes ​https://github.com/WordPress/gutenberg/issues/53284
Trac ticket: https://core.trac.wordpress.org/ticket/59000

This PR fixes ​https://github.com/WordPress/gutenberg/issues/53284. ​https://github.com/WordPress/wordpress-develop/pull/4959 tried to fix the same issue, but there is a concern about its reliability, depending on timing.

Hence, this PR changed to load theme-previews.php, which is the cause of this issue, after the pluggable functions. The reason for this is that running capability checks before defining pluggable functions itself does not seem to be a good approach.

Fixes ​https://github.com/WordPress/gutenberg/issues/53284
Trac ticket: https://core.trac.wordpress.org/ticket/59000

Hello, @SergeyBiryukov @johnbillion!
I've made the changes as described in the PR to make the block theme previews work on every site. I'd appreciate it if you take a look!

Both failing tests (​E2E, ​performance) are passing on my local, at least.

This makes perfect sense to me, thanks for working on it.

Closing in favour of ​https://github.com/WordPress/wordpress-develop/pull/5028

Hello, @azaozz @mtias @noisysocks!
To fix the crash of Block Theme Previews, I'd appreciate it if you could review and commit this change to the core. Thank you in advance!

[azaozz]

At first look ​https://github.com/WordPress/wordpress-develop/pull/5028 makes sense: some functions defined in theme-previews.php call functions that are defined later, hence the fatal error.

Also there is some "loose" code in theme-previews.php that should probably not be there. Adding of actions and filters should be in default-filters.php unless there is a need for a (well documented) exception.

Looking at the ​original issue, it seems better to warn plugin authors that methods like $wp_theme->get_stylesheet() cannot be used before the plugins_loaded action (which runs immediately after all plugins and pluggable.php are loaded). Then add a "doing it wrong" in these methods similarly to the way in script-loader.php (i.e. check if the plugins_loaded action has run and call doing_it_wrong if not). This will ensure that the stylesheet, template, etc. filters do not run too early and similar errors do not happen in the future.

[okat]

@azaozz Thank you for taking a look at this!

Looking at the original issue, it seems better to warn plugin authors that methods like $wp_theme->get_stylesheet() cannot be used before the plugins_loaded action (which runs immediately after all plugins and pluggable.php are loaded). Then add a "doing it wrong" in these methods similarly to the way in script-loader.php (i.e. check if the plugins_loaded action has run and call doing_it_wrong if not). This will ensure that the stylesheet, template, etc. filters do not run too early and similar errors do not happen in the future.

If we proceed with this approach, the user will just have to wait for the error currently occurring in Block Theme Previews to be fixed over time by the plugin author, which may take an unpredictable amount of time.
Also, for plugin authors, not being able to use $wp_theme->get_stylesheet() before the plugins_loaded action could be perceived as a breaking change. Not a few plugins (such as CoBlocks, Elementor, WooCommerce) will be affected. That's the reason I changed the order of loading in ​https://github.com/WordPress/wordpress-develop/pull/5028 while keeping the current behavior.

Could we confirm if taking this direction is acceptable? Do you have any thoughts?

[azaozz]

Replying to okat:

Also, for plugin authors, not being able to use $wp_theme->get_stylesheet() before the plugins_loaded action could be perceived as a breaking change. Not a few plugins (such as CoBlocks, Elementor, WooCommerce) will be affected. That's the reason I changed the order of loading in ​https://github.com/WordPress/wordpress-develop/pull/5028 while keeping the current behavior.

Thinking we may be misunderstanding one another a bit :)

Calling $wp_theme->get_stylesheet() is unaffected by the proposed PR/patch. Any plugin or theme or core code that does this before wp-includes/pluggable.php is loaded is "doing it wrong".

The PR only affects the case that seems to happen because of the code in theme-previews.php where filters and actions are added in the global scope, here: https://core.trac.wordpress.org/browser/tags/6.3/src/wp-includes/theme-previews.php#L78.

Looking at this more, if the theme-previews.php file is loaded later (as in the PR) it may also cause problems if a plugin is trying to remove one of these filters from "loose code" that's defined in global scope, as these won't exist when the plugin is loaded. That seems very unlikely but not impossible.

Could we confirm if taking this direction is acceptable? Do you have any thoughts?

Thinking the underlying problem here is with calling $wp_theme->get_stylesheet() before pluggable.php is loaded. That needs to be fixed. As far as I see the only way to fix it would be to educate plugin authors not to do it. In any case, having a "loose" functional code that runs in global scope in a plugin's file is a big "bad practice" usually. Triggering "doing it wrong" in such cases seems warranted.

There seem to be couple alternatives to fix the particular case with theme-previews.php. Perhaps instead of moving the file it would be better to move the code that is adding the actions and filters to a function, and then load that function on plugins_loaded (add the hook for it in default_filters.php)? Then the file would not need moving and there will be no "loose code" in the global scope there.

Moving this tentatively to 6.3.2 for a review providing there is a good patch for it.

This PR ensures theme preview hooks are added after the plugins_loaded action fires. This change aligns with best practices and helps avoid the fatal error on Block Theme Previews.

• Encapsulated the action and filter hooks related to theme previews into a function initialize_theme_preview_hooks.
• Hooked initialize_theme_preview_hooks to the plugins_loaded action in default-filters.php

Trac ticket: https://core.trac.wordpress.org/ticket/59000
Fixes: ​https://github.com/WordPress/gutenberg/issues/53284

## Why

Currently, the action and filter hooks related to theme previews are added in the global scope, which could lead to unexpected behaviors with other plugins or internal functionalities. See https://core.trac.wordpress.org/ticket/59000#comment:15 and ​https://github.com/WordPress/gutenberg/issues/53284. This change ensures that the hooks are added at the appropriate time in the lifecycle, specifically after all plugins have been loaded.

## Testing

• Navigate to /wp-admin/themes.php and click the Live Preview button on any Block theme (e.g. Twenty Twenty-Two).
• Verify that Block Theme Previews functionality works as expected.
• Test with third-party plugins, such as CoBlocks, to ensure there is no fatal error on Block Theme Previews.

[okat]

Calling $wp_theme->get_stylesheet() is unaffected by the proposed PR/patch. Any plugin or theme or core code that does this before wp-includes/pluggable.php is loaded is "doing it wrong".

In any case, having a "loose" functional code that runs in global scope in a plugin's file is a big "bad practice" usually. Triggering "doing it wrong" in such cases seems warranted.

Thank you for the insightful feedback, @azaozz. It helped clarify several key points for me!
I agree that best practices should be followed to avoid "loose" functional code running globally, and triggering "doing it wrong" makes sense.

There seem to be couple alternatives to fix the particular case with theme-previews.php. Perhaps instead of moving the file it would be better to move the code that is adding the actions and filters to a function, and then load that function on plugins_loaded (add the hook for it in default_filters.php)? Then the file would not need moving and there will be no "loose code" in the global scope there.

For the issue this time, I took the approach by encapsulating hooks into a function and initializing it in default_filters.php. I submitted another PR, so I would greatly appreciate it if you retake a look at the PR. Thank you in advance! ​https://github.com/WordPress/wordpress-develop/pull/5107

Thinking this looks good, thanks.

Wondering if any unit tests may be possible here? Or maybe better to add tests for the underlying problem: calling $wp_theme->get_stylesheet() before pluggable.php is loaded.

Thank you for your review, @azaozz!
I could write a test to guarantee that plugin_loaded triggers the initialization. What do you think?

Hello, @azaozz @mtias @noisysocks! To fix the crash of Block Theme Previews, I'd appreciate it if you could review and commit this change to the core. Thank you in advance!

I'm closing this PR in favor of ​https://github.com/WordPress/wordpress-develop/pull/5107.

Hello, @azaozz @mukeshpanchal27 @mtias 👋
Could you help us move this forward and fix the fatal error on theme previews?

[azaozz]

Could you help us move this forward

Sure, ​the latest patch/PR seems to fix the regression. However I'd like to really "get to the bottom of it" and try to remedy the underlying problem if at all possible. Alternatively can add some more docs to try to prevent similar usage in the future.

[azaozz]

In 56529:

Editor: Prevent possibility of a fatal error when previewing block themes.

Ensures that preview callbacks attached to the stylesheet and template filters do not run before pluggable.php has been included. These callbacks need functionality from pluggable.php.

Props: scruffian, johnbillion, SergeyBiryukov, okat, okat.
Fixes: #59000.

[azaozz]

Reopen for 6.3.2.

[azaozz]

Uh, typo: meant to add myself at the end of the props string, not repeat okat :)

This PR adds a _doing_it_wrong notice if a theme is requested too early when previewing a theme.

Trac ticket: https://core.trac.wordpress.org/ticket/59000
Related: ​https://github.com/WordPress/gutenberg/issues/53284

## Why

This is a follow-up for ​https://github.com/WordPress/wordpress-develop/pull/5107.

The approach in ​https://github.com/WordPress/wordpress-develop/pull/5107, “running the filters later”, works in most cases. The one exception is the case when the site has a plugin that runs stylesheet or templates before pluggable.php (which means a plugin is loaded before plugin_loaded), AND when the plugin wants to show the previewing theme. In this case, it gets the active theme’s stylesheet (not the previewing theme), which may lead to inconsistency.

For such a case, this PR adds a _doing_it_wrong notice if a theme is requested too early when previewing a theme.

## Testing

• Add a file in wp-content/plugins/sample-plugin/sample-plugin.php on your site.

  <?php
  /*
  Plugin Name: Sample plugin
  */
  
  $wp_theme = wp_get_theme();
  $wp_theme->get_stylesheet();
  

• Navigate to /wp-admin/themes.php and click the Live Preview button on any Block theme (e.g. Twenty Twenty-Two).
• See a notice in your debug.php: Function get_stylesheet() was called <strong>incorrectly</strong>. Calling get_stylesheet() before pluggable.php has loaded can result in getting the current theme instead of the previewed theme. Please ensure that you are using these functions after the plugins_loaded action has fired if this is not intended.

[okat]

Thank you for applying the patch to trunk, @azaozz!

However I'd like to really "get to the bottom of it" and try to remedy the underlying problem if at all possible. Alternatively can add some more docs to try to prevent similar usage in the future.

I submitted the PR trying to prevent early function calls by adding a notice. Could you take a look at ​https://github.com/WordPress/wordpress-develop/pull/5182? Or I'd appreciate any other suggestions or comments 🙏🙂

[azaozz]

Replying to okat:

I submitted the PR trying to prevent early function calls by adding a notice.

Thanks!

I'm actually starting to have some "second thoughts" about this (yea, I know it was my idea, sorry). Looking at how get_stylesheet() is used in core, it's mostly to check the (active) theme, not to actually get the path/URI to load the theme assets. It only returns the (filtered) option from the options table, nothing else. Seems it should be possible to do that at any time.

The problem in this ticket doesn't seem to be that the stylesheet and template filters may be called too early, but that it is likely they are not the best filters for wp_get_theme_preview_path() (previewing changes/edits to the theme should not be changing what the active theme is, right?).

Thinking that the current patch: attaching the filters late (on plugins_loaded) seems like an okay solution, for now. Ideally the way wp_get_theme_preview_path() is implemented can be improved so it uses different filters/hooks.

[okat]

Thank you for sharing your insights, @azaozz! I see your perspective that get_stylesheet() should be accessible anytime. The core of the issue may be about ensuring that wp_get_theme_preview_path() is implemented with appropriate filters or anything, as you pointed out.

While I currently don't have a concrete implementation plan for that approach, I do believe it's valuable to explore for version 6.4 while I think the current patch (https://core.trac.wordpress.org/ticket/59000#comment:23) (and ​https://github.com/WordPress/wordpress-develop/pull/5182 maybe?) would be good additions to 6.3.2.

📝 It's committed to trunk in https://core.trac.wordpress.org/changeset/56529; leaving the PR open for backporting to the 6.3.2.

[danielbachhuber]

@okat @azaozz It looks like initialize_theme_preview_hooks could benefit from a prefix -> wp_initialize_theme_preview_hooks.

Updated the name of initialize_theme_preview_hooks to align with WordPress standards by prefixing with wp_. This is for consistency and reducing naming conflicts.

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

As that ticket was merged in 6.3 branch could you update the base branch to 6.3? Also these changes land in 6.3.2 right?

[danielbachhuber]

In 56757:

Editor: Add function prefix to avoid conflicts.

Transforms initialize_theme_preview_hooks to wp_initialize_theme_preview_hooks to avoid conflicts with third-party code.

Follow up to [56529].

Props okat.
See #59000.

[danielbachhuber]

In 56758:

Editor: Prevent possibility of a fatal error when previewing block themes.

Ensures that preview callbacks attached to the stylesheet and template filters do not run before pluggable.php has been included. These callbacks need functionality from pluggable.php.

Props scruffian, johnbillion, SergeyBiryukov, okat, azaozz.
Merges [56529] and [56757] to the 6.3 branch.
Fixes #59000.

Closing, as this was shipped in 6.3.2. https://core.trac.wordpress.org/changeset/56757