WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 6 months ago

#21931 closed enhancement (fixed)

White screen of death if theme can't be found

Reported by: ericlewis Owned by:
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Themes Keywords:
Focuses: Cc:

Description

If the current theme can't be found or is renamed, the front-end of the site will white screen. This is developer error, as when WP_USE_THEMES is true, the template loader should always have a template to load (as far as I understand).

It's rare, but I come across it every once in a while in developing. It might be nice to have a wp_die() if a proper template can't be found in template-loader.

Proof of concept:

if ( $template = apply_filters( 'template_include', $template ) )
	include( $template );
else
	wp_die("Whoops! Looks like you're missing a theme.");

Attachments (11)

21931.diff (537 bytes) - added by MikeHansenMe 4 years ago.
21931.2.diff (497 bytes) - added by SergeyBiryukov 4 years ago.
21931.3.diff (513 bytes) - added by MikeHansenMe 22 months ago.
Refreshed
21931.4.diff (635 bytes) - added by SergeyBiryukov 7 months ago.
21931.5.diff (636 bytes) - added by ericlewis 6 months ago.
21931.6.diff (635 bytes) - added by ericlewis 6 months ago.
Screen Shot 2016-01-17 at 1.14.06 PM.png (184.5 KB) - added by ericlewis 6 months ago.
21931.7.diff (521 bytes) - added by ericlewis 6 months ago.
21931.8.diff (522 bytes) - added by ericlewis 6 months ago.
21931.patch (678 bytes) - added by sebastian.pisula 6 months ago.
21931.9.diff (538 bytes) - added by ericlewis 6 months ago.

Download all attachments as: .zip

Change History (55)

@MikeHansenMe
4 years ago

#1 @MikeHansenMe
4 years ago

I think this is a good idea as most users would not know where to begin looking if they experienced a white screen.

Last edited 4 years ago by MikeHansenMe (previous) (diff)

#2 @MikeHansenMe
4 years ago

  • Cc mdhansen@… added

#3 @miqrogroove
4 years ago

Last edited 4 years ago by miqrogroove (previous) (diff)

#4 @miqrogroove
4 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #11282.

#5 @SergeyBiryukov
4 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Related, but not exactly a duplicate.

#6 follow-up: @scribu
4 years ago

I've also run into this. Should be careful with the error message, since it can show up on live sites in certain circumstances.

#7 in reply to: ↑ 6 @ericlewis
4 years ago

Replying to scribu:

Should be careful with the error message, since it can show up on live sites in certain circumstances.

Can you expand on that? I can't imagine the circumstance where $template is expected to be empty.

#8 follow-up: @scribu
4 years ago

I remember I screwed up a deploy once and the theme was renamed. Since I had deleted the twentyeleven theme, I got a WSOD. With this bug, I would get a message instead.

What I'm saying is that the error message shouldn't assume that it will only be seen by developers.

#9 follow-up: @MikeHansenMe
4 years ago

what about "Looks like the website is having theme trouble, please notify the site owner" or something to that effect?

#10 in reply to: ↑ 9 @ericlewis
4 years ago

After chatting with Nacin in IRC a bit, some thought has to be put in here in regard to the ambiguity of the problem, and the ambiguity of the audience. This situation may arise from not just the themes folder missing, it may be a filesystem error, or come from another unknown cause. Therefore, our presentation of the error should respect this fact, and not send developers with possibly misleading debug information.

Last edited 4 years ago by ericlewis (previous) (diff)

#11 @scribu
4 years ago

It's nearly the same thing, but not quite; as the developer, you can at least know that WP was loaded.

And if there's a hook available, you can write a plugin that outputs a more useful message or logs it somewhere.

#12 @SergeyBiryukov
4 years ago

Could we do something along the lines of what wp_dashboard_right_now() does?
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-admin/includes/dashboard.php#L365

If the current theme is not found, 21931.2.diff shows "The theme directory does not exist".

Related #21670 (for more exact message in the admin).

Version 0, edited 4 years ago by SergeyBiryukov (next)

#13 follow-up: @MikeHansenMe
4 years ago

The thing about using Right Now in the dashboard to deliver messages is that it can be turned off. Perhaps there is a problem with that as well. Either not allow right now to be turned off or move error messages above the separate content blocks?

#14 in reply to: ↑ 8 @johnbillion
4 years ago

Replying to scribu:

What I'm saying is that the error message shouldn't assume that it will only be seen by developers.

Let's make the message capability-dependant then.

