[dev] [commits] Horde branch master updated. 3eb04cb5d8972b124aed95ed568d056c8e980dc3

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael M Slusarz <slusarz at horde.org>:
>
>> Quoting Jan Schneider <jan at horde.org>:
>>
>>> Zitat von Michael M Slusarz <slusarz at horde.org>:
>>>
>>>> Quoting Jan Schneider <jan at horde.org>:
>>>>
>>>>>> commit 3eb04cb5d8972b124aed95ed568d056c8e980dc3
>>>>>> Author: Michael M Slusarz <slusarz at horde.org>
>>>>>> Date:   Mon Feb 6 15:32:05 2012 -0700
>>>>>>
>>>>>> Add Horde_Core_Alarm_Handler_Notify class
>>>>>>
>>>>>> The injector passed to the Alarm Notify class requires that create()
>>>>>> have no arguments.  However, the factory class requires that create()
>>>>>> have a Horde_Injector argument.
>>>>>>
>>>>>> The alternative is to create a wrapper object that wraps the create()
>>>>>> method and adds the argument.  But at that point, we might as well
>>>>>> create a local Horde_Core driver.
>>>>>>
>>>>>> Wondering if we should do this for the Mail and Desktop drivers also.
>>>>>>
>>>>>> framework/Core/lib/Horde/Core/Alarm/Handler/Notify.php |   79  
>>>>>> ++++++++++++++++
>>>>>> framework/Core/lib/Horde/Core/Factory/Alarm.php        |    8 +-
>>>>>> framework/Core/package.xml                             |   12 ++-
>>>>>> 3 files changed, 91 insertions(+), 8 deletions(-)
>>>>>> create mode 100644  
>>>>>> framework/Core/lib/Horde/Core/Alarm/Handler/Notify.php
>>>>>>
>>>>>> http://git.horde.org/horde-git/-/commit/3eb04cb5d8972b124aed95ed568d056c8e980dc3
>>>>>
>>>>> To me it looks like the original intention was to have  
>>>>> Horde_Core_Factory_Notification extend Horde_Core_Factory_Base  
>>>>> so that create() does indeed not require a parameter. Is there  
>>>>> anything speaking against this?
>>>>
>>>> That would be one solution, but adding another Factory with a  
>>>> different injector style just to pass to the Alarm Handler is  
>>>> more confusing than just moving the essentially Horde-specific  
>>>> Alarm handler to Core.
>>>
>>> Yes, that won't make sense, I was rather thinking of replacing the  
>>> existing factory. I don't think we call create() on Factory_Base  
>>> factories explicitly anyway.
>>
>> Are we talking about H5?  Because this would obviously be a BC API  
>> break in H4.
>
> Technically yes, because the factories had different parent classes.  
> Practically not because they have the same API, or actually the new  
> factory had the "corrected" create() API.

I don't care too much about this, especially since this should now be  
a moot point with the major version bump.

> It's an edge case though, and strictly this would be a BC break. We  
> might have precedence with with the Horde_Mail Rfc822 objects  
> returned by some of the methods that still evaluate to arrays that  
> have been returned by earlier versions of the API.

Yeah - this is the only BC-breaking element of these kind of changes.   
I made the decision that this did not warrant holding back, since this  
would very rarely be used in practice: if the API tells you that an  
array would be used, doing an is_array() call is pointless.   
Previously, when we possibly returned PEAR_Error objects also, this  
may have been an issue.  But now with Exceptions you should never need  
to do type checking on return values.

In a truly correct world, we should probably have all of our framework  
methods return objects instead of arrays; that we we could always make  
BC-appropriate changes in the future.  But this is not practical due  
to number of method calls that exist.

michael

___________________________________
Michael Slusarz [slusarz at horde.org]