Re: [commits] Horde branch refactor-Notification updated. 3b590095a4312f85639071510adc42d5dc9090fc

[Gunnar Wrobel-2]

Hi,

as I need to use the Horde_Notification package in stand-alone mode I  
was forced to refactor it a bit. The main problem was that the module  
is firmly embedded into the global Horde context and contains a number  
of calls to services such as Horde_Registry, Horde_Auth, Horde_Nls,  
Horde::logMessage(). These service are of course missing if the  
package is used on its own.

I pushed the necessary refactorings into the "refactor-Notification"  
branch. It would be nice if I could get some feedback on the changes.  
Do they work or are there conflicts? Does the structure look okay? Can  
I merge the branch?

The main changes were two new components: The  
Horde_Notification_Storage and the Horde_Notification_Handler  
interfaces. The storage part allows to exchange the underlying storage  
mechanism (for Horde this is the session by default). And the handler  
part moves the references to global scope that were present in  
Horde_Notification into separate decorators. This way they can be  
avoided when using the Horde_Notification for itself.

Thanks!

Gunnar






--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Jan Schneider]

Zitat von Gunnar Wrobel <p@...>:

> Hi,
>
> as I need to use the Horde_Notification package in stand-alone mode  
> I  was forced to refactor it a bit. The main problem was that the  
> module  is firmly embedded into the global Horde context and  
> contains a number  of calls to services such as Horde_Registry,  
> Horde_Auth, Horde_Nls,  Horde::logMessage(). These service are of  
> course missing if the  package is used on its own.
>
> I pushed the necessary refactorings into the "refactor-Notification"  
>  branch. It would be nice if I could get some feedback on the  
> changes.  Do they work or are there conflicts? Does the structure  
> look okay? Can  I merge the branch?
>
> The main changes were two new components: The  
> Horde_Notification_Storage and the Horde_Notification_Handler  
> interfaces. The storage part allows to exchange the underlying  
> storage  mechanism (for Horde this is the session by default). And  
> the handler  part moves the references to global scope that were  
> present in  Horde_Notification into separate decorators. This way  
> they can be  avoided when using the Horde_Notification for itself.

Looks good so far. Beside the small fixes that I already committed,  
alarm snoozing doesn't work anymore though. It looks like the alarms  
are being cached in the session somehow, but I didn't have to track  
this down yet. That should be fixed before the merge.

I'm not perfectly sure yet about the file and class name layout of the  
decorators. The base handler and the decorators all implement  
Horde_Notification_Handler, so it makes sense to have them in  
Notification/Handler/. OTOH it might be more intuitive to make it  
clearer that those are decorators, by using different class names.  
Chuck might want to chime in here too.
Also, I don't recall right now if we already discussed whether we want  
interfaces to stick more out by using a naming scheme for those. I  
would prefer that, to distinguish them from base classes.
All this is general Horde CS stuff of course, and has nothing to do  
with the Notification refactoring. Meaning that we can always change  
this later if don't find an immediate agreement.

Jan.

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


--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Gunnar Wrobel-2]

Quoting Jan Schneider <jan@...>:

> Zitat von Gunnar Wrobel <p@...>:
>
>> Hi,
>>
>> as I need to use the Horde_Notification package in stand-alone mode  
>> I  was forced to refactor it a bit. The main problem was that the  
>> module  is firmly embedded into the global Horde context and  
>> contains a number  of calls to services such as Horde_Registry,  
>> Horde_Auth, Horde_Nls,  Horde::logMessage(). These service are of  
>> course missing if the  package is used on its own.
>>
>> I pushed the necessary refactorings into the  
>> "refactor-Notification"  branch. It would be nice if I could get  
>> some feedback on the changes.  Do they work or are there conflicts?  
>> Does the structure look okay? Can  I merge the branch?
>>
>> The main changes were two new components: The  
>> Horde_Notification_Storage and the Horde_Notification_Handler  
>> interfaces. The storage part allows to exchange the underlying  
>> storage  mechanism (for Horde this is the session by default). And  
>> the handler  part moves the references to global scope that were  
>> present in  Horde_Notification into separate decorators. This way  
>> they can be  avoided when using the Horde_Notification for itself.
>
> Looks good so far. Beside the small fixes that I already committed,  
> alarm snoozing doesn't work anymore though. It looks like the alarms  
> are being cached in the session somehow, but I didn't have to track  
> this down yet. That should be fixed before the merge.

