#59000 closed defect (bug) (fixed)
Add a check for wp_get_current_user to current_user_can
Reported by: | scruffian | Owned by: | 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.
15 months ago
#1
- Keywords has-patch added
#3
follow-up:
↓ 5
@
15 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
@
15 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
@
15 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.
@scruffian commented on PR #4959:
15 months ago
#6
Intended to fix https://core.trac.wordpress.org/ticket/59000
This ticket was mentioned in PR #5028 on WordPress/wordpress-develop by @okat.
15 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.
15 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
15 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.
@scruffian commented on PR #5028:
15 months ago
#10
This makes perfect sense to me, thanks for working on it.
@scruffian commented on PR #4959:
15 months ago
#11
Closing in favour of https://github.com/WordPress/wordpress-develop/pull/5028
15 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
@
15 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.
#14
follow-up:
↓ 15
@
15 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
@
15 months ago
Replying to okat:
Also, for plugin authors, not being able to use
$wp_theme->get_stylesheet()
before theplugins_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.
15 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 theplugins_loaded
action indefault-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 theLive 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
@
15 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
15 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.
15 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?
15 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.
14 months ago
#21
Hello, @azaozz @mukeshpanchal27 @mtias 👋
Could you help us move this forward and fix the fatal error on theme previews?
#22
@
14 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.
#23
@
14 months ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 56529:
#24
@
14 months ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopen for 6.3.2.
#25
@
14 months ago
Uh, typo: meant to add myself at the end of the props string, not repeat okat
:)
This ticket was mentioned in PR #5182 on WordPress/wordpress-develop by @okat.
14 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 theLive 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:
↓ 28
@
14 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
@
14 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.
#29
@
14 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.
14 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.
14 months ago
#32
@
14 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.
13 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:
13 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?
13 months ago
#37
Closing, as this was shipped in 6.3.2. https://core.trac.wordpress.org/changeset/56757
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
beforewp_get_current_user
has been defined. We could guard against this by checking whetherwp_get_current_user
is defined before callingcurrent_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