[dev] Previous exceptions for Horde_Exception

[Gunnar Wrobel]

Hi!

This morning I moved some functionality from the Horde_Exception class  
into a separate Horde_Exception_Prior class and modified all calls  
where we provided Horde_Exception with either a PEAR error or an  
Exception as first argument.

I did this in order to support previous exceptions as described in  
this proposal:  
http://framework.zend.com/wiki/display/ZFPROP/previous+Exception+on+Zend_Exception+-+Marc+Bennewitz

Jan did not like the approach of introducing a new Exception class so  
I wanted to ask for feedback. I attach the log of the discussion I had  
with one. It should already cover some of the pros and cons.

Cheers,

Gunnar

-------------- next part --------------
10:55 <wrobel> yunosh: if possible don't place those in the default
               exception. I'm currently cleaning that one up to be not
               more than the standard PHP exception.
10:58 <yunosh> uh why? that's one of the important things about
               Horde_Exception, beside having a common base exception:
               to implement some convenience stuff that's missing from
               the php exception
11:00 <wrobel> sure, that is fine. but the current Horde_Exception
               tries to determine what we actually throw at it via the
               arguments. In all cases we know what we are providing
               there and so we can have a separate class for that that
               won't need to guess
11:01 <wrobel> We currently misuse the message argument by allowing
               PEAR Errors and Exceptions there. Both should be prior
               exceptions however
11:03 <yunosh> what's the problem with that? that's php-style, this is
               not java ;)
11:03 <yunosh> do you want separate exception classes for that?
11:04 <wrobel> yes. Primarily because I want to be able to use the PHP
               5.3 concept of prior exceptions
11:05 <yunosh> that won't work, because due to missing
               multiple-inheritance in php, you can't have an
               exception that uses one of those classes in the
               applications. the special purpose exceptions like
               NotFound are already a compromise
11:06 <yunosh> but each application and each library should use it's
               own exceptions, extending Horde_Exception
11:09 <wrobel> Horde_Exception_Prior extends Horde_Exception. And I
               ensured that the applications extend this class.
11:10 <wrobel> The PEAR handling should go away later anyhow. And if
               we catch an exception and rethrow it by stuffing it
               into Horde_Exception, then we should actually use the
               prior exception approach.
11:10 <yunosh> but if anything is using Horde_Exception_Prior anyway,
               why having this as a separate exception at all?
11:10 <wrobel> Not all applications or libraries need that
11:11 <wrobel> For many the base exception is quite fine
11:11 <wrobel> if you library does not handle PEAR errors or rethrows
               exception from other libraries I don't see why it
               should use Horde_Exception_Prior
11:13 <yunosh> and still i don't see why you don't want this in the
               base exception. it's not a huge overhead or something
11:14 <wrobel> because we want to support < PHP 5.3. And that one does
               not have prior exceptions. It is not too hard to add it
               though
11:14 <wrobel> but it has to happen in Horde_Exception
11:14 <wrobel> and I don't wan't to duplicate the code lines we had in
               there
11:14 <wrobel>
               http://framework.zend.com/wiki/display/ZFPROP/previous+Exception+on+Zend_Exception+-+Marc+Bennewitz
11:14 <yunosh> sorry, i lost you. can you point me to some php docs?
11:15 <wrobel> see link
11:20 <yunosh> are getPrevious or __toString() final methods in php
               5.3?
11:21 <wrobel> yes
11:21 <yunosh> yeah getPrevious is
11:21 <wrobel> right, __toString not
11:22 <yunosh> we could still add this unconditionally to the base
               exception if we use a different method than getPrevious
11:22 <yunosh> probably not the best stlye though
11:23 <yunosh> it doesn't even mention in the docs that it's php 5.3
               only. awesome :/
11:24 <wrobel> I had the impression that Horde_Exception was being
               forced into being a bit of a garbage dump, too. we had
               several places where the constructor arguments were
               used in a way that information was lost. 
11:25 <wrobel> so I think using several classes also makes it clearer
               to us how we should actually use it.
11:25 <yunosh> yeah, but that was just oversights from the
               conversion. and the missing concept of user informatino
               in the horde exception
11:25 <wrobel> yeah, true :)
11:27 <yunosh> i'm still not really convinced because if we have
               separate exception classes (iiuc you also want one for
               accepting user info), you still have to decide  which
               one to extend in the library/app
11:28 <yunosh> i might get convinced to introduce the prior exception,
               to only add the additional methods/attributes for php
               5.2
11:29 <wrobel> I'm fine with actually having one class doing all the
               stuff we need in the applications. So if that means
               adding user info, fine.
11:30 <wrobel> I did create a separate class for what chuck was doing
               with error_get_last(). We only use this in a few
               specific packages where we did not handle PEAR errors
               or Exceptions at the same time
11:31 <wrobel> so I thought it is fine to handle this with a different
               class there
11:32 <yunosh> it is fine as longs as this doesn't change. but if you
               want to handle both or all three in the future, you are
               lost, because you can't extend your library exception
               from the two different exceptions
11:32 <wrobel> I think it also depends on where you are: In a library
               I actually want clean basic exceptions that should
               catch exceptions from libraries they use and rethrow
               them as their own. applications are a different matter
               though
11:32 <yunosh> sure
11:33 <wrobel> if we have tons of error_get_last in the applications
               in the future, sure, lets add this to the general
               exception used in applications
11:34 <yunosh> you might want to discuss this on the developer list to
               get more opinions, but i'm still for having most
               flexibility in the base class and only mix-in stuff
               that we need for php 5.2
11:36 <wrobel> I already pushed the things this morning but I'll send
               a mail to the dev list to get more feedback
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digitale PGP-Unterschrift
URL: <http://lists.horde.org/archives/dev/attachments/20100211/72b347af/attachment.bin>