Okay, will look into that.

>
> I'm not perfectly sure yet about the file and class name layout of  
> the decorators. The base handler and the decorators all implement  
> Horde_Notification_Handler, so it makes sense to have them in  
> Notification/Handler/. OTOH it might be more intuitive to make it  
> clearer that those are decorators, by using different class names.  
> Chuck might want to chime in here too.

I think that would be useful. I used decorators at a few places now  
and sometimes I had to check to determine if it was a "real" class or  
a decorator. I assume this would mean another subfolder in  
"Notification/Handler"? As the class names would start to get really  
long by then I'd tend to use "Notification/Handler/Deco" rather than  
"Notification/Handler/Decorator".

> Also, I don't recall right now if we already discussed whether we  
> want interfaces to stick more out by using a naming scheme for  
> those. I would prefer that, to distinguish them from base classes.
> All this is general Horde CS stuff of course, and has nothing to do  
> with the Notification refactoring. Meaning that we can always change  
> this later if don't find an immediate agreement.

"Notification/Handler/Interface.php"? Or  
"Notification/Interfaces/Handler.php"? Yes, something like that would  
be good.

Cheers,

Gunnar

>
> Jan.
>
> --
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/
>
>
> --
> Horde developers mailing list - Join the hunt: http://horde.org/bounties/
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe@...
>

--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Chuck Hagenbuch]

Quoting Gunnar Wrobel <p@...>:

> I think that would be useful. I used decorators at a few places now  
> and sometimes I had to check to determine if it was a "real" class  
> or a decorator. I assume this would mean another subfolder in  
> "Notification/Handler"? As the class names would start to get really  
> long by then I'd tend to use "Notification/Handler/Deco" rather than  
> "Notification/Handler/Decorator".

No :) Deco is just unclear. Horde_Notification_Handler_Decorator is  
indeed long, but eventually we can look at namespaces to help with  
that, people can use editor code completion - there are plenty of ways  
to cope.

>> Also, I don't recall right now if we already discussed whether we  
>> want interfaces to stick more out by using a naming scheme for  
>> those. I would prefer that, to distinguish them from base classes.
>> All this is general Horde CS stuff of course, and has nothing to do  
>> with the Notification refactoring. Meaning that we can always  
>> change this later if don't find an immediate agreement.
>
> "Notification/Handler/Interface.php"? Or  
> "Notification/Interfaces/Handler.php"? Yes, something like that  
> would be good.

I'd go for Notification/Handler/Interface.  
Notification/Interfaces/Handler isn't autoloadable, at least not with  
our naming scheme and not with a sane interface name.

-chuck

--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Chuck Hagenbuch]

Quoting Gunnar Wrobel <p@...>:

> Hi,
>
> as I need to use the Horde_Notification package in stand-alone mode  
> I was forced to refactor it a bit. The main problem was that the  
> module is firmly embedded into the global Horde context and contains  
> a number of calls to services such as Horde_Registry, Horde_Auth,  
> Horde_Nls, Horde::logMessage(). These service are of course missing  
> if the package is used on its own.
>
> I pushed the necessary refactorings into the "refactor-Notification"  
> branch. It would be nice if I could get some feedback on the  
> changes. Do they work or are there conflicts? Does the structure  
> look okay? Can I merge the branch?
>
> The main changes were two new components: The  
> Horde_Notification_Storage and the Horde_Notification_Handler  
> interfaces. The storage part allows to exchange the underlying  
> storage mechanism (for Horde this is the session by default). And  
> the handler part moves the references to global scope that were  
> present in Horde_Notification into separate decorators. This way  
> they can be avoided when using the Horde_Notification for itself.

