Updated: Comment #42

Problem/Motivation

The default/fallback error message includes the words "unknown error" and should be modified to make a little more sense.

Proposed resolution

Make the error message incredibly simple but translatable in both instances.

Remaining tasks

  1. Determine the translatability in both cases.
  2. Decide on the language to be used.
  3. Preform a Beta Evaluation

User interface changes

Changes to the string which appears.

API changes

none

Original report by @chx

The website encountered an unexpected error. Please try again later.
Really. Because there are expected errors, right? Are we trying to get on the DailyWTF?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Issue tags: +Novice

I think this is trivial to fix just need to cook up better words. Warning: needs fixing twice: core/includes/errors.inc and core/lib/Drupal/Core/ExceptionController.php both.

Anonymous’s picture

Rather than "Please try again later." a more appropriate phase might be "Please send this page link to [site_mail] for resolution." or similar. Maybe even make the phrasing configurable by the admin.

mbrett5062’s picture

Assigned: Unassigned » mbrett5062
FileSize
1.6 KB

Would like to try and fix this.

Here is my proposed new message.

'The website has encountered an error and is unable to display your page. Please send this page link to [site_mail] for resolution'

And here is the patch.

Anonymous’s picture

Status: Active » Needs review

Here testbot.

mbrett5062’s picture

Send for test, and review.

Status: Needs review » Needs work

The last submitted patch, 1818692-3-clean_site_error_message.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

I would not encourage people to bombard the site mail -- if the site is down, believe me, the owner will know. Clearly this issue was filed to fix the first sentence.

mbrett5062’s picture

Status: Needs review » Needs work
FileSize
1.6 KB

OK try that again.

mbrett5062’s picture

OK, will fix that and send again.

mbrett5062’s picture

Here is the fix. Should pass tests!

mbrett5062’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1818692-3-clean_site_error_message.patch, failed testing.

shadcn’s picture

How about some helpful messages? Like "If you're the site admin, check your logs". Something along those lines.

ksenzee’s picture

Remember this message will be seen by visitors of very large enterprise sites, as well as small sites. Let's keep the wording as brief and professional as possible. I'd suggest simply removing the word "unexpected."

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

OK following on from ksenzee's suggestion, which I feel gives us the best re-write.

Here is the new patch. Don't know why it was failing tests. Lets try again.

Status: Needs review » Needs work

The last submitted patch, 1818692-15-clean_site_error_message.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

OK, I am new to this. I think I know what I did wrong. One more go.

Status: Needs review » Needs work

The last submitted patch, 1818692-17-clean_site_error_message.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

Last try, before I defer to an adult.

Missed the full stop off end of message.

Status: Needs review » Needs work

The last submitted patch, 1818692-19-clean_site_error_message.patch, failed testing.

Anonymous’s picture

Chx, should this comment apply to both use cases?

// Should not translate the string to avoid errors producing more errors.

mbrett5062, the test bot complains about the last line of the patch file. Does your patch file contain a newline character on the last line of the file?

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

Thanks @ernie that was indeed the problem. Recreated patch with git bash instead of eclipse IDE.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Great.

chx’s picture

Tagging for backport.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. Thanks!

mbrett5062’s picture

Working on D7 backport now.

mbrett5062’s picture

Status: Patch (to be ported) » Needs review
FileSize
660 bytes

D7 backport patch attached.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Good to go as well.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Um, I do not think this should be backported. That's a really bad string to break for a rewording like this.

Setting NR for now.

David_Rothstein’s picture

So what's the answer to @earnie's question in #21: Is this actually supposed to be a translated string? (I would tend to think so, for settings.php overrides even if nothing else.)

If it is supposed to be, I don't see how we can backport this change, per http://drupal.org/node/1527558 - this is kind of the definition of an end-user-facing error-message string change...

David_Rothstein’s picture

Oops, that was a cross-post, but same idea :)

xjm’s picture

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Fixed
Issue tags: -Needs backport to D6, -Needs backport to D7

Ha. :)

Setting back to fixed for D8 based on all the nice crosspostage above.

David_Rothstein’s picture

Title: The stupidest error message I ever saw » Improve the maintenance page error message
Category: bug » task
Status: Fixed » Needs review

Actually, I think this needs more thought for Drupal 8 as well.

The phrase "unexpected error" is very common in software programs (not just Drupal), and I believe the purpose of it actually is to distinguish it from expected errors. (An example of an expected error would be when you make a typo in your password while logging in to a site; this is an error that is expected to occur on occasion, and the system handles it gracefully.)

