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

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Gunnar Wrobel <p at rdus.de>:
>
>> Quoting Michael M Slusarz <slusarz at horde.org>:
>>>
>>>   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.

I agree with Jan.  My mistake in trying to guess your original  
motivation.  Obviously, the it would be great if all of our framework  
packages (except 1 - see below) are usable outside of a Horde  
application.

The one exception is going to be the Core package.  This is the place  
where all of the glue between Horde *application* and Horde  
*framework* is going to go.  And with the recent changes that are  
starting to make this separation a reality, one overriding thought has  
come to mind: why do we even have a Core package?  It seems to me that  
everything in Core should be moved to the horde application package.   
Is this the eventual goal?

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

I would agree with the comment that test are good, but not necessarily  
with the comment that more tests are better.  There can be a problem  
with too many tests, and then you just run into the issue of having to  
add additional maintenance costs for the test code on top of the  
actual code.  There's a fine balance between trying to test all  
permutations of the code vs. testing that you can replicate a  
big-picture result.

When I had finished the rewrite, I found that the big-picture results  
were correct (i.e. given a particular message, the notification output  
was as expected) but I found myself constantly going in and tweaking  
what some intermediate steps were doing.  But this probably also had a  
lot to do with the design of the various Notification libraries -  
namely that there was a bunch of public functions.  Once I simplified  
the API and moved a lot of things to protected/private functions, the  
tests made a lot more sense - they focused more on "given this  
real-world input, we should expect this output" vs. "let's try to fake  
some input in order to test the output of a particular intermediate  
function used."

I'm not a tremendously big fan of PHPUnit from what I know about it so  
far; it seems somewhat hackish to me - e.g. naming functions like  
testTryingToDescribeAnEntireTestInA100CharacterFunctionNameUsingStudlyCapsIsNotTheMostClearWayToIdentifyAFailingTest.  Not sure if this is a limitation of PHPUnit or simply a case where we have not yet used all capabilities available in the package.  This makes determining what tests were failing a matter of bashing through a bunch of Exception-like text dumps.  Maybe I am simply using phpunit wrong - a wiki entry might be  
useful.

I just have to get into the habit of running the unit tests when  
making a change.  This is something I admittedly need to become better  
at.

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

I probably should have described this a bit more - my original  
comments weren't meant to be derogatory in the least.  But I will  
admit there was a little bit of frustration at the time I wrote the  
commit message, since it took me several hours of re-reading the code  
multiple times to figure out what the heck it was trying to accomplish.

I think the frustration had more to do with the coding decisions  
rather than the unit tests.  As mentioned in the commit message, the  
use of a PHP interface rather than extending a PHP class made the code  
extremely unwieldy.  And it led to a bunch of tests in the testing  
suite that didn't really test anything.  Most of these tests were of  
the pattern "test if this function in the interface calls a given  
parent function".  Once I converted to a normal class/extends pattern  
all these tests weren't needed.

My one bit of constructive criticism is that


The commit message was as long as it was not to be confrontational or  
bragging, but instead as a kind of brain dump of things that I learned  
while tacking the problem.  Whether or not it is/was of any usefulness  
to anybody but myself... that's a different question.

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

There is always going to be a difference in opinion when it comes to  
coding.  Give 100 programmers a problem, and you will get 100 opinions  
on the best way to do that.

But I feel I am fair when it comes to analyzing an issue like this  
(obviously, this can be debated... :) ).  And my thinking was as  
follows:

1.) I have been working with the Notification code for years
2.) I had a somewhat simple issue - figuring out why a notification in  
an application wasn't working
3.) Tracing the code to the framework package, I was absolutely  
stumped as to what the various classes were trying to do.

So that's when I started trying to figure out the motivation as to why  
the code was changed in the first place.

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

Correct.

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

Well... my ultimate desire is to remove Horde_Mobile entirely, and  
possibly very soon.  It has been superseded (or will be superseded) by  
Horde_View anyway - or whatever other output/template/view system will  
eventually decide on.

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

That would be great!  Details on the best means of converting to reach  
this goal are obviously still being worked out (this is probably the  
time to thank you for all your work on Horde_Injector) and further  
input from someone converting another library would be great.

Sorry for any misunderstanding I may have caused.  And much thanks to  
all the work you have done for the project recently.

michael

-- 
___________________________________
Michael Slusarz [slusarz at horde.org]