Show different fatal error message when in Recovery Mode and unable to redirect

[TimothyBlynJacobs]

WP_Fatal_Error_Handler::handle() will call WP_Recovery_Mode::handle_error(). If recovery mode is active, that method will try and refresh the page via a redirect to catch multiple errors at once. However, if headers have already been sent, it bails out and returns true.

This has the effect of printing the "The site is experiencing technical difficulties. Please check your site admin email inbox for instructions." when already in Recovery Mode which may be confusing. We should display a different message when the error was handled but we were unable to redirect.

[TimothyBlynJacobs]

I've uploaded a patch that adds the different text. The text itself is really just a placeholder and needs to be discussed.

A fatal error was caught by recovery mode. Check the Plugins and/or Themes page for details.

[mukesh27]

Updated patch.

[mukesh27]

@TimothyBlynJacobs patch updated as ticket address in 5.2.2 milestone and change newly added document as per core standard.

Check below:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/author-template.php#L182
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-http.php#L156

@SergeyBiryukov Can you please check it and let me know if i am getting anything wrong.

[afragen]

Adding a simple test case for triggering these errors and testing the patch would be very helpful.

[marybaum]

Suggested copy:

“You’re in recovery mode, but for some reason we are unable to redirect you. This may mean there are several errors. Please contact your site admin, your developer or the WordPress support forums.”

[garrett-eclipse]

Refreshed patch to include verbiage supplied by @marybaum

[marybaum]

Looks good to me!

[garrett-eclipse]

Replying to marybaum:

Suggested copy:

“You’re in recovery mode, but for some reason we are unable to redirect you. This may mean there are several errors. Please contact your site admin, your developer or the WordPress support forums.”

I've refreshed the patch in 47321.3.diff to include @marybaum's suggested copy. Thanks @TimothyBlynJacobs & @mukesh27 for the initial patches.

Minor question: What's the policy on fancy quote in core? You’re

Also should the two other strings found in the display_default_error_template function get some copy love as well to match the tone and conveyance of this string? They are;

} elseif ( is_protected_endpoint() ) {
        $message = __( 'The site is experiencing technical difficulties. Please check your site admin email inbox for instructions.' );
} else {
        $message = __( 'The site is experiencing technical difficulties.' );
}

Also adding 'needs-unit-tests' to account for @afragen's request;
Adding a simple test case for triggering these errors and testing the patch would be very helpful.

[TimothyBlynJacobs]

“You’re in recovery mode, but for some reason we are unable to redirect you. This may mean there are several errors. Please contact your site admin, your developer or the WordPress support forums.”

I'm not sure this is 100% correct. And I think we might end up displaying a severe message too often. In technical terms, this would happen when an error happens after the page has partially rendered to the screen. I think this could end up being a large number of errors. The only thing the user needs to do in this situation is manually refresh the page or navigate to another admin page.

[marybaum]

“You’re in recovery mode, but for some reason we are unable to redirect you. Please try refreshing. If you see this again, please contact your site admin, your developer or the WordPress support forums.”

[SergeyBiryukov]

Looks like this needs some more work.

we are unable to redirect you

This should be rewritten without the "we", see #46057.

Please contact your site admin, your developer

What if the user is a site admin or developer?

or the WordPress support forums

This should link to the forums.

if ( true === $handled ... )

Core does not generally use strict comparison in cases like this, if ( $handled ... ) should be enough.

[TimothyBlynJacobs]

Replying to SergeyBiryukov:

if ( true === $handled ... )

Core does not generally use strict comparison in cases like this, if ( $handled ... ) should be enough.

I don't think this will work in this case since a WP_Error object will be truthy. $handled is a bool|WP_Error.

[SergeyBiryukov]

Replying to TimothyBlynJacobs:

I don't think this will work in this case since a WP_Error object will be truthy. $handled is a bool|WP_Error.

Ah, thanks, I missed that.

[marybaum]

Copy here -- how about

There's been a fatal error, so you are now in recovery mode. Please check the Plugins and/or Themes page for details.

[audrasjb]

This ticket still needs some discussion. As seen per today's bug scrub, let's punt it to 5.3 so we could have a better resolution :-)

[garrett-eclipse]

After discussing in #core-site-health here - ​https://wordpress.slack.com/archives/CKSU841L7/p1562699059112200

The decision was;
“There has been a critical error on your website, putting it in recovery mode. Please check the Themes and Plugins pages for more details. If you just now installed a new Theme or Plugin, check the relevant page for that first.”

[garrett-eclipse]

Updated verbiage

[garrett-eclipse]

Replying to garrett-eclipse:

After discussing in #core-site-health here - ​https://wordpress.slack.com/archives/CKSU841L7/p1562699059112200

The decision was;
“There has been a critical error on your website, putting it in recovery mode. Please check the Themes and Plugins pages for more details. If you just now installed a new Theme or Plugin, check the relevant page for that first.”

I've updated the verbiage according to the Slack discussion mentioned above in 47321.4.diff.

Aside from the verbiage the outstanding questions were;
Original comment by garrett-eclipse:

