[dev] [commits] Horde branch Horde_LoginTasks-Refactor created. b70b84343d05538bad914a0738ae25e63ab31806
Michael M Slusarz
slusarz at horde.org
Tue Mar 9 21:10:06 UTC 2010
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:
>>>> 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 Slusarz [slusarz at horde.org]
More information about the dev