So I believe the point of the "unexpected error" message is just to signal to the user that something occurred which the programmers didn't anticipate, and to hint that that's why they're seeing some kind of weird error page rather than the normal website.

There are probably ways to convey this without the word "unexpected", but the way it reads to me now (with no word there at all) is kind of like "There was an error, but we aren't going to tell you any more details about it because we're secretive. Hope you enjoy staring at this maintenance page!" :)

David_Rothstein’s picture

Component: base system » user interface text
xjm’s picture

Issue tags: +Usability
mbrett5062’s picture

How about,

"An unknown error has occurred, the site administrator has been informed. Please try again later."

And lets make sure in the background that the admin is actually informed. via email

mbrett5062’s picture

Revised message to the following.

'The website has encountered an unknown error, the site administrator is aware. Please try again later.'

This is both more informative, and gives an indication that the site owners are doing something about it.

Unexpected has been replaced with unknown, which in essence is true, we are handling an unknown error, and no error's are expected.

xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work

There is no guarantee the "site administrator is aware" so I don't think that works as text. Also, please get rid of the comma splice. :)

mbrett5062’s picture

Assigned: mbrett5062 » Unassigned

Unable to continue working on this so opening for someone else to complete.

chrisfromredfin’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
949 bytes

OK, I'll bite. I'm taking the approach that if you can call theme() then you should be able to call t() as well. So this patch wraps BOTH in t() and keeps them at the simple, bland error message that's in D8 HEAD as of now. (In fact, I might even argue to remove the "Please try again later." sentence entirely, as well... but for now I've left it as it currently is until there's more feedback.)

Since it therefore IS translatable in both instances, we can leave it up to the site owners to modify the bland message to their own liking (in settings.php, etc).

Status: Needs review » Needs work

The last submitted patch, 42: 1818692-unhandled_error_message-42.patch, failed testing.

chrisfromredfin’s picture

Status: Needs work » Needs review

Re-queuing this for testing; the test passes on my local machine and the log from the PIFR seems to imply an unrelated / potentially fleeting issue.

chrisfromredfin’s picture

parthipanramesh’s picture

Status: Needs review » Reviewed & tested by the community

Seems to be ok. Better message now. Thank you!

lokapujya’s picture

Status: Reviewed & tested by the community » Needs review
adnanc’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed. Looks good.

An argument can be made for including a more obvious message to the user. However, that decision should lie with the site administrator.

Xano’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

The last patch no longer applies and needs a re-roll.

Status: Needs review » Needs work

The last submitted patch, 50: unhandled_error_message-1818692-50.patch, failed testing.

bohart’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 50: unhandled_error_message-1818692-50.patch, failed testing.

ipo4ka704’s picture

Status: Needs work » Needs review
dawehner’s picture

OK, I'll bite. I'm taking the approach that if you can call theme() then you should be able to call t() as well. So this patch wraps BOTH in t() and keeps them at the simple, bland error message that's in D8 HEAD as of now. (In fact, I might even argue to remove the "Please try again later." sentence entirely, as well... but for now I've left it as it currently is until there's more feedback.)

Isn't the difference that that t() might call the database which is kind of a way bigger problem then just calling theme() which can work with just code.

lokapujya’s picture

FileSize
1.55 KB

We should not have t(). Also, #817160: Don't translate exceptions will handle removing t() from exceptions.

Fixed the length of the comment for the error message. Doesn't seem to be a great answer for a better message. We do have an expected error or unexpected conditions.

raj.nitdgp’s picture

Its my first try at Drupal contribution. I took a clone today and tried applying the patch in #56 at the current HEAD and it did not apply.

lokapujya’s picture

FileSize
1.67 KB

Thanks raj.nitdgp. Rerolled.

vonn.new’s picture

Assigned: Unassigned » vonn.new

#58 Patch did not pass on SimplyTest.me
Error msg: An error occurred while patching the project.

vonn.new’s picture

Assigned: vonn.new » Unassigned
Status: Needs review » Needs work
lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.65 KB

If we aren't going to handle the error, we should call it an unexpected error as in D7.

vonn.new’s picture

patch in #61 passes simplytest.me

holist’s picture

Issue tags: +Amsterdam2014

I'm looking at this.

holist’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks OK to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The website encountered an unexpected error. Please try again later.
Really. Because there are expected errors, right? Are we trying to get on the DailyWTF?

I agree with @chx. I'm not sure what this patch is fixing. I think the text in HEAD is fine.

neelam.chaudhary’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
1.7 KB

Reroll of #61

blacklight4’s picture

