Closed Bug 126456 Opened 23 years ago Closed 23 years ago

Fix our error handling

Categories

(Bugzilla :: Bugzilla-General, defect)

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.15
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: gerv, Assigned: gerv)

References

Details

Attachments

(2 files, 4 obsolete files)

Patch v.4
(deleted), patch
Details | Diff | Splinter Review
Patch2 v.2
(deleted), patch
myk
: review+
myk
: review+
Details | Diff | Splinter Review 
We should implement and use something like the following before 2.16. This is to
get content-type headers right, and pave the way for l10n.

UserError("message"). Prints content type, and uses a template to format the
error message. Called anywhere in any CGIs.

CodeError("message", hashref). Also prints a content-type and attempts to use a
template, but falls back on a non-template error message if that fails.
Optionally logs info to a file for diagnosis of bugs. hashref is an optional ref
to a hash; the log would print the contents as:
foo: bar
baz: 73

TemplateError("content-type"). Does not print a content-type, but produces the
correct sort of "template failed" error message for the given content-type. This
should be in the || arm of all template calls, and nowhere else.

See also bug 100092.

Gerv
We need to do this before 2.16 - the double-printing of "Content-Type:
text/html" is very ugly.

Gerv
Blocks: 100092
Severity: normal → blocker
Target Milestone: --- → Bugzilla 2.16
Attached patch Patch v.1 (obsolete) (deleted) — — Splinter Review 
This implements the above scheme, with a few extra frills. I anticipate us
moving over to use these functions gradually.

If this gets checked in soon, the still-not-checked-in templates can use it.

Gerv
I like that.
There may be admins who would like to use different templates for UserError and
CodeError, to give different visual signs to the user. So shouldn't we use two
different templates for them? This would get rid of some IFs in the template
code. (Of course, it would still duplicates some code, but that's mostly code
which is duplicated in all templates anyway.)

Shouldn't the text about CodeErrors ask the user about providing the URL
together with the page? Or is the URL always included somewhere in the error
message?
Attached patch Patch v.2 (obsolete) (deleted) — — Splinter Review 
Afranke's comments addressed. Looking for re-review.

Gerv
Attachment #70793 - Attachment is obsolete: true
You have r=afranke if you answer the following questions:

+  $template->process("global/error.html.tmpl", $vars)
Shouldn't all of these use "user-error.html.tmpl" or "code-error.html.tmpl"?
(4 occurrences).

+  $vars->{'code_error'} = 1;
Is this still necessary?

+      <p>Template->process() failed: $error.</p></tt>";
Doesn't $error need to be html-escaped?

This still needs to be tested I guess, and you need a second review from someone
who is familiar with current usage of DisplayError and PuntTryAgain and knows
the perl and template hacking guides.

This is really strange. Mozilla is happy to attach my patches to every other bug
in b.m.o, but not this one. It just sits there and spins. I can't figure out why.

Gerv
Attached patch Patch v.3 (obsolete) (deleted) — — Splinter Review 
Yeah, sorry, I botched that. I've fixed the lot. Escaping $error is "better
safe than sorry".

Gerv
Attachment #71065 - Attachment is obsolete: true
And 4.x will do it fine. Curiouser and curiouser...

Gerv
Gervase Markham wrote in comment #0:
> This is to get content-type headers right, and pave the way for l10n.
For l10n of the Content-Type headers see also bug 126266


There are some bug-activity changes in this patch; I ignored them. r=afranke.
Still needs testing and a look from someone who uses the current routines.
-> patch author
Assignee: justdave → gerv
Keywords: patch, review
Comment on attachment 71201 [details] [diff] [review]
Patch v.3

You've replaced params with hard coded text, but left the params in place.

Do you need to have checksetup convert the params into a custom template, or
are we not bothering with that?
Attachment #71201 - Flags: review-
> You've replaced params with hard coded text, but left the params in place.
> 
> Do you need to have checksetup convert the params into a custom template, or
> are we not bothering with that?

There's a bug on removing all the obsolete params at once. You raise a good
point about migration; I'm not sure what we are doing there. Apart from these
admin issues, do you have any problems with the code?

Gerv


No, if you can point me to a bug# on that, then r=bbaetz. Not that I could
trigger half those errors to test, mind you.
Removing old template parameters is bug 100089.

For migration, I think the best solution is merely to dump the contents of all
the params to a file. Those admins who don't change that stuff can then ignore
it, and those that do have a basis to merge into the templates.

Any sort of auto-merging would be hard to do and probably more hassle for the
admin than doing it themselves, as they'd have to make sure we hadn't messed it up.

If you agree with that, can I have r=bbaetz?

Gerv
Comment on attachment 71201 [details] [diff] [review]
Patch v.3

r=bbaetz
Attachment #71201 - Flags: review- → review+
Comment on attachment 71201 [details] [diff] [review]
Patch v.3

Index: CGI.pl

