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

[Jan Schneider]

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

>>> The other 2 Alarm handlers are probably even better candidates to  
>>> move to Core.  And I doubt the Desktop handler even works since it  
>>> uses javascript notification handling which was deprecated long ago.
>>
>> Can you explain why? I don't see any reason why those shouldn't  
>> work just fine in a plain Horde_Notification installation (once the  
>> Desktop handler is fixed).
>
> The only correct way to add Javascript to the page is via the  
> Horde::addScriptFiles()/Horde::addInlineScript().  So theoretically  
> you could pass a bunch of methods to abstract/wrap these Horde  
> functions to the Desktop driver but at some point it is just going  
> to make more sense to call it a Horde-specific driver and move it to  
> Core.

Makes sense.

> My rule of thumb is that once you start having to do these awkward  
> code gymnastics in order to introduce base Horde dependencies, the  
> driver is probably not worth using outside of Horde.  At least I am  
> not interested in supporting these kind of drivers when there is  
> little/no chance they will ever be used outside of a Horde  
> installation.

Agreed.

-- 
The Horde Project
http://www.horde.org/