High-level, your changes sound good. For specifics, Jan's note about  
alarms, and what we already said about decorator naming. And a random  
detail:

+    public function setNotificationListeners(array &$options);

Why a reference there? If $options is being changed, shouldn't it be  
returned instead?


Otherwise, I'm on board with this going forward and being merged. Thanks!

-chuck

--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Gunnar Wrobel-2]

Quoting Jan Schneider <jan@...>:

> Zitat von Gunnar Wrobel <p@...>:
>
>> Hi,
>>
>> as I need to use the Horde_Notification package in stand-alone mode  
>> I  was forced to refactor it a bit. The main problem was that the  
>> module  is firmly embedded into the global Horde context and  
>> contains a number  of calls to services such as Horde_Registry,  
>> Horde_Auth, Horde_Nls,  Horde::logMessage(). These service are of  
>> course missing if the  package is used on its own.
>>
>> I pushed the necessary refactorings into the  
>> "refactor-Notification"  branch. It would be nice if I could get  
>> some feedback on the changes.  Do they work or are there conflicts?  
>> Does the structure look okay? Can  I merge the branch?
>>
>> The main changes were two new components: The  
>> Horde_Notification_Storage and the Horde_Notification_Handler  
>> interfaces. The storage part allows to exchange the underlying  
>> storage  mechanism (for Horde this is the session by default). And  
>> the handler  part moves the references to global scope that were  
>> present in  Horde_Notification into separate decorators. This way  
>> they can be  avoided when using the Horde_Notification for itself.
>
> Looks good so far. Beside the small fixes that I already committed,

I understood why you removed the "array" type hint for the parameters.  
Why is it required to use Horde_String though?

> alarm snoozing doesn't work anymore though. It looks like the alarms  
> are being cached in the session somehow, but I didn't have to track  
> this down yet. That should be fixed before the merge.

Yes, the message stacks were not cleared anymore. I implemented  
ArrayAccess for the storage handler and that was aparently a bad idea  
as these functions do not pass the values by reference. I will remove  
that tomorrow morning and merge the branch then.

Thanks for the feedback!

Gunnar

>
> I'm not perfectly sure yet about the file and class name layout of  
> the decorators. The base handler and the decorators all implement  
> Horde_Notification_Handler, so it makes sense to have them in  
> Notification/Handler/. OTOH it might be more intuitive to make it  
> clearer that those are decorators, by using different class names.  
> Chuck might want to chime in here too.
> Also, I don't recall right now if we already discussed whether we  
> want interfaces to stick more out by using a naming scheme for  
> those. I would prefer that, to distinguish them from base classes.
> All this is general Horde CS stuff of course, and has nothing to do  
> with the Notification refactoring. Meaning that we can always change  
> this later if don't find an immediate agreement.
>
> Jan.
>
> --
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/
>
>
> --
> Horde developers mailing list - Join the hunt: http://horde.org/bounties/
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe@...
>

--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Jan Schneider]

Zitat von Gunnar Wrobel <p@...>:

> Quoting Jan Schneider <jan@...>:
>
>> Zitat von Gunnar Wrobel <p@...>:
>>
>>> Hi,
>>>
>>> as I need to use the Horde_Notification package in stand-alone  
>>> mode  I  was forced to refactor it a bit. The main problem was  
>>> that the  module  is firmly embedded into the global Horde context  
>>> and  contains a number  of calls to services such as  
>>> Horde_Registry,  Horde_Auth, Horde_Nls,  Horde::logMessage().  
>>> These service are of  course missing if the  package is used on  
>>> its own.
>>>
>>> I pushed the necessary refactorings into the  
>>> "refactor-Notification"  branch. It would be nice if I could get  
>>> some feedback on the changes.  Do they work or are there  
>>> conflicts?  Does the structure look okay? Can  I merge the branch?
>>>
>>> The main changes were two new components: The    
>>> Horde_Notification_Storage and the Horde_Notification_Handler    
>>> interfaces. The storage part allows to exchange the underlying  
>>> storage  mechanism (for Horde this is the session by default). And  
>>>  the handler  part moves the references to global scope that were  
>>> present in  Horde_Notification into separate decorators. This way  
>>> they can be  avoided when using the Horde_Notification for itself.
>>
>> Looks good so far. Beside the small fixes that I already committed,
>
> I understood why you removed the "array" type hint for the  
> parameters.  Why is it required to use Horde_String though?

