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

[Gunnar Wrobel]

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.

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.

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.

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?

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]



-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digitale PGP-Unterschrift
URL: <http://lists.horde.org/archives/dev/attachments/20100224/316bfc70/attachment.bin>