[dev] [cvs] commit: horde login.php horde/templates/index frames_index.inc horde/templates/login header.inc login.inc mobile.inc horde/services changepassword.php facebook.php logintasks.php twitter.php twitterapi.php horde/services/portal edit.php ...

[Jan Schneider]

Zitat von Michael M Slusarz <slusarz at horde.org>:

> Quoting Jan Schneider <jan at horde.org>:
>
>> This is far too much "knowledge" that the consumer of pushApp()  
>> should have. And assuming that the only exception that pushApp()  
>> might throw happens if the user is not authenticated, is too short  
>> sighted IMO. We need a specific exception for that case, i.e.  
>> Horde_Exception_NotAuthenticated, so that with any other exception  
>> the global exception handler is called, and not the user silently  
>> being redirected to the login page.
>
> You do get this information in the getCode() method of the exception  
> - it will let you know if there was an authentication failure  
> (Horde_Registry::AUTH_FAILURE), a permissions failure  
> (Horde_Registry::PERMISSION_DENIED), or the fact that the  
> application is not active (Horde_Registry::NOT_ACTIVE).  See imp for  
> an example of what an app could potentially do to work with one of  
> those types of errors.  But 99% of the time, applications shouldn't  
> be dealing with these issues.  What's the point of a framework if  
> the applications have to do all the error checking.

The code in IMP you referred to is a perfect example where the  
application *does* all the error checking.

> I am *very* against the idea of determining errors based on  
> Exception classnames - that is not what classnames are for.  Not to  
> mention it is overkill to define a bunch of Exception files that  
> need to be included in every page, when most of these exception  
> files don't extend the original class one bit.

This is *exactly* what exceptions are for, and this has nothing to do  
with class name checking:

try {
     $registry->pushApp('app');
} catch (Horde_Exception_NotAuthenticated $e) {
     Horde_Auth::authenticationFailureRedirect('app', $e);
} catch (Horde_Exception_PermissionDenied $e) {
     $notification->push('Go away');
}
// Any other exception handled by the global handler.

> Reading back at your post, I guess I am confused what your concern  
> is.  Using Horde::authenticationFailureRefirect(), you will *never*  
> be redirected to the login page without there being an explanation  
> why authentication was unsuccessful (either auth issue or  
> permissions issue).  And in the third error case (application not  
> active), this will throw an exception that should result in a fatal  
> error.

I missed that the exception is being passed to  
authenticationFailureRedirect(). But this should still not be called  
unconditionally, no matter what exception we see.
You say that there are only 3 possible exception cases. But that could  
change in any point in the future. And we might not want to redirect  
to the login page at this case.

>>>>>>> * Hooks probably don't work yet.  We may need to rethink hooks  
>>>>>>> a bit.  First, there should not be need for app-specific  
>>>>>>> pre/post-auth hooks.  This will all be handled by a single  
>>>>>>> hook in Horde ($app will be one of the parameters passed to  
>>>>>>> the hook).  Unfortunately, pre-auth hooks don't make any sense  
>>>>>>> for transparent auth or for apps that don't need  
>>>>>>> authentication since any value returned from the pre-auth hook  
>>>>>>> is totally ignored.  I recommend the following refactoring of  
>>>>>>> these hooks:
>>>>>>>
>>>>>>> + pre-auth hook: called only for horde-auth/apps that need  
>>>>>>> authentication. This hook is solely for the purpose of  
>>>>>>> altering auth credentials.
>>>>>>> + post-auth hook: called for all apps after user has been  
>>>>>>> authenticated to the module. App-specific setup can be handled  
>>>>>>> in here. Return value of false indicates application is not  
>>>>>>> available.
>>>>>>
>>>>>> Does that mean that post-auth is called for every module?
>>>>>
>>>>> Yes.  This removes the need for per-app hooks to do work after  
>>>>> first login.
>>>>
>>>> Sorry, I still don't get this. Is this hook called for every  
>>>> installed application, once the user logged in?
>>>
>>> It is called for every application the first time it is accessed  
>>> in the session.  If you have enabled something like chora, and it  
>>> is never accessed, the postauth hook is never called.
>>
>> That's pretty useful, but should be a different hook than postauth.  
>> It might confuse others too. I expect postauth to do the same like  
>> the old postauth hook, i.e. being called directly after the (Horde)  
>> authentication, as a last resort to deny authentication.
>> A hook that being called on first access to an application should  
>> be clearly named like that, e.g. "firstaccess" or  
>> "application_init" or even separate ones ("initialize") for each  
>> application in each app's hook.php.
>
> I am against this also.  We have *way* too many hooks as it is.  I  
> work on this code everyday, and I don't know what half of the hooks  
> do.  In the interest of slimming down configuration, which I think  
> we all agree is a big goal of Horde 4, this is one of the ways to  
> accomplish that.

I disagree. The large number of hooks is, what Horde's real power of  
flexibility is. Being able to call hooks at all kind of places in the  
code is major selling point, and we should rather extend this, than  
slimming it down.
There is no disagreement about, that those "grown" hooks need to be  
cleaned up.

Jan.

-- 
Do you need professional PHP or Horde consulting?
http://horde.org/consulting/
-------------- 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/20090724/2dcbd2af/attachment.bin>