WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 weeks ago

#47050 new defect (bug)

Recovery Email - Keep consistent newline spacing around ###CAUSE### mergetag

Reported by: garrett-eclipse Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 5.2
Component: Site Health Keywords: has-patch
Focuses: ui-copy Cc:

Description

Hello,

While translating I came to the recovery email which uses several mergetags. They're all surrounded by empty lines with the exception of ###CAUSE### which I felt odd.

Current Email;

Howdy!↵
↵
Since WordPress 5.2 there is a built-in feature that detects when a plugin or theme causes a fatal error on your site, and notifies you with this automated email.↵
###CAUSE###↵
First, visit your website (###SITEURL###) and check for any visible issues. Next, visit the page where the error was caught (###PAGEURL###) and check for any visible issues.↵
↵
###SUPPORT###↵
↵
If your site appears broken and you can't access your dashboard normally, WordPress now has a special "recovery mode". This lets you safely login to your dashboard and investigate further.↵
↵
###LINK###↵
↵
To keep your site safe, this link will expire in ###EXPIRES###. Don't worry about that, though: a new link will be emailed to you if the error occurs again after it expires.↵
↵
###DETAILS###

Code Reference - https://build.trac.wordpress.org/browser/trunk/wp-includes/class-wp-recovery-mode-email-service.php?marks=134#L139

Opening this ticket to introduce empty lines around ###CAUSE###

The cause strings I found are;
"In this case, WordPress caught an error with one of your plugins, %s."
"In this case, WordPress caught an error with your theme, %s."

Both could use the space and not be sandwiched between the first two large paragraphs.

Cheers

Attachments (2)

47050.diff (693 bytes) - added by garrett-eclipse 2 years ago.
Introduce empty lines around ###CAUSE### in recovery email
47050.2.diff (978 bytes) - added by garrett-eclipse 23 months ago.
Refreshed patch to add unknown cause string

Download all attachments as: .zip

Change History (13)

@garrett-eclipse
2 years ago

Introduce empty lines around ###CAUSE### in recovery email

#1 @garrett-eclipse
2 years ago

  • Keywords has-patch added

#2 follow-up: @pento
2 years ago

This is intentional, as the cause can also be empty, so we don't want masses of whitespace in the email if there's no cause listed.

I'm fine with reworking the email text to make it consistent, but that needs to be taken into account.

#3 in reply to: ↑ 2 @garrett-eclipse
2 years ago

  • Severity changed from normal to minor

Replying to pento:

This is intentional, as the cause can also be empty, so we don't want masses of whitespace in the email if there's no cause listed.

I'm fine with reworking the email text to make it consistent, but that needs to be taken into account.

Thanks @pento I didn't realize there was the case of an empty cause when $extension isn't set (I've moved this down to minor). I do wonder if a 'Unknown Cause' string could be more useful than no cause. Or possibly reworking the cause section in a future iteration to provide a 'Cause: ' prefix on it's own line and either supply the cause or an 'N/A' (or Unknown).
*If this type of prefix is used then the existing cause strings won't need the verbose 'In this case, ' prefix.

Cheers

#4 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 5.2.1

#5 @desrosj
2 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.2.1 to Future Release

This still needs more work. Since it's a minor severity, let's move it to Future Release and it can be milestoned whenever it becomes ready.

@garrett-eclipse
23 months ago

Refreshed patch to add unknown cause string

#6 @garrett-eclipse
23 months ago

  • Focuses ui-copy added
  • Keywords dev-feedback needs-testing added; needs-refresh removed
  • Milestone changed from Future Release to 5.3

In 47050.2.diff I've introduced consistent new line around CAUSE and ensured there's always a string inserted. When no cause is know I used the following verbiage;
In this case, WordPress caught an unknown error.

#7 @pento
23 months ago

I think this path is only triggered if it happens in Core, which isn't unknown, it just doesn't have a relevant cause.

I don't recall the original discussion around why the error details weren't include for Core errors: it'd be worth looking at that, and seeing if there's a way we can get that information.

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


21 months ago

#9 @davidbaumwald
21 months ago

  • Milestone changed from 5.3 to Future Release

This ticket hasn't seen any recent movement for 5.3, so this is being moved to Future Release. If it can be included in 5.4, feel free to milestone during that cycle.

#10 @SergeyBiryukov
21 months ago

  • Component changed from Administration to Site Health

#11 @Clorith
6 weeks ago

  • Keywords dev-feedback needs-testing removed

I wasn't able to track down any discussions on why this section isn't spaced out, other than it being the only piece of the email that was introduced as a placeholder like this and wasn't inline (see [44973]) in the original implementation, so other patches likely erred on the side of not changing things out of their own scope.

As @pento mentioned, the fallback of just outputting an empty line is generally only done in a few situations, when the source can not reliably be determined as a plugin or theme, this information is populated from the error_get_last() PHP function, so if a plugin or theme filtered a value core was using, providing an invalid value for example, you may get an incorrect result pointing at core, when core isn't really to blame.

So absolutely, we can re-word the email to not need the cause to be wedged inside two other strings, but erring on the side of avoiding "An unknown error" type messages when possible is preferable (I can't say if this was the initial intent with how it was implemented, since it was implemented this way straight away), as such messages doesn't help the end user in any way, so it's often more useful to omit this. And of course, need to make sure we avoid added whitespace :)

Note: See TracTickets for help on using tickets.