Throw exception / error on unsuccessful execution run

[matteodem]

I didn't find any documentation on error handling thus far that's why I'd propose the following:

Add a new flag called throwErrorOnFail to the CommandStack, so that there's a way to configure that the code stops execution after a failed execution (result was unsuccessful).

This is helpful to avoid running any code after a command previously failed.

[danepowell]

I'd love to see a solution for this in Robo. I think it might need to go beyond just a flag, to something where commands throw an exception by default. Otherwise it doesn't really solve the problem BLT is facing, which is that we'd like for commands to handle errors conservatively (i.e. throw an exception) by default, rather than forcing developers to remember to check exit codes every time they run a command.

It's awfully hard for downstream projects to work around this problem. Here's BLT's WIP that I'm half tempted to abandon due to the shear complexity: acquia/blt#3839

@greg-1-anderson I'd love to hear your thoughts on this issue. Do you think the request, or at least BLT's problem statement of trying to avoid uncaught errors, makes sense? Would you propose an alternative solution, or a cleaner workaround than what we've started in the PR?

I'm happy to put in some legwork if we can reach consensus on a solution.

[greg-1-anderson]

I think that throwing exceptions by default might be the right thing to do for Robo 2.x. I am -1 on having a bunch of flags like stopOnFail and throwErrorOnFail, especially if they do not work uniformly across all types of tasks.

The one question about throw by default is, how do scripts respond if they want to catch a failure? Throw is undoubtably best for clients using Robo as a framework, but Robo's roots lie in simple scripts that work reliably, and are still easy to read (not a lot of nesting with conditionals and try / catch).

This can evolve as Robo 2 develops. I'm spending a lot of time on Composer In Core initiative right now, but I would like to make it easier to define tasks in Robo 2 that execute in sequence (e.g. in a collection), but can still pass values from one step to the next using a promise-like object. Perhaps an exception handling model can fit into this development.

[danepowell]

I figured that projects implementing Robo would just use try/catch. So for tasks that should exit on error:

// Robo 1.x
$result = $this->taskExecStack()
      ->exec("do something that should exit on error")
      ->run();
if (!$result->wasSuccessful()) {
  throw new Exception("Task failed!")
}

// Robo 2.x
$this->taskExecStack()
      ->exec("do something that should exit on error")
      ->run();

I suspect this use case is most common, and it becomes much simpler and safer in the new paradigm. To your point about excessive conditionals and such, this actually helps the situation.

For commands that should not exit on error:

// Robo 1.x
$result = $this->taskExecStack()
      ->exec("do something that should not exit on error")
      ->run();
if (!$result->wasSuccessful()) {
  // Do something intelligent here to recover or throw error.
}

// Robo 2.x
try {
  $this->taskExecStack()
        ->exec("do something that should not exit on error")
        ->run();
}
catch (RoboException $e) {
    // $e contains the result including exit code.
    // Do something intelligent here to recover or throw error.
}

This is only moderately more complex, and somewhat less common I suspect.

Another option to get this into Robo 1.x without affecting backwards compatibility would be to add a flag to the Robo container itself that persists for all commands, i.e. throwExceptionOnError. I personally prefer this to the idea of adding this flag just to the CommandStack, although it is more complex.

[greg-1-anderson]

I wanted to avoid having folks have to put try / catch in their scripts to recover from errors.

Robo 2.x idea.

Throws an exception on error (caught by application, which exits):

$promise = $this->task()->ExecStack()
  ->exec('do something that must work')
  ->run();

Does not throw an exception on error:

$promise = $this->task()->execStack()
  ->exec('do something that might error');
$result = $this->task()->run();
if (!$result->wasSuccessful()) {
  // Optional recovery code here
}
// Error will also be reported if returned without handling.
return $result;

The way this works is that task() returns an object that is both the Tasks factor (that has all of the old taskFoo() methods, minus the word task) and also the task collection. Creating a task implicitly adds it to the collection. Running the task implicitly removes it from the collection (and throws if it fails). Running a task also implicitly runs any task that was added to the collection prior to the task being run, also removing them from the collection. This also resolves the promises for all of the tasks executed (if any).

The task() collection also provides a run() method that runs all of the tasks already added to the collection and catches any exception that is thrown, returning the result in an object.

So, you have the choice of whether to throw or not depending on how you run your tasks.

If this is a little too magical, then perhaps the different forms of run() should have names that are explicitly different.

I am also thinking that I can make this work:

$promise = $this->task()->foo()
    ->setValue($parameter);
$this->task()->bar()
   ->setOtherValue($promise->value())
  ->run();

That is, promise->value() is unknown at the time that setOtherValue() is called, but once bar() is run(), foo() will be run first, and its promises will be resolved. After the preceding tasks are run, then bar() will look up the value of all of the resolved promises that were used as parameters to its set method and substitute them, or throw if there are any promises that are not resolved. Robo 1.x nearly does this today with its "deferred" mechanism.

Again, there's a trade off between having magical things happening in the task runner vs. having to do a lot of error checking throughout the script. Folks are going to tend to leave off the if(!$result->wasSuccessful()) or the try / catch because it's "usually going to work" (read: I'm too lazy / in a hurry to type the extra lines right now). Postponing error recovery to the spot in the program where it actually has to be done gives us the advantage of having fewer if / catch blocks in the script, which is fewer opportunities to be lazy, and less nested logic to read when reading the script again later.

For a framework-type application, I think that having a lot of explicit error checking / catch blocks is best. For a task runner, though, I think think that there's benefit to keeping the scripts easy to write and easy to read without making them brittle e.g. if a failed step is not checked and execution continues. That's why Robo 0.6 had the whole stopOnFail() paradigm. The problem with the flags, though, is that there's extra overhead required (conceptual overhead) to remember to set the flags correctly and so on.

[danepowell]

That's pretty clever, I do like the idea of maintaining the "old" style checking of return objects rather than using try/catch. I agree that try/catch is a bit ugly, but I'm probably biased because I grew up writing PHP instead of... I don't know, just about any other programming language? :)

I agree it feels a bit magical, I might need to think about it more to have a strong opinion on whether it's too magical, and honestly to grok everything happening with the promises.

It sounds like we are on the same page that the priority is making it easy to implement Robo tasks in a (usually) safe way, without having to remember a bunch of flags and patterns such as stopOnFail() and if($result->wasSuccessful()) {yadda yadda}.

Tangentially, I discovered that we were using stopOnFail() all over the place in a totally ineffective way the other day, such as on single-command tasks that should exit the process on failure. This code was written a long time ago so I can only guess how that happened, but I think it's because stopOnFail() implies that a failing task will stop the entire process (i.e. throw an exception), not just the task itself.

[greg-1-anderson]

I don't use stopOnFail() myself, but if I recall correctly, it only applies to "stack"-style tasks, and it only stops that one task.

By default, collections always stop if one of the tasks in them fails.

[danepowell]

Just so. But my point was just that the name is confusing, in the theme of having to remember what flags to use and what they do, how to do error handling consistently, etc...