[dev] [commits] Horde branch Horde_LoginTasks-Refactor created. b70b84343d05538bad914a0738ae25e63ab31806

[Michael M Slusarz]

Quoting Gunnar Wrobel <p at rdus.de>:

> Hi Micheal,
>
> Quoting Michael M Slusarz <slusarz at horde.org>:
>
>> Quoting Gunnar Wrobel <p at rdus.de>:
>>
>>>> For example, if you have the following tasks:
>>>>
>>>> DISPLAY_CONFIRM_NO
>>>> DISPLAY_CONFIRM_YES
>>>> DISPLAY_NONE
>>>> DISPLAY_NOTICE
>>>> DISPLAY_CONFIRM_YES
>>>> DISPLAY_NONE
>>>>
>>>> There should be 3 confirmation screens generated: 1 with  
>>>> CONFIRM_NO and CONFIRM_YES tasks,
>>>
>>> As CONFIRM_NO != CONFIRM_YES the check "$v['task']->display !=  
>>> $display" in needDisplay() should disallow displaying both on the  
>>> same screen or do I misunderstand that section?
>>
>> I think the code is correct.  When adding the task with addTask(),  
>> CONFIRM_NO will have 'display' set to true and $_addFlag will be  
>> false.  CONFIRM _YES will also have 'display set to true since it  
>> is not DISPLAY_NONE and $_addFlag is not set.  Therefore, when you  
>> run needDisplay(), both tasks will skip the break statement (in our  
>> example, CONFIRM_NO will skip because 'displa'y is true and  
>> $display is null; CONFIRM_YES will skip because 'display' is true and
>
> Everything correct ...
>
>> $v['task']->display == $display).
>
> ... except for this statement. In the first round when handling  
> CONFIRM_NO $display will be set to CONFIRM_NO while  
> $v['task']->display is CONFIRM_YES in the second round.
>
> Just to be certain I tested this outside of my test suite by adding  
> two dummy tasks to a horde base installation. I do indeed only get  
> one confirmation.
>
> After confirming this first task I get redirected to the portal  
> instead of getting the remaining login tasks displayed. This is  
> something I also see in my unit tests. The redirection will  
> transport me to the URL initially provided to runTasks() instead of  
> moving to the login tasks page again.
>
> I'm going to provide suggested fixes in the branch.

OK - I'll take your word that it is not working as expected and trust  
that your fixes have remedied the problem :)

>>> I'm done with the core part of the refactoring. Having the  
>>> abstract Horde_LoginTasks_Backend class should be sufficient for  
>>> me to plug the system into something other than Horde. Can you  
>>> give me some feedback if you think the modifications are okay and  
>>> if I can merge?
>>
>> I will take a look.  It would be best to move all Horde-specific  
>> backend stuff to Core - like I did with the Notification driver.   
>> That would remove all horde core-specific code from LoginTasks.
>
> This would mean moving Horde_LoginTasks_Backend_Horde into  
> Horde_Core_LoginTasks_Backend, right? That would make sense to me.  
> I'd convert Horde_LoginTasks_Backend to an interface in that case  
> (rather than the abstract class it is now).

Correct, I think (although Horde_Core_Backend_LoginTasks would  
probably be more appropriate).  Since I'm not sure we have yet decided  
definitively how to use Horde_Injector with variable constructors  
(i.e. constructor arguments that may vary in usage throughout Horde).

> I noticed some additional things when I started using the package  
> outside of Horde now. I can't place the redirection feature into the  
> backend as I'm in a Zend MVC context. And redirection needs to be  
> done by the controller. So runTasks() must return the redirection  
> address so that the controller can perform the actual redirection.  
> This is something already supported as runTasks() returns the result  
> of the redirect call to the backend. Hope that is okay.
>
> And I need to consider if it is possible to have tasks that redirect  
> to their own specific URLs rather than a global login task URL.

As above, I will defer to your conclusions on these issues since you  
are the one that is currently actively working on the code.

michael

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