>+sub CodeError {
>+sub UserError {
>+sub TemplateError {

Nit: The purpose of these functions would be clearer if their names
began with a verb.


> sub PuntTryAgain ($) {

Nit: You could reduce code redundancy by making PuntTryAgain call UserError
instead of replicating its functionality.


>Index: template/default/global/code-error.html.tmpl

>+Variables:
>+  [% FOREACH key = variables.keys %]
>+    [%+ key %]: [%+ variables.$key %]
>+  [% END %]
>+</pre>

Nit: Either the word "Variables:" should only display if there are variables,
or "None" should be printed next to it if there aren't any variables.

All issues are nits.  r=myk
Attachment #71201 - Flags: review+
> >+sub CodeError {
> >+sub UserError {
> >+sub TemplateError {
> 
> Nit: The purpose of these functions would be clearer if their names
> began with a verb.

Do you (or anyone else) have suggestions?

> > sub PuntTryAgain ($) {
> 
> Nit: You could reduce code redundancy by making PuntTryAgain call UserError
> instead of replicating its functionality.

I know there's a reason I didn't do this - but I can't remember what it is now.

Gerv
>Do you (or anyone else) have suggestions?

Here are a few:

(Display|Show|Throw|Die)(User|Code|Template)Error

Of these, I like "Throw" best because it is correct English and implies the end
of processing that occurs when these functions call "exit".  "Die" is actually
the most meaningful in this regard but not as correct linguistically.

Don't know if it is of any use, but Mozart-Oz uses the term "raise" for
exceptions.
I'd like to avoid using "Exception"-style terminology if possible; exceptions
are very specific things, and these are not them. Both "raise" and "throw" have
this problem. These are errors. 

DieWithTemplateError()?

Gerv
Sounds good.
Attached patch Patch v.4 (deleted) — — Splinter Review 
I reconsidered and used "Throw" :-) I also added a script which will add the
URL to the page body if JS is enabled. The changes are eversoslightly above
trivial, so a quick re-review is required.

Gerv
Attachment #71201 - Attachment is obsolete: true
Let's make this easier: Check in version 3, then attach another patch converting
the names and adding the javascript.  That patch should be small enough to get a
review x 2, while this new patch (even with interdiff) will not and is more
painful to review.
Attached patch Patch2 v.1 (obsolete) (deleted) — — Splinter Review 
Extra changes - get the function names right, and add the <script> to add the
URL to the error page.

Gerv
Comment on attachment 77542 [details] [diff] [review]
Patch2 v.1

this looks good!
Attachment #77542 - Flags: review+
Comment on attachment 77542 [details] [diff] [review]
Patch2 v.1

>+        document.write("<p>URL: " . document.location . "</p>");

Use plus signs (+) to concatenate in JavaScript.

>- <p>
>-   <tt>
>-     Bugzilla has suffered an internal error. Please save this page and send
>-     it, and its URL, to [% Param("maintainer") %], with details of what you
>-     were doing at the time this message appeared.
>-   </tt>
>- </p>
>+<tt>
>+  <p>
>+    Bugzilla has suffered an internal error. Please save this page and send
>+    it to [% Param("maintainer") %] with details of what you were doing at 
>+    the time this message appeared.
>+  </p>
>+  <script> <!--
>+    document.write("<p>URL: " . document.location . "</p>");
>+  // -->
>+  </script>
>+</tt>

Why use <tt> here?
Attachment #77542 - Flags: review-
Plus signs fixed.

> Why use <tt>?

Because it's more "error-message"-like, and people are less likely to just hit
Back and ignore it.

Gerv
>Plus signs fixed.

Was an attachment supposed to be posted along with this comment?

>Because it's more "error-message"-like, and people are less likely to just hit
>Back and ignore it.

On the other hand, it's much less obtrusive than big text on a red background,
making it harder to read, and I can't imagine that we have a problem with people
hitting the back button and ignoring error messages.
> >Plus signs fixed.
> 
> Was an attachment supposed to be posted along with this comment?

No - you have to use your imagination :-)

> >Because it's more "error-message"-like, and people are less likely to just 
> > hit Back and ignore it.
> 
> On the other hand, it's much less obtrusive than big text on a red background,
> making it harder to read, and I can't imagine that we have a problem with 
> people hitting the back button and ignoring error messages.

So what are you saying? The entire message should be in the big-red-text style?

Gerv


>No - you have to use your imagination :-)

Show me the source, Luke!

>So what are you saying? The entire message should be in the big-red-text style?

Yes, the entire message in the big-red-text style, and another bug report filed
for a UI review of the error messages, is what *should* happen.  However, the
current patch, even with <tt>, is good enough and should go in, after which we
can file either a regression or a "UI review" bug report for it.

So, attach a new patch with the plus signs and let's get this bug resolved.
Attached patch Patch2 v.2 (deleted) — — Splinter Review 
The light side of the source.

Gerv
Attachment #77542 - Attachment is obsolete: true
Comment on attachment 77807 [details] [diff] [review]
Patch2 v.2

looks good. r=myk x 2
Attachment #77807 - Flags: review+
Fixed.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.143; previous revision: 1.142
done
Checking in template/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.3; previous revision: 1.2
done

Gerv
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: