[Koha-patches] [PATCH 2/2] Bug 5415 Let calls of SimpleSearch utilize considtent interface

[Clay Fouts]

Hello, Henri.

No, presenting an HTTP 500 error is not appropriate way to handle this
situation. Catching the exception with eval{} is more proper.

With this CLI bulkmarcimport script, yes, it should die if it can't perform
the match. What if the bogus record is the one that matches? Then depending
on the circumstance you could have two identical records floating around
(unexpected and not good) or it could cause an exception somewhere else down
the line when it tries to insert a duplicate key (causing the developer to
have to trace back through who knows what in order to find the source of the
error later). Skimming over the broken record is not a solution!

Clay


On Thu, Nov 18, 2010 at 9:11 AM, LAURENT Henri-Damien <
henridamien.laurent at biblibre.com> wrote:

> Le 18/11/2010 17:37, Clay Fouts a écrit :
> > The die() in the above code is perfectly legitimate because the routine
> > does not know how to handle the error gracefully. I cannot count the
> > number of hours I've had to spend tracking a bug back to its source
> > through all sorts of twists and turns because Koha did not die() when it
> > couldn't figure out how to handle the original error and simply warn()ed
> > about it or, more frequently, opted not to check for errors at all.
> >
> > The good thing about exceptions is that you cannot ignore the error and
> > get away with it. Warn()s can and do get ignored, much to the detriment
> > of developers, users, and everyone in between. Every best practices
> > guide will tell you to die() as soon as you figure out that you can't
> > accommodate the nature of the error, precisely because it is ugly and
> > cannot be ignored, forcing the developer to handle it somehow at a
> > higher level. Programmers are lazy. Unless we compel ourselves to
> > address errors, it's far too easy to ignore them, so we will (and do, if
> > Koha's code is to serve as an example). Exceptions demand better coding
> > practicing.
> ok. But
> >
> > Koha already has a CGI-friendly die() handler with CGI::Carp. If how it
> > is presented to the user is a concern, the template for that can be
> > prettified. Nevertheless, it's a favor to everyone not to let errors
> > propagate throughout the system. It corrupts data and gives the false
> > perception that everything is behaving correctly.
> >
> > Cheers,
> > Clay
> Hi Clay.
> So If in a result list or in a bulkedition, there is one biblio which
> can't be decoded (because XML::Parser fails), you are in favor of
> returning Error 500 to the user ?
> I am not. You may think it hides problems. But if for one record which
> has a problem, you throw away all the results by breaking at the first
> error, then you end up with having to make kind of a hell of a work only
> to find where the error might be and even worse, make the user feel very
> uncomfortable with your tool... (Just look at the way zebra is dying
> when you have an error with the indexing process. I just think it is not
> a sane way to do things.) Yes raise an error, Yes warn that error AND
> provide information about the context (biblionumber if it is about
> biblios). But No, donot simply die... because when running a
> bulkmarcimport on a 100000 records, having to do things over and over
> again for 10 records with an error is just insane. Or Please, please,
> die in functions, AND add tests to whatever script or function which is
> using this other function and try to provide correct information.
>
> > Exceptions demand better coding
> > practicing.
> I fully agree though. Should we begin some kind of New module for that ?
> Croak could be good, but we would have to make things more failsafe and
> not fail first error of an array of task.
>
>
> Friendly.
> --
> Henri-Damien LAURENT
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20101118/8f18a8a8/attachment.htm>