TYPO3 Core

That sounds reasonable indeed.

Francois Suter wrote:

I was thinking about another solution: the execute() method of a given task could be allowed to throw exceptions (in case of fatal problems). So exception could be caught indicating a failure, rather than getting a boolean return value. The advantage is that the exception could return a message which would be better for information.

Good idea, I thought about how to provide a message too, but rather through a separate method like mysql_errno() does. A exception would be a superior solution of course. I'd suggest to have it like now, with a boolean and add the exception handling on top for more detailed responses.

Instead of having a lastexecution_success field, we could call it lastexecution_message. It would be left empty if no exception was thrown and filled with the message otherwise. The message can then be read and displayed in the BE module.

using flash messages for example :)

It could also be written to the sys_log, so that an admin can go back through the history.

Sounds good too, as the current approach would take care of the very last failure only.

Or it could be mix and match: the BE module just displays a failure icon and instructs the admin to look at the sys_log. Or the message could come as a tooltip when moving over the icon, whatever.

Tooltip is also good in case multiple tasks fail so that it's easy to see which task failed for which reason.

I'll go ahead with the changes...

Francois, I'm not sure whether we should really set the default value for $successfullyExecuted in tx_scheduler_Task::unmarkExecution() to false. On the one hand side we could argue that it's a new implementation (scheduler in core) and thus developers have to adopt to the API anyways. On the other hand side developers might not notice that they actually have to return a boolean value according to the execution status.

I wouldn't worry about this too much. There are many changes anyway and I would include a chapter about adapting a Gabriel event to a Scheduler task in the manual.

great!

The patch looks globally good, but I think there's a logic flaw with this part:

if (!$successfullyExecuted) {
    throw new tx_scheduler_FailedExecutionException(
        'Task failed to execute successfully. CRID: ' . $task->getCrid() . ', UID: ' . $task->getTaskUid(),
        1250596541
    );
}

As far as I can see the exception must not be thrown. It must be instantiated and stored into $failure, so that it can be passed to unmarkExecution(). Right?

Francois Suter wrote:

As far as I can see the exception must not be thrown. It must be instantiated and stored into $failure, so that it can be passed to unmarkExecution(). Right?

Nope, it's correct the way it is, by throwing the exeption in the try block it's cought in the catch block below. Also if a task throws an exception while executing the execution of the task will stop at that point and will not reach the mentioned if so that the task's original exception is caught by the catch block.

:)

Nope, it's correct the way it is, by throwing the exeption in the try block it's cought in the catch block below.

I didn't expect this behaviour, but it's fine then. Go ahead and I'll do a post-commit review.

OK, I know what the problem is: in the BE module, every time a task has a non-empty "lastexecution_failure" field, the error message of that failure is displayed as an error box. This is not correct as it means the error is displayed all the time until the task has run again. This may not happen for a good while if the task runs at low intervals (say, once a day or less). During all this time, the error message will be displayed, which is totally confusing.

I propose to just put the error message as a hover of the failure bullet (as it is now) and not as an error message. We could add an indication in the legend, that people should hover over the failure bullet to get more details.

Just a note: after quite some reviewing of the scheduler code, I have a good idea of what could be streamlined in the execution process. This simplification in turn will make it easier to solve the different error reportings that are needed depending on the execution process (CLI script or BE module).

I just don't have the time to do it tonight. Stay tuned!

Francois Suter wrote:

OK, I know what the problem is: in the BE module, every time a task has a non-empty "lastexecution_failure" field, the error message of that failure is displayed as an error box. This is not correct as it means the error is displayed all the time until the task has run again. This may not happen for a good while if the task runs at low intervals (say, once a day or less). During all this time, the error message will be displayed, which is totally confusing.

Well to me that's intended behavior. It's also what we agreed on AFAIR - to show the message until someone spots it and resolves the error (by fixing it and running the task again). How would I otherwise notice?

Well to me that's intended behavior. It's also what we agreed on AFAIR - to show the message until someone spots it and resolves the error (by fixing it and running the task again). How would I otherwise notice?

Indeed, that corresponds to what we discussed and I'm not criticising what you did. It's just that seeing in action, it doesn't seem like such a good idea anymore.

The user will notice because the status bullet is red and indicates a failure. So the failure information is not lost. The problem with displaying the failure message as an error flash message, is that it appears to be out of context. If several tasks have failed, there's no way to relate an error message to a specific task. Furthermore, if you are working in the BE module, you get those error messages on every screen view, no matter what operation your performed (unless it was a successful execution of that very task), which is very confusing.

So I think the failure bullet with the hover bubble is quite enough. The error message would still appear when a manual execution fails.