Make WordPress Core

Opened 12 months ago

Closed 10 months ago

Last modified 9 months ago

#59000 closed defect (bug) (fixed)

Add a check for wp_get_current_user to current_user_can

Reported by: scruffian's profile scruffian Owned by: azaozz's profile azaozz
Milestone: 6.3.2 Priority: normal
Severity: normal Version: 6.3
Component: Users Keywords: has-patch has-unit-tests needs-testing fixed-major
Focuses: Cc:

Description

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.

Change History (37)

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


12 months ago
#1

  • Keywords has-patch added

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

#2 @SergeyBiryukov
12 months ago

  • Component changed from General to Users

#3 follow-up: @johnbillion
12 months ago

  • Keywords 2nd-opinion added

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?

#4 @scruffian
12 months ago

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.

#5 in reply to: ↑ 3 @SergeyBiryukov
12 months ago

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.

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


11 months ago
#7

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 ticket was mentioned in PR #5028 on WordPress/wordpress-develop by @okat.


11 months ago
#8

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

@okat commented on PR #5028:


11 months ago
#9

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.
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5287479/74eafb14-2395-44f8-add6-f9edc7d6dc82
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5287479/44ab1aca-47ab-4bdf-92dd-1506566e6389

@scruffian commented on PR #5028:


11 months ago
#10

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

@okat commented on PR #5028:


11 months ago
#12

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!

#13 @azaozz
11 months ago

  • Keywords needs-patch added; has-patch removed

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.

Last edited 11 months ago by azaozz (previous) (diff)

#14 follow-up: @okat
11 months ago

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

#15 in reply to: ↑ 14 @azaozz
11 months ago

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 ticket was mentioned in PR #5107 on WordPress/wordpress-develop by @okat.


11 months ago
#16

  • Keywords has-patch added; needs-patch removed

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.

#17 @okat
11 months ago

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

Last edited 11 months ago by okat (previous) (diff)

@azaozz commented on PR #5107:


11 months ago
#18

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.

@okat commented on PR #5107:


11 months ago
#19

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

@okat commented on PR #5028:


11 months ago
#20

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.

@okat commented on PR #5107:


11 months ago
#21

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

#22 @azaozz
11 months ago

  • Keywords has-unit-tests needs-testing added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 6.3.2
  • Version set to 6.3

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.

Last edited 11 months ago by azaozz (previous) (diff)

#23 @azaozz
11 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

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.

Version 0, edited 11 months ago by azaozz (next)

#24 @azaozz
11 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 6.3.2.

#25 @azaozz
11 months ago

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

Last edited 11 months ago by azaozz (previous) (diff)

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


11 months ago
#26

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.

#27 follow-up: @okat
11 months ago

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 🙏🙂

#28 in reply to: ↑ 27 @azaozz
11 months ago

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? Seems get_stylesheet() is used to identify the active theme.

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.

Last edited 11 months ago by azaozz (previous) (diff)

#29 @okat
11 months ago

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.

@okat commented on PR #5107:


10 months ago
#30

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

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


10 months ago

#32 @danielbachhuber
10 months ago

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

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


10 months ago
#33

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

@mukesh27 commented on PR #5361:


10 months ago
#34

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?

#35 @danielbachhuber
10 months ago

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.

#36 @danielbachhuber
10 months ago

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

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.

@okat commented on PR #5361:


9 months ago
#37

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

Note: See TracTickets for help on using tickets.