[dev] Dealing with Exceptions

[Gunnar Wrobel]

Zitat von Jan Schneider <jan at horde.org>:

> Zitat von Gunnar Wrobel <wrobel at horde.org>:
>
>> Hi Jan,
>>
>> we already briefly discussed Exceptions on IRC and decided to  
>> continue on the mailing (has been a while back, please forgive me  
>> for the rather long delay on the topic).
>>
>>
>> Zitat von Jan Schneider <jan at horde.org>:
>>
>>> The branch "master" has been updated.
>>> The following is a summary of the commits.
>>>
>>> from: 1e943c0937d592233379d8cac82b89f80861b11c
>>>
>>> 43b8c54 Fix package name.
>>> 7390733 Here too.
>>> e627e65 Fix directory name, remove outdated tests.
>>> f2b35e7 Consistently extend exception classes from  
>>> Horde_Exception_Prior. CS, cleanup.
>>
>> Why should there be any need for a low level framework package to  
>> extend from Horde_Exception_Prior?
>>
>> On IRC you mentioned that
>>
>> 1) this should be done so that re-throwing Exceptions can be done  
>> on both 5.2 and 5.3 in a decent way (using the third argument of  
>> the Horde_Exception constructor, named "previous")
>>
>> 2) it is convenient to allow throwing anything as the first  
>> argument at the constructor (which is what Horde_Exception_Prior  
>> does on top of Horde_Exception)
>>
>> 3) "this has to be consistent"
>>
>> I have been looking at the Kolab_Format package the last week and I  
>> must say that I see no reason for it to extend from  
>> Horde_Exception_Prior.
>>
>> It does not re-throw any exceptions as it is at the base of the  
>> hierarchy. So Kolab_Format has no specific need to extend from  
>> Horde_Exception. It would not use any of its functionality.
>>
>> The package also does not require any convenience methods when  
>> constructing new exceptions. The only exception parameter is  
>> usually just the message string. So Kolab_Format has no specific  
>> need to extend from Horde_Exception_Prior either. It would not use  
>> any of its functionality.
>>
>> This leaves "consistency" which is what I'd like to understand  
>> first. My assumption is that this means that it should be  
>> consistent on the application level. So that we can rely on  
>> catching "Horde_Exception_Prior" and be certain that we can catch  
>> anything coming from the underlying framework that way.
>>
>> If that is what you refer to with "this has to be consistent" then  
>> I agree to the point that we need to know what to catch on the  
>> application level. A generic "catch (Exception $e)" might not be a  
>> good thing. But I would say that this still does not require all  
>> framework libraries to extend from the same exception.
>>
>> Kolab_Format is at the base of the hierarchy. So it should not care  
>> or know about the application level. Throwing a  
>> Horde_Exception_Prior because some upper level might need it seems  
>> like a problematic design choice to me. Kolab_Format should care  
>> about its own business and ensure that it handles its exceptions in  
>> a decent way.
>>
>> In case there is another layer on top using Kolab_Format it should  
>> definitely ensure that it is able to deal with all Exceptions that  
>> might be thrown from Kolab_Format. This would refer to  
>> Kolab_Storage for example which is the main consumer for  
>> Kolab_Format. It should always only throw its own type of  
>> exceptions. If it would run into situations where a  
>> Horde_Kolab_Format_Exception bubbles to the surface I would  
>> consider this to be a bug of Kolab_Storage.
>>
>> Now Kolab_Storage could decide to extend from Horde_Exception_Prior  
>> or it could happen in an exception class of a package even further  
>> up in the chain. Nearer to the application level.
>>
>> Kolab_Format is a good example to me because it is definitely used  
>> stand-alone outside of any Horde context. And I don't see any  
>> reason to force other consumers to pull in the Horde_Exception  
>> package if that is not required.
>>
>> An additional remark: We should of course rename  
>> Horde_Exception_Prior because I chose a real bad name for that  
>> class. May "Horde_Exception_Base" or something like that would be  
>> more appropriate.
>
> First of all, I still think it makes sense to have a common base  
> exception class for all horde code, so that consumers (whether those  
> are horde applications or external code using some horde library)  
> can implement some catch-all for any exceptions thrown by horde code.

As I argued above such a catch-all wouldn't be necessary if the  
libraries only throw exceptions specific to this library.

> I see that it makes sense to throw special purpose PHP/SPL  
> extensions though, and we actually do this, which is inconsistent  
> again. But that's not the inconsistency I care about right now.
>
> What I mean with consistency is that all horde exceptions should  
> behave the same way. You may know that Horde_Kolab_Format_Exception  
> is never going to wrap another extension/php error/pear error. But I  
> don't. We have so many libraries, each using its own exception  
> class, and I don't want do look up each exception's class definition  
> to see how I have to use it, when I'm going to fix/code something in  
> some arbitrary library. I want to be sure that any Horde exception  
> class is working the same, so I can wrap it around any of the  
> errors/exceptions that might occur during PHP development.
>
> This helps a lot during development and it doesn't add much overhead  
> either. All library exceptions should extend a common horde  
> exception anyway, and whether this is Horde_Exception or  
> Horde_Exception_Prior doesn't really matter.

If that is the consistency we seek (and Micheal S. confirmed that he  
has similar needs) then I would say the most consistent interface we  
can get is the one the base PHP Exception class provides. This is even  
known to all PHP developers that don't know Horde. And in case a  
package needs to use the feature of previous exceptions it must extend  
Horde_Exception as we want to support PHP 5.2.

The drawbacks I see with this approach:

  - You can't catch Exceptions generically. But for all I know this is  
only necessary if the called library has issues and passes exceptions  
from lower levels without catching them. This basically repeats my  
initial argument on how layered exceptions should work.

  - When using previous exceptions you always have to set a message  
and a code. You can't omit that and if you want to reuse the original  
message and code you have to use "throw new  
Horde_Xyz_Exception($e->getMessage(), $e->getCode(), $e);". I believe  
in most cases it actually makes sense to use a new message and a new  
code as the context is different from the original source of the error.

The advantages I see:

  - no unnecessary dependencies

  - forces us to use a decent layered exception approach

  - very consistent exception interface known to all devs familiar with PHP5

Cheers,

Gunnar



>
> Jan.
>
> -- 
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/
>
> -- 
> Horde developers mailing list
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe at lists.horde.org

-- 
Core Developer
The Horde Project

e: wrobel at horde.org
t: +49 700 6245 0000
w: http://www.horde.org

pgp: 9703 43BE
tweets: http://twitter.com/pardus_de
blog: http://log.pardus.de