[dev] Dealing with Exceptions

[Gunnar Wrobel]

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

> Zitat von Gunnar Wrobel <wrobel at horde.org>:
>
>>
>> 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
>
> Maybe there's some misunderstanding. We don't need the consistency  
> when catching and using Exceptions, but when *throwing* exceptions.  
> And the base Exception is not offering the convenience wrappers that  
> Horde_Exception_Prior provides.
>

I was referring to the throwing of exceptions, yes. The lack of the  
convenience wrappers was what I considered a potential drawback. But  
to me this is less important than the dependency that I feel is not  
required. And I would like to see us developing libraries that only  
throw exceptions specific to that library.

> I don't have any problems with all Horde libraries requiring a  
> dependency on the Horde_Exception package either.

So, as everyone seems to be okay with adding this dependency I'll  
probably better shut up now - ... and try to reopen it once PHP 5.2 is  
a thing of the past ;)

Thanks for the feedback.

Gunnar

-- 
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