Because the strtolower/strtoupper/ucfirst family of functions is  
locale-dependent. The result is not the same in all languages.

>> alarm snoozing doesn't work anymore though. It looks like the  
>> alarms  are being cached in the session somehow, but I didn't have  
>> to track  this down yet. That should be fixed before the merge.
>
> Yes, the message stacks were not cleared anymore. I implemented  
> ArrayAccess for the storage handler and that was aparently a bad  
> idea  as these functions do not pass the values by reference. I will  
> remove  that tomorrow morning and merge the branch then.

Great!

Jan.

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


--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Gunnar Wrobel-2]

Quoting Chuck Hagenbuch <chuck@...>:

> Quoting Gunnar Wrobel <p@...>:
>
>> Hi,
>>
>> as I need to use the Horde_Notification package in stand-alone mode  
>> I was forced to refactor it a bit. The main problem was that the  
>> module is firmly embedded into the global Horde context and  
>> contains a number of calls to services such as Horde_Registry,  
>> Horde_Auth, Horde_Nls, Horde::logMessage(). These service are of  
>> course missing if the package is used on its own.
>>
>> I pushed the necessary refactorings into the  
>> "refactor-Notification" branch. It would be nice if I could get  
>> some feedback on the changes. Do they work or are there conflicts?  
>> Does the structure look okay? Can I merge the branch?
>>
>> The main changes were two new components: The  
>> Horde_Notification_Storage and the Horde_Notification_Handler  
>> interfaces. The storage part allows to exchange the underlying  
>> storage mechanism (for Horde this is the session by default). And  
>> the handler part moves the references to global scope that were  
>> present in Horde_Notification into separate decorators. This way  
>> they can be avoided when using the Horde_Notification for itself.
>
> High-level, your changes sound good. For specifics, Jan's note about alarms,

Fixed.

> and what we already said about decorator naming.

Interfaces and Decorators have been renamed now.

> And a random detail:
>
> +    public function setNotificationListeners(array &$options);
>
> Why a reference there? If $options is being changed, shouldn't it be  
> returned instead?

Probably clearer that way. Modified that.


I tested the branch with kronolith which worked fine. So I merged it now.

I am however unable to delete the branch in the horde repository.

# git push horde-write :refs/heads/refactor-Notification
error: denying ref deletion for refs/heads/refactor-Notification
To ssh://dev.horde.org/horde/git/horde
  ! [remote rejected] refactor-Notification (deletion prohibited)
error: failed to push some refs to 'ssh://dev.horde.org/horde/git/horde'

Is that intentional?

Cheers,

Gunnar

>
>
> Otherwise, I'm on board with this going forward and being merged. Thanks!
>
> -chuck
>
> --
> Horde developers mailing list - Join the hunt: http://horde.org/bounties/
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe@...
>

--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Michael Rubinsky-2]

Quoting Gunnar Wrobel <p@...>:

