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

[Chuck Hagenbuch]

Quoting Michael M Slusarz <slusarz at horde.org>:

>> 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 :)

I'm willing to say this is how we should go. I agree this is what  
exceptions are for.

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

This will change with controllers/routes, etc. anyway, so making  
decisions based on whether or not to rewrite apps doesn't seem like a  
strong argument to me.

-chuck