WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 41 hours ago

#47321 reviewing enhancement

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

Reported by: TimothyBlynJacobs Owned by: marybaum
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch servehappy needs-copy-review has-copy-review needs-refresh
Focuses: Cc:

Description

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.

Attachments (3)

47321.diff (3.0 KB) - added by TimothyBlynJacobs 5 weeks ago.
47321.2.diff (3.0 KB) - added by mukesh27 3 weeks ago.
Updated patch.
47321.3.diff (3.0 KB) - added by garrett-eclipse 3 weeks ago.
Refreshed patch to include verbiage supplied by @marybaum

Download all attachments as: .zip

Change History (28)

#1 @TimothyBlynJacobs
5 weeks ago

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.

#2 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Awaiting Review to 5.2.2

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


5 weeks ago

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


4 weeks ago

@mukesh27
3 weeks ago

Updated patch.

#5 @mukesh27
3 weeks ago

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

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


3 weeks ago

#7 @afragen
3 weeks ago

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

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


3 weeks ago

#9 @audrasjb
3 weeks ago

  • Owner set to marybaum
  • Status changed from new to reviewing

#10 follow-up: @marybaum
3 weeks ago

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
3 weeks ago

Refreshed patch to include verbiage supplied by @marybaum

#11 @marybaum
3 weeks ago

  • Keywords commit added

Looks good to me!

#12 in reply to: ↑ 10 @garrett-eclipse
3 weeks ago

  • Keywords needs-testing needs-unit-tests added; commit removed

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.

#13 @TimothyBlynJacobs
3 weeks ago

“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.

#14 @marybaum
3 weeks ago

  • Keywords needs-testing needs-unit-tests removed

“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.”

#15 @marybaum
3 weeks ago

  • Keywords commit added

#16 follow-up: @SergeyBiryukov
2 weeks ago

  • Keywords commit removed

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.

#17 in reply to: ↑ 16 ; follow-up: @TimothyBlynJacobs
2 weeks ago

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.

#18 in reply to: ↑ 17 @SergeyBiryukov
2 weeks ago

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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


2 weeks ago

#20 @TimothyBlynJacobs
2 weeks ago

  • Keywords needs-copy-review added

#21 @marybaum
2 weeks ago

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.

#22 @marybaum
2 weeks ago

  • Keywords has-copy-review needs-refresh added

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


2 weeks ago

#24 @audrasjb
2 weeks ago

  • Milestone changed from 5.2.2 to 5.3

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 :-)

#25 @spacedmonkey
41 hours ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.