Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
- Determine the translatability in both cases.
- Decide on the language to be used.
- 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?
Comment | File | Size | Author |
---|---|---|---|
#76 | unhandled-error-message-1818692-76.patch | 1.55 KB | David_Rothstein |
#66 | unhandled-error-message-1818692-66.patch | 1.7 KB | neelam.chaudhary |
#61 | 1818692-60.patch | 1.65 KB | lokapujya |
#58 | 1818692-58.patch | 1.67 KB | lokapujya |
#56 | 1818692-56.patch | 1.55 KB | lokapujya |
Comments
Comment #1
chx CreditAttribution: chx commentedI 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.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedRather 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.
Comment #3
mbrett5062 CreditAttribution: mbrett5062 commentedWould 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.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedHere testbot.
Comment #5
mbrett5062 CreditAttribution: mbrett5062 commentedSend for test, and review.
Comment #7
chx CreditAttribution: chx commentedI 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.
Comment #8
mbrett5062 CreditAttribution: mbrett5062 commentedOK try that again.
Comment #9
mbrett5062 CreditAttribution: mbrett5062 commentedOK, will fix that and send again.
Comment #10
mbrett5062 CreditAttribution: mbrett5062 commentedHere is the fix. Should pass tests!
Comment #11
mbrett5062 CreditAttribution: mbrett5062 commentedComment #13
shadcn CreditAttribution: shadcn commentedHow about some helpful messages? Like "If you're the site admin, check your logs". Something along those lines.
Comment #14
ksenzeeRemember 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."
Comment #15
mbrett5062 CreditAttribution: mbrett5062 commentedOK 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.
Comment #17
mbrett5062 CreditAttribution: mbrett5062 commentedOK, I am new to this. I think I know what I did wrong. One more go.
Comment #19
mbrett5062 CreditAttribution: mbrett5062 commentedLast try, before I defer to an adult.
Missed the full stop off end of message.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedChx, 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?
Comment #22
mbrett5062 CreditAttribution: mbrett5062 commentedThanks @ernie that was indeed the problem. Recreated patch with git bash instead of eclipse IDE.
Comment #23
chx CreditAttribution: chx commentedGreat.
Comment #24
chx CreditAttribution: chx commentedTagging for backport.
Comment #25
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #26
mbrett5062 CreditAttribution: mbrett5062 commentedWorking on D7 backport now.
Comment #27
mbrett5062 CreditAttribution: mbrett5062 commentedD7 backport patch attached.
Comment #28
chx CreditAttribution: chx commentedGood to go as well.
Comment #29
xjmUm, 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.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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...
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedOops, that was a cross-post, but same idea :)
Comment #32
xjmReference: http://drupal.org/node/1527558
Comment #33
xjmHa. :)
Setting back to fixed for D8 based on all the nice crosspostage above.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedActually, 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!" :)
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commentedComment #36
xjmComment #37
mbrett5062 CreditAttribution: mbrett5062 commentedHow 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
Comment #38
mbrett5062 CreditAttribution: mbrett5062 commentedRevised 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.
Comment #40
xjmThere 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. :)
Comment #41
mbrett5062 CreditAttribution: mbrett5062 commentedUnable to continue working on this so opening for someone else to complete.
Comment #42
chrisfromredfinOK, 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).
Comment #44
chrisfromredfinRe-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.
Comment #45
chrisfromredfin42: 1818692-unhandled_error_message-42.patch queued for re-testing.
Comment #46
parthipanramesh CreditAttribution: parthipanramesh commentedSeems to be ok. Better message now. Thank you!
Comment #47
lokapujyaDid we deal with #1808864: Exception handling is not 'locale safe' (thus not database safe)?
Comment #48
adnanc CreditAttribution: adnanc commentedReviewed. 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.
Comment #49
XanoThe last patch no longer applies and needs a re-roll.
Comment #50
InternetDevels CreditAttribution: InternetDevels commentedRerolled patch.
Comment #52
bohart50: unhandled_error_message-1818692-50.patch queued for re-testing.
Comment #54
ipo4ka704 CreditAttribution: ipo4ka704 commented50: unhandled_error_message-1818692-50.patch queued for re-testing.
Comment #55
dawehnerIsn'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.
Comment #56
lokapujyaWe 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.
Comment #57
raj.nitdgp CreditAttribution: raj.nitdgp commentedIts 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.
Comment #58
lokapujyaThanks raj.nitdgp. Rerolled.
Comment #59
vonn.new CreditAttribution: vonn.new commented#58 Patch did not pass on SimplyTest.me
Error msg: An error occurred while patching the project.
Comment #60
vonn.new CreditAttribution: vonn.new commentedComment #61
lokapujyaIf we aren't going to handle the error, we should call it an unexpected error as in D7.
Comment #62
vonn.new CreditAttribution: vonn.new commentedpatch in #61 passes simplytest.me
Comment #63
holist CreditAttribution: holist commentedI'm looking at this.
Comment #64
holist CreditAttribution: holist commentedPatch looks OK to me.
Comment #65
alexpottI agree with @chx. I'm not sure what this patch is fixing. I think the text in HEAD is fine.
Comment #66
neelam.chaudhary CreditAttribution: neelam.chaudhary commentedReroll of #61
Comment #68
blacklight4 CreditAttribution: blacklight4 commentedFor 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.
Comment #69
blacklight4 CreditAttribution: blacklight4 commentedFor 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.
Comment #70
meeli CreditAttribution: meeli commentedWhy 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.
Comment #71
lokapujyaOriginally, 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.
Comment #72
meeli CreditAttribution: meeli commentedMarking as fixed.
Comment #73
David_Rothstein CreditAttribution: David_Rothstein commentedWhat'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.
Comment #74
David_Rothstein CreditAttribution: David_Rothstein commentedComment #76
David_Rothstein CreditAttribution: David_Rothstein commentedI'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.
Comment #77
nateB CreditAttribution: nateB as a volunteer commentedComment #80
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThose test failures look bogus to me - let's try again.
Comment #81
Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commentedSo 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:
Comment #82
xjmThanks @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.
Comment #83
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedFor some reason the commit did not get auto-linked here, so here it is for reference: http://cgit.drupalcode.org/drupal/commit/?id=b58943a