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

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

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.

michael

___________________________________
Michael Slusarz [slusarz at horde.org]