[dev] [commits] Horde branch master updated. 1e394986ba8dd83a6f2854f51c361f72fce9e7b5

[Jan Schneider]

I'm not talking for Michael obviously, but I'm still gonna add my two cent.

Zitat von Gunnar Wrobel <p at rdus.de>:

> Hi Micheal,
>
> Quoting Michael M Slusarz <slusarz at horde.org>:
>
> [snip]
>
>>
>> commit e48b8a54fc54a179be749967a6a8b6b73cc1162e
>> Author: Michael M Slusarz <slusarz at curecanti.org>
>> Date:   Mon Feb 15 23:27:31 2010 -0700
>>
>>    Another Notification rewrite.
>>
>>    First, fix Bug #8870. Fixed by removing Nag specific event type
>>    (nag.alarm).  However, this was just a symptom of a larger problem.
>>
>>    The problem: using application specific Notification handlers to handle
>>    application specific event types.  The problem comes when switching
>>    between applications.  Since these application handlers don't have any
>>    knowledge of each other, events created by one handler may not be able
>>    to be displayed when notify() was eventually called, because another
>>    status handler had replaced the original handler.
>>
>>    The solution: all notifications need to be handled by a single,
>>    centralized source - namely, the horde-level handlers.  Application
>>    specific details are instead injected into the horde-level handler to
>>    extend behavior.
>>
>>    While reworking the code, also provided opportunity to remove all
>>    application-specific code from Notification.  Horde-specific
>>    instantiation (i.e. adding Horde logging and Alarm decorators) is now
>>    done in Horde_Core rather than in the base Notification object.
>
> Cool, great! Thanks.
>
>>
>>    Additionally, rework some of the complexity added to the package.  I
>>    believe the goal of the recent Notification changes was to make the
>>    Notification package testable and/or usable outside of a base Horde
>>    install.
>
> Doing such a rewrite for testing purposes would seem wrong to me.  
> The  only intention was to make the package usable outside of a  
> standard  Horde installation.
>
> I hope it is okay to transform Horde framework packages this way. My  
>  expectation has always been that the framework packages should work  
>  that way and be as independent of each other as reasonable.  
> Probably  originates from the fact that the Kolab server always  
> relied to a  larger degree on the framework than on the  
> applications. This has been  somewhat difficult in the past because  
> of the tendency to use global  scope in the current code. But things  
> are improving of course.

This expectation is absolutely correct.

> One question concerning the unit tests: I added them because they  
> are  required to a certain degree by the project where I'm using the  
>  Horde_Notification package. I am however not 100% certain if these   
> tests are useful to the Horde project. I see a tendency towards   
> PHPUnit testing within Horde. But we are also ignoring them to a   
> certain degree as we don't seem to care much if they break. And I  
> feel  that at that point unit tests may become more of a nuisance  
> rather  than being helpful.

We rather need more tests than less. The fact that they break from  
time to time is probably due to us not having used tests that much in  
the past. We still have to get used to run them after each change.  
Some continuous integration would probably help too, but I don't see  
anyone having the resources at the moment to set this up.

> Did the tests make sense to you while rewriting Horde notification  
> or  did you consider them to be a hindrance?
>
>>    But these changes also made the code unreadable, redundant,
>>    and overly complex.
>
> This sounds very negative which is why I am asking whether making  
> the  packages more independant is actually okay.

It's absolutely okay and should be forced even more.

> Concerning the changes I did I can easily accept that a lot of the   
> stuff might not be necessary and too complex for the package. I  
> looked  at your changes and they made sense to me. So there is hope  
> that the  next time I'll do things like that it will be somewhat  
> cleaner or more  effective. :)
>
> Another thing would be if the desire to make the packages more   
> independent meets resistance in itself. That would mean that I   
> shouldn't use them at all. But my impression was that this was not   
> what your were aiming at with your comment, right?
>
>>
>>    e.g. using interfaces where simple class extensions
>>    make much more sense (IMHO - there are very few cases where an interface
>>    makes more sense than an abstract class. Using interfaces for the
>>    Handler class was simply overkill.  Out of the 10 methods defined, there
>>    are only 2 methods useful for decorator purposes - push() and notify().
>>    And any given decorator won't even use both of these.  Having to contort
>>    code to do things like chaining handlers to achieve this in an interface
>>    pattern was almost impossible to follow.  It is much simpler to simply
>>    add decorators directly to the base handler object.
>
> Ack and thanks for the hints.
>
> One remaining question concerning Horde_Notification: Currently   
> framework/Notification/lib/Horde/Notification/Listener/Status.php  
> and  framework/Notification/lib/Horde/Notification/Event/Status.php  
> have an  optional dependency on Horde_Mobile and Horde_Nls  
> respectively. Both  are only required if a specific option has been  
> set. Is that  sufficient to make the dependency in package.xml  
> optional or is  another check with "class_exists" required in order  
> to disable the  dependency in case the corresponding packages are  
> not installed?

That makes sense to me.

> And finally: Is it okay if I grab your recent Horde_LoginTasks and   
> start to transform it in a similar way? :) I would like to use it in  
>  the very same project as a standalone version and would need to  
> remove  the hard dependency on the usual global Horde constants.
>
> Cheers,
>
> Gunnar
>
> [snip]
>
>
>
>



Jan.

-- 
Do you need professional PHP or Horde consulting?
http://horde.org/consulting/