> > Quoting Chuck Hagenbuch <chuck@...>:
> >
> >> Quoting Gunnar Wrobel <p@...>:
> >>
> >>> Hi,
> >>>
> >>> as I need to use the Horde_Notification package in
> stand-alone mode >>> I was forced to refactor it a bit. The main problem
> was that the >>> module is firmly embedded into the global Horde
> context and >>> contains a number of calls to services such as
> Horde_Registry, >>> Horde_Auth, Horde_Nls, Horde::logMessage(). These
> service are of >>> course missing if the package is used on its own.
> >>>
> >>> I pushed the necessary refactorings into the >>>  
> "refactor-Notification" branch. It would be
> nice if I could get >>> some feedback on the changes. Do they work or are
> there conflicts? >>> Does the structure look okay? Can I merge the branch?
> >>>
> >>> The main changes were two new components: The >>>  
> Horde_Notification_Storage and the
> Horde_Notification_Handler >>> interfaces. The storage part allows  
> to exchange the
> underlying >>> storage mechanism (for Horde this is the session by
> default). And >>> the handler part moves the references to global scope
> that were >>> present in Horde_Notification into separate
> decorators. This way >>> they can be avoided when using the  
> Horde_Notification
> for itself.
> >>
> >> High-level, your changes sound good. For specifics, Jan's
> note about alarms,
> >
> > Fixed.
> >
> >> and what we already said about decorator naming.
> >
> > Interfaces and Decorators have been renamed now.
> >
> >> And a random detail:
> >>
> >> +    public function setNotificationListeners(array
> &$options);
> >>
> >> Why a reference there? If $options is being changed,
> shouldn't it be >> returned instead?
> >
> > Probably clearer that way. Modified that.
> >
> >
> > I tested the branch with kronolith which worked fine. So I
> merged it now.
> >
> > I am however unable to delete the branch in the horde
> repository.
> >
> > # git push horde-write :refs/heads/refactor-Notification
> > error: denying ref deletion for
> refs/heads/refactor-Notification
> > To ssh://dev.horde.org/horde/git/horde
> >  ! [remote rejected] refactor-Notification (deletion
> prohibited)
> > error: failed to push some refs to
> 'ssh://dev.horde.org/horde/git/horde'
> >
> > Is that intentional?

Yes. Deletion and non fast-forward commits must be done directly on  
dev.horde.org.

  --
Thanks,
mike

--
The Horde Project (www.horde.org)
mrubinsk@...


--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Michael M Slusarz-2]

Quoting "Michael J. Rubinsky" <mrubinsk@...>:

>>> I am however unable to delete the branch in the horde
>> repository.
>>>
>>> # git push horde-write :refs/heads/refactor-Notification
>>> error: denying ref deletion for
>> refs/heads/refactor-Notification
>>> To ssh://dev.horde.org/horde/git/horde
>>>  ! [remote rejected] refactor-Notification (deletion
>> prohibited)
>>> error: failed to push some refs to
>> 'ssh://dev.horde.org/horde/git/horde'
>>>
>>> Is that intentional?
>
> Yes. Deletion and non fast-forward commits must be done directly on  
> dev.horde.org.

We should probably change the text on the horde.org/source git page,  
since I believe it says the method used by Gunnar is the proper method.

michael

--
___________________________________
Michael Slusarz [slusarz@...]


--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...

[Michael Rubinsky-2]

Quoting Michael M Slusarz <slusarz@...>:

> > Quoting "Michael J. Rubinsky"
> <mrubinsk@...>:
> >
> >>>> I am however unable to delete the branch in the
> horde
> >>> repository.
> >>>>
> >>>> # git push horde-write
> :refs/heads/refactor-Notification
> >>>> error: denying ref deletion for
> >>> refs/heads/refactor-Notification
> >>>> To ssh://dev.horde.org/horde/git/horde
> >>>> ! [remote rejected] refactor-Notification
> (deletion
> >>> prohibited)
> >>>> error: failed to push some refs to
> >>> 'ssh://dev.horde.org/horde/git/horde'
> >>>>
> >>>> Is that intentional?
> >>
> >> Yes. Deletion and non fast-forward commits must be done
> directly on >> dev.horde.org.
> >
> > We should probably change the text on the horde.org/source git
> page, > since I believe it says the method used by Gunnar is the
> proper > method.

Done.

  --
Thanks,
mike

--
The Horde Project (www.horde.org)
mrubinsk@...


--
Horde developers mailing list - Join the hunt: http://horde.org/bounties/
Frequently Asked Questions: http://horde.org/faq/
To unsubscribe, mail: dev-unsubscribe@...