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

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

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

No - the code in IMP only does very IMP-specific checks in one  
particular situation.  99% of the time, an authentication failure is  
handled by the authenticationFailureRedirect() call.

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

I still disagree with this, but I think I am in the minority here.  My  
personal belief is that there is no reason to extend the Exception  
class unless you are adding functionality to that Exception class.   
Because in essence what you are doing in the above example is nothing  
more than classname checking - it is no different than using  
Exceptions and getCode().

If this is the way we want to do things going forward, then I'll  
follow along.  Now is probably a fantastic time to make that decision :)

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

Wouldn't you just add a new exception case to  
authenticationFailureRedirect()?  That is ten times easier than trying  
to rewrite authentication error handling in 50 different applications.

If you happen to be in the rare case where you don't want to redirect,  
an app is free to do whatever it wants before calling  
authenticationFailureRedirect().  [Probably should change the name of  
that function to Horde_Auth::failureHandler() or something a bit more  
concise.]

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

It's going to be a major PITA to maintain hooks (and the corresponding  
config options) in every single application.  Although thinking about  
this a bit more, doing everything in one hook in Horde is also not  
optimal since we would be in Horde scope (not the app scope) at that  
point.  Any code written there would need to do manual  
pushApp()/popApp() calls which is probably asking too much for an  
admin-level coder.

michael

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