For reference, this is how Wordpress writes its "unexpected error" message: "An unexpected error occurred. Something may be wrong with WordPress.org or this server’s configuration. If you continue to have problems, please try the support forums"

The word "unexpected" indicates the possibility of a one-off event like a file corruption. In the second sentence, there is a statement about the probable cause or the probable issue. The third statement is about the action that should be taken. The statement about the probable issue should be specific enough that we, including the sys admins, know that we are referring to this "unknown error" and not some other "unknown error" within Drupal.

I reran the patch 66 - the patch is fine and in good health and it will be complete as soon as we have some sort of meaningful agreement on the phraseology of the error messages.

blacklight4’s picture

For reference, this is how Wordpress writes its "unexpected error" message: "An unexpected error occurred. Something may be wrong with WordPress.org or this server’s configuration. If you continue to have problems, please try the support forums"

The word "unexpected" indicates the possibility of a one-off event like a file corruption. In the second sentence, there is a statement about the probable cause or the probable issue. The third statement is about the action that should be taken. The statement about the probable issue should be specific enough that we, including the sys admins, know that we are referring to this "unknown error" and not some other "unknown error" within Drupal.

I reran the patch 66 - the patch is fine and in good health and it will be complete as soon as we have some sort of meaningful agreement on the phraseology of the error messages.

meeli’s picture

Status: Needs review » Needs work

Why is this issue still in Needs Review? The original issue description (which is very confusing - unknown/unexpected used interchangeably) was fixed and committed by Dries two years ago and now we're getting patches to change it back to what it originally was.

We should either move this discussion to a different issue and close this one, or update the issue summary to actually explain what's going on.

As for the wording - in my opinion this should either be a) configurable or b) more clear in the second sentence. "Please try again later" is a bit vague (try what again? how much later?) and I think will not really encourage people to come back to the site. Simple is better and I think "The website has encountered an error, sorry!" is probably going to be fine.

lokapujya’s picture

Originally, I agreed with comment #34 and think that "unexpected error" is standard usage. So, why change it now in D8? However, since the change was already committed and agreed upon 2 years ago, I'm not against just leaving it as it is now in head. I guess it was assumed that it would be simpler to translate.

We can probably just mark it fixed and close the issue. Sounds like we don't want to back port it. Would be nice to see this issue disappear from the queue.

meeli’s picture

Status: Needs work » Fixed

Marking as fixed.

David_Rothstein’s picture

Status: Fixed » Needs review

What's being proposed here is essentially just a simple rollback of the original committed patch (even though it's been a long time, it's still a rollback). I think keeping it in the same issue is easier for that.

David_Rothstein’s picture

Title: Improve the maintenance page error message » Improve the maintenance page error message (rollback)

David_Rothstein’s picture

I'm uploading a version that is just a pure rollback instead (i.e. this goes back to the exact same wording Drupal 7 has).

The recent history of this:

1. #64 marked the earlier patch RTBC.
2. The concern in #65 was already addressed in #34, then addressed further by @blacklight4 in #68.
3. #68 also pointed out that the "unexpected error" terminology is used by WordPress too.

I think "unexpected error" is a de facto standard on the web and don't see any real rationale in this issue for why Drupal should have deviated from it... I think this should go back to RTBC, but won't set it that way myself since I changed the patch a bit.

nateB’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DrupalConLA

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: unhandled-error-message-1818692-76.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Those test failures look bogus to me - let's try again.

Patrick Storey’s picture

Issue summary: View changes
Issue tags: +Needs beta evaluation

So it looks like we're just re testing here and then this is good to go?

Let's add a beta evaluation in preparation of this being committed.

Reference: https://www.drupal.org/core/beta-changes | Contributor task:

xjm’s picture

Title: Improve the maintenance page error message (rollback) » Improve the maintenance page error message
Status: Reviewed & tested by the community » Closed (won't fix)
Issue tags: -Novice, -Needs beta evaluation

Thanks @David_Rothstein for the summary of what's going on here. I find this extremely confusing to be all in one issue rather than in a followup since the original commit was in 2012 (!) and the issue summary doesn't explain the history, and I would have preferred a followup issue or a straight git revert, but the summary in #76 helped me understand the status.

Since this issue only changes strings (restoring them to the D7 version), it's okay to go in during the beta. I've committed David's rollback patch. And, since that reverts the original change, based on the discussion in this issue, I'm marking it wontfix. If anyone would like to see this fixed in a different way, let's open a new issue.

Thanks everyone for your input here.

David_Rothstein’s picture

For some reason the commit did not get auto-linked here, so here it is for reference: http://cgit.drupalcode.org/drupal/commit/?id=b58943a