if ( current_user_can( 'switch_themes' ) )
	$message = __( 'You just broke everything!' );
else
	$message = __( 'Everything is fine and we will be back shortly.' );

wp_die( $message );

#15 in reply to: ↑ 13 ; follow-up: @SergeyBiryukov
4 years ago

Replying to MikeHansenMe:

The thing about using Right Now in the dashboard to deliver messages is that it can be turned off.

The idea behind 21931.2.diff was to display a similar message on the front end.

#16 in reply to: ↑ 15 @MikeHansenMe
4 years ago

Replying to SergeyBiryukov:

Replying to MikeHansenMe:

The thing about using Right Now in the dashboard to deliver messages is that it can be turned off.

The idea behind 21931.2.diff was to display a similar message on the front end.

I thought you were suggesting a user message in the front and developer error in the admin. I do think giving the error on the front end is a good idea because like I said, the Right Now area in the admin can be turned off. I think that may be a problem of it's own though.

#17 @ericlewis
3 years ago

  • Keywords ux-error-recovery added

This ticket was mentioned in IRC in #wordpress-dev by tierra. View the logs.


3 years ago

@MikeHansenMe
22 months ago

Refreshed

#20 @MikeHansenMe
22 months ago

Just tested this and the WSOD is still the case. Refreshed existing patch.

#21 @SergeyBiryukov
20 months ago

#13768 was marked as a duplicate.

#22 @SergeyBiryukov
15 months ago

#32199 was marked as a duplicate.

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


10 months ago

#24 @danielbachhuber
7 months ago

  • Milestone changed from Awaiting Review to Future Release

+1 to showing different messages based on user capability.

The WSOD still trips me up when I come across it. The first few times it happened, I remember losing an hour trying to figure out why my frontend didn't load.

+100 to showing a message when the theme can't be found.

#25 @ericlewis
7 months ago

  • Keywords has-patch added; 2nd-opinion ux-error-recovery removed
  • Milestone changed from Future Release to 4.5

I think we should do something along the lines of @SergeyBiryukov's suggestion, see attachment:21931.3.diff. which outputs the message The theme directory "{{ theme_directory_name }}" does not exist.

What's wrong with a site visitor seeing this message? Visitors can see the Error establishing a database connection message, this seems in the same vein. A WP site needs a DB connection, a WP site needs a theme. Fail early, fail explicitly.

@nacin, @SergeyBiryukov — thoughts?

#26 @SergeyBiryukov
7 months ago

This check seems a bit out of place in wp_templating_constants(), and I can't remember why I put it there :)

21931.4.diff moves it to template-loader.php, as per the initial suggestion and @MikeHansenMe's first patch.

#27 follow-up: @nacin
7 months ago

I don't hate 21931.4.diff, but I'm not sold on showing this to anonymous users. Minor code thing: a WP_Error object can be passed directly to wp_die().

Way back when (according to lore), WP used to actively switch to the default theme on the frontend when the theme was broken. In some situations (network-based filesystem, perhaps? — I assume an I/O glitch), this would cause a perfectly normal site to suddenly switch to the default theme. This now only happens in the admin in validate_current_theme(). Just providing some additional context, which may help inform whether cap checks are necessary here.

One other question to think about: under what circumstances would this else block be hit? Are we unnecessarily calling wp_get_theme() (a filesystem hit) in a common situation where we shouldn't?

@ericlewis
6 months ago

#28 in reply to: ↑ 27 @ericlewis
6 months ago

Replying to nacin:

I don't hate 21931.4.diff, but I'm not sold on showing this to anonymous users. Minor code thing: a WP_Error object can be passed directly to wp_die().

Updated in attachment:21931.5.diff, let's show this to logged-in users.

Replying to nacin:

One other question to think about: under what circumstances would this else block be hit? Are we unnecessarily calling wp_get_theme() (a filesystem hit) in a common situation where we shouldn't?

This should only be hit if a theme template is intended to be loaded as far as I can imagine.

@ericlewis
6 months ago

#29 @ericlewis
6 months ago

In attachmnet:21931.6.diff, use `elseif`.

#30 @ericlewis
6 months ago

In 36335:

Themes: Show an error message to logged-in users if a template file isn't loaded.

