Error handling in bugzilla

[Bradley Baetz]

Currently, bugzilla uses CGI::Carp to report/log errors. This has a number 
of problems:

a) Newer versions of CGI::Carp override CORE::GLOBAL::die rather than
using $::SIG{__DIE__} (to better work with mod_perl). The problem is that
for this to work, it must happen at compile time, but we bring this in at 
runtime (via require), so it doesn't work for anything compiled before 
CGI::Carp is brought in. This is mostly unnoticed, since we don't use die 
directly in our .cgis, but we may do so via an intermediate |use|

b) This only applies to CGIs (hence the name). This is why we get the 
errorlog style error messages from checksetup if the error occurs afer 
globals.pl is brought in, or we did when CGI::Carp used the 
$::SIG{__DIE__} version, anyway.

c) It doesn't work with mod_perl. Both forms of the die stuff happen even
if we're in an eval. This was the cause of the File::Find/File::Temp issue
which people were having - File::Temp tried to get some comnstants which
didn't always exist, so it used an eval. But CGI::Carp caught the die, and
exited it, before the eval could work. Newer CGI::Carp versions work
arround this by checkinbg $^S, and doing nothing if we were in an eval.  
mod_perl code runs in an eval, so it doens't work. Even newer versions of
CGI::Carp try to work arround this too. They do this by constructing a
backtrace (via Carp, effectively), and checking if the _text_ looks
mod_perlish, by |=~ /eval [\{\']/m|. 

d) It only works with CGIs. It doesn't work with commandline programs, 
mailed in stuff, or xmlrpc. Fundamentally, it can't.

e) The code relies on a global variable, $::vars->{header_done}. mod_perl
doesn't like global vars. OK, in this case its easy to fix, but since we
already have a Bugzilla::CGI class (or will RSN), and it knows if we've
sent anything, it makes sense to use that. Plus, only 1.5 scripts (the
buglist.cgi usage is almost certainly incorrect) use it, and we're going 
to rewrite process_bug anyway, right? :)

So I'd like to propose a new way of doing this. For motivation, see 
http://axkit.org/docs/presentations/tpc2001/exceptions.axp/a.pdf (and the 
2002 version of the same)

Basic idea:

- all errors, of any sort, are handled as exceptions.
- Said exceptions are Exception::Class subclasses, probably.
- die calls from other, non-bugzilla modules are made into objects via a 
CORE::GLOBAL::die handler (see the above pdf for an example)
- throw{user,code}error become exceptions, whose 'what do I do to ouput as 
html' method becomes a call to TT. See the patch for how this (roughly) 
will work. This has the advantage that they're catchable, so we could do 
stuff like run a set of queries, without dying on the first invalid one, 
and so on.
- all scripts are wrapped in an eval block, with a postscript of:

if ($@) {
   Bugzilla::CGI->HandleError($@);
}

or something like that, anyway. (Actually, probably $cgi->handle_error($@) 
instead). This is the ugly part, but we can't really do anything else, and 
still handle mod_perl.

- Plain text errors are handled by rearranging the 
{user,code}-error.html.tmpl stuff so that it can be used from a .txt, too. 
This is something to be done in teh future, not something I want to do 
now.

Its not quite as simple as that, because we need to work arround some perl 
bugs. The first affects the debug output from TT, and affects all versions 
of perl, when using CORE::GLOBAL::die to do this - see 
http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2002-09/msg01204.html
I've made a patch to fix TT - see 
http://template-toolkit.org/pipermail/templates/2002-October/003779.html
This only affects TT's error cases, though, making the error message 
useless/uninformative/etc. This is fixed in perl-5.9-devel, but the only 
workarround is to use $SIG{__DIE__} instead, or to use extra brackets in 
the die statement.

The second bug only affects perl 5.8, using $SIG{__DIE__}. Exception 
objects are stringified, making them basically useless. See 
http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2002-10/msg00271.html, 
which hasn't yet been dealt with. The work arround for this is to use 
CORE::GLOBAL::die.

So we have two almost mutually exclusive workarrounds, which leads to the
above TT patch being required. Ain't perl fun....

I've attached a really really really hacky patch to bug 173626. It doesn't
do the eval thing, or the compile time thing (ie, it doesn't actually
work). It has lots of XXX comments, with things which may/should change.  
That code assumes that we can get this to work (at least for the moment)
w/o the eval-round-each-script thing. I'm not convinced that this is the
case any more, though, or that we should try.

The other thing is that if Bugzilla::Template reports errors via
Bugzilla::Exception, but Bugzilla::Exception uses tempalates for format
the error, then we have a circular dependancy.... The fix for this is
probably to not have Bugzilla::Exception objects use templates at all, but
rather define all the exceptions so that they have a template name/tag
which we can then process in Bugzilla::CGI/Bugzilla::xmlrpc/etc. I think 
that thats the best way, but I haven't thought it through entirely

Thoughts?


Bradley