[sork] Patch for vacation forwards-driver (.forward-parser)

[Espen Jürgensen]

Citat Eric Rostetter <eric.rostetter at physics.utexas.edu>:

> >   // Try to login with the username/password, no realm.
> >   $status = $_vfs->checkCredentials();
> >   if (is_a($status, 'PEAR_Error')) {
> >       $this->err_str = $status->getMessage();
> >       $this->err_str .= '  ' .  _("Check your username and password.");
> >       return false;
> >   }
> >
> Yes, I would expect it to show an error, and AFAIK it does, just not
> in that code, but in the main.php code.  The code there reads:

Sorry for being unprecise, I forgot that this piece of code appears other 
places in forwards.php. I was talking about the occurance in getUserDetails, 
line 161 to 165.

I understand your points about setVacation and unsetVacation, but they don't 
apply in this case. As you know, getUserDetails gets called by isEnabled, which 
gets called after setVacation/unsetVacation. Therefore the above err_str never 
gets shown.

> Now, I suppose we could stack the messages at in the driver, for 
> display later,
...
> Where are you calling it from?  It is the calling code's job (currently) to
> display the error.

Yes, and that is how it should be, if you ask me. Calling notification->push 
from the driver wouldn't be nice. The problem is that isEnabled (the caller) 
doesn't react to errors.

> Well, it used to ignore it, but now it checks the return (Y/N/other).
> But while it doesn't ignore the error, it does (at least sometimes) ignore
> the error text.  This is because it is doing something behind the scenes
> without user action.  If the user requests the action, then it should
> return
> the error text.  But if it is just trying to pre-emptively help the user
> out without the user's knowledge, and the success or failure is of no
> real importance to the user, then it silently discards the error text.

I agree, you shouldn't give the user useless error messages.

But I think that the user will normally expect that what vacation displays is 
actually a reflection of the current settings. So wouldn't it be nice to tell 
him when that is not the case?

I propose something like this added after line 113 in main.php:

$notification->push(_("The current state of your vacation settings could not be 
determined"));

This would cover the following:

1) Driver doesn't support retrieval
2) Couldn't establish connection/retrieve settings (if ftp failed)
3) Couldn't understand settings (if parser couldn't understand .forward)

It might not be as elegant as PEAR_error, but it is very simple ... and it 
would stop the stream of emails from me! (perhaps...)

> Yes, that is clear now.  I'm still not sure what the problem is, but I know
> what you are talking about at least.  I guess I need to dig up your parser
> patch to see what you are trying to do.

The patch is already commited, just check the last part of forward.php. I 
believe it works fine, but it does not show any errors. Also it currently only 
makes getUserDetails exit with Y/N, it doesn't make getUserDetails exit with 
false on error (because errors didn't get shown anyway, and because there is a 
problem related to exiting with false when .forward couldn't be parsed, but 
still saving .vacation.msg in $this->_details). I plan to change that, however.

Regards,
Espen