On the off-chance the active theme folder is renamed or deleted, a "white screen
of death" was displayed to the user. Instead, the user is shown a useful error
screen displaying any errors the theme has (e.g. if the theme can't be found).

Props MikeHansenMe, SergeyBiryukov.
See #21931.

#31 follow-up: @joehoyle
6 months ago

What's the reasoning behind only checking is_user_logged_in() - this doesn't really prove anything about the user being privileged to see anything special. For example, you can be a user without any capabilities whatsoever. IMO is_user_logged_in() shouldn't be used to indicate anything to do with privilege, we should use current_user_can( 'manage_themes' ) or something similar. If there's no need for any permissions to view this, then is_user_logged_in seems arbitrary.

#32 @ericlewis
6 months ago

The previous attachment shows the error page.

@ericlewis
6 months ago

#33 in reply to: ↑ 31 @ericlewis
6 months ago

Replying to joehoyle:

What's the reasoning behind only checking is_user_logged_in() - this doesn't really prove anything about the user being privileged to see anything special. For example, you can be a user without any capabilities whatsoever. IMO is_user_logged_in() shouldn't be used to indicate anything to do with privilege, we should use current_user_can( 'manage_themes' ) or something similar. If there's no need for any permissions to view this, then is_user_logged_in seems arbitrary.

That's a good point. Added that cap check in attachment:21931.7.diff.

@ericlewis
6 months ago

#34 @ericlewis
6 months ago

install_themes seems like the right cap check here.

#35 @ericlewis
6 months ago

In 36338:

Themes: Only users with proper capability should see theme errors.

After [36335], if a template file is not loaded, an error is displayed
to logged-in users. As logged-in users may have no capabilities,
this check is insubstantial. Limit the display of this error to users
with the install_themes capability, i.e. someone who has the capacity
to deal with the error.

See #21931.

#36 follow-up: @johnbillion
6 months ago

  • Keywords has-patch removed

Why install_themes instead of switch_themes?

#37 in reply to: ↑ 36 @ericlewis
6 months ago

Replying to johnbillion:

Why install_themes instead of switch_themes?

I was thinking in the worst-case scenario where all themes have been deleted, it will require a user with install_themes to fix the problem. What do you think?

#38 @dd32
6 months ago

I feel a more basic switch_themes would be more appropriate if anything here.

Keep multisite in mind as a use-case, where a site admin would rarely have the ability to install themes.

#39 follow-up: @sebastian.pisula
6 months ago

  • Keywords 2nd-opinion added

I use this filter:

<?php
add_filter('template_include', function(){
        return locate_template('test.php');
});

And I have white page. I think that should be this if:

<?php
if ( $template = apply_filters( 'template_include', $template ) ) {
                include( $template );
        } elseif ( current_user_can( 'install_themes' ) ) {
                $theme = wp_get_theme();
                if ( $theme->errors() ) {
                        wp_die( $theme->errors() );
                } else {
                        wp_die( __( 'Template is missing.' ) );
                }
        }

But theme->erros return error if not exists index.php file so better patch is:

<?php
        if ( $template = apply_filters( 'template_include', $template ) ) {
                include( $template );
        } elseif ( current_user_can( 'install_themes' ) ) {
                wp_die( __( 'Template is missing.' ) );
        }

#40 @sebastian.pisula
6 months ago

Patch for 4.5-alpha-1453085953122

#41 @iseulde
6 months ago

Love it

@ericlewis
6 months ago

#42 @ericlewis
6 months ago

In 36344:

Themes: Show template loading error to users with switch_themes cap.

In [36338], a template loading error was shown only to users with the install_themes capability. This is now displayed users with the switch_themes capability, as users with this cap can at least switch to a different theme. Also, this will now show for site administrators in multisite, whereas install_themes is limited to superadmins.

Props dd32.
See #21931.

#43 in reply to: ↑ 39 @ericlewis
6 months ago

  • Keywords dev-feedback 2nd-opinion removed

Replying to sebastian.pisula:

But theme->erros return error if not exists index.php file so better patch is:

<?php
        if ( $template = apply_filters( 'template_include', $template ) ) {
                include( $template );
        } elseif ( current_user_can( 'install_themes' ) ) {
                wp_die( __( 'Template is missing.' ) );
        }

Thanks for the suggestion here @sebastian.pisula!

This approach is more prone to break things. I could imagine plugins purposefully filtering template_include to a falsy value to, say, output some JSON (or who knows what else). Of course this is not what template_include is intended for, but in support of backwards compatibility, we should respect this functionality by displaying an error only if there are issues with the theme, which is an exceptional problem for the template loader.

#44 @ericlewis
6 months ago

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

Going to close this as fixed, thanks to all involved :)

If anyone would like to continue discussion about whether this error should be displayed to anonymous users, feel free to open a new ticket.

Note: See TracTickets for help on using tickets.