Also should the two other strings found in the display_default_error_template function get some copy love as well to match the tone and conveyance of this string? They are;

} elseif ( is_protected_endpoint() ) {
        $message = __( 'The site is experiencing technical difficulties. Please check your site admin email inbox for instructions.' );
} else {
        $message = __( 'The site is experiencing technical difficulties.' );
}

Also adding 'needs-unit-tests' to account for @afragen's request;
Adding a simple test case for triggering these errors and testing the patch would be very helpful.

*Added dev-feedback, needs-copy-review and needs-unit-tests to address these items.

[garrett-eclipse]

P.S. Please along with the initial patch props for @TimothyBlynJacobs & @mukesh27 can you please ensure additional props for the copy discussion be added for @SergeyBiryukov (as he provided input here on Trac), @marybaum, @clorith, @afragen and @mdwolinski as they participated in the Slack copy discussion. Thank you

[SergeyBiryukov]

Please check the Themes and Plugins pages for more details.

I think admin pages are generally referred to as screens in documentation, to avoid confusion with page as a post type.

If you just now installed a new Theme or Plugin, check the relevant page for that first.

I don't think we generally capitalize "theme" or "plugin" in sentences like this.

Looks good to me otherwise.

[garrett-eclipse]

Updated patch

[garrett-eclipse]

Thanks @SergeyBiryukov greatly appreciate the input

I've updated the patch (47321.5.diff) with the following;

• Updated versions to 5.3 from 5.2.2
• Updated 'pages' to 'screens'
• Lowercased references to installing themes and plugins but left the screen names capitalized.
• Updated the 'If you just now installed a theme or plugin' verbiage to cover updates by stating 'If you just installed or updated a theme or plugin'. *Also dropped 'now' as it felt redundant and in many cases they may have done an update before the weekend and only come back to work to find the issue later.

*Concerning the capitalization I went through core and from searching found that in a sentence a plugin or theme reference is lowercase such as install a plugin. But when referring to a screen it's a name and is capitalized.

Thoughts?

[garrett-eclipse]

*Probably easier to review at a glance if I surfaced the full sentence, in 47321.5.diff it is as follows;
'There has been a critical error on your website, putting it in recovery mode. Please check the Themes and Plugins screens for more details. If you just installed or updated a theme or plugin, check the relevant page for that first.'

[marybaum]

I think we have a winner!

Replying to garrett-eclipse:

*Probably easier to review at a glance if I surfaced the full sentence, in 47321.5.diff it is as follows;
'There has been a critical error on your website, putting it in recovery mode. Please check the Themes and Plugins screens for more details. If you just installed or updated a theme or plugin, check the relevant page for that first.'

[garrett-eclipse]

Awesome thanks @marybaum

So to move this forward the only outstanding questions were;

Also should the two other strings found in the display_default_error_template function get some copy love as well to match the tone and conveyance of this string? They are;
} elseif ( is_protected_endpoint() ) {

$message = ( 'The site is experiencing technical difficulties. Please check your site admin email inbox for instructions.' );

} else {

$message = ( 'The site is experiencing technical difficulties.' );

}
Also adding 'needs-unit-tests' to account for @afragen's request;
Adding a simple test case for triggering these errors and testing the patch would be very helpful.

@clorith / @afragen / @TimothyBlynJacobs do we want to make those two additional strings consistent with this new one? And does this actually need Unit Tests?
Thanks

[afragen]

I was only thinking of unit tests so we could be sure we know what activates this code. I’m OK with removing that tag.

[garrett-eclipse]

Thanks @afragen I've dropped the tag so the only remaining question is should those two additional strings be updated to be more consistent with the new verbiage. If not this is ready to move into commit, otherwise am happy to include those strings in the patch. Cheers

[marybaum]

Replying to garrett-eclipse:

OMG I missed this part completely! Yes, I think we should update these two strings to match.

<?php
} elseif ( is_protected_endpoint() ) {

$message= ( 'There has been a critical error. Please check your site's admin email for instuctions.' );

} else {

$message= ( 'There has been a critical error.' );

}

Awesome thanks @marybaum

So to move this forward the only outstanding questions were;

Also should the two other strings found in the display_default_error_template function get some copy love as well to match the tone and conveyance of this string? They are;
} elseif ( is_protected_endpoint() ) {

$message = ( 'The site is experiencing technical difficulties. Please check your site admin email inbox for instructions.' );

} else {

$message = ( 'The site is experiencing technical difficulties.' );

}
Also adding 'needs-unit-tests' to account for @afragen's request;

[garrett-eclipse]

Updated patch, using 'There has been a critical error on your website.' verbiage

[garrett-eclipse]

Thanks @marybaum I've refreshed the patch - 47321.6.diff

@afragen / @SergeyBiryukov could you have a last look, I feel this is ready for commit

[afragen]

@garrett-eclipse it looks good to me.

[garrett-eclipse]

Thanks @afragen marking it good to go

[SergeyBiryukov]

In 46119:

Site Health: Show a more specific fatal error message when in Recovery Mode with headers already sent.

Props garrett-eclipse, TimothyBlynJacobs, mukesh27, marybaum, afragen, Clorith, mdwolinski, SergeyBiryukov.
Fixes #47321.