TaskExitException should contain the error output

[prudloff-insite]

This is a followup to findings in #919.

Steps to reproduce

We have a task like this:

  public function test() {
    $sshTask = $this->taskExecStack();
    $sshTask
      ->interactive(FALSE)
      ->exec('echo "this command output is very long"')
      ->exec('echo "this command output is very long"')
      ->exec('echo "this command output is very long"')
      ->exec('echo "this command output is very long"')
      ->exec('echo "this command output is very long"')
      ->exec('echo "this command output is very long"')
      ->exec('invalid_cmd')
      ->run();
  }

Expected behavior

If the exception contained the error output, it would be quite readable (with the error easily spotted at the end):

 [ExecStack] echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && invalid_cmd
 [ExecStack] Running echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 invalid_cmd
this command output is very long
this command output is very long
this command output is very long
this command output is very long
this command output is very long
this command output is very long
sh: 1: invalid_cmd: not found
 [ExecStack]  Exit code 127  Time 0.003s
 [notice] Stopping on fail. Exiting....
 [error]  Exit Code: 127 
 [error]    in task Robo\Task\Base\ExecStack 

  sh: 1: invalid_cmd: not found
 

Actual behavior

It displays this:

 [ExecStack] echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && echo "this command output is very long" && invalid_cmd
 [ExecStack] Running echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 echo "this command output is very long" &&
 invalid_cmd
this command output is very long
this command output is very long
this command output is very long
this command output is very long
this command output is very long
this command output is very long
sh: 1: invalid_cmd: not found
 [ExecStack]  Exit code 127  Time 0.003s
 [notice] Stopping on fail. Exiting....
 [error]  Exit Code: 127 
 [error]    in task Robo\Task\Base\ExecStack 

  this command output is very long
this command output is very long
this command output is very long
this command output is very long
this command output is very long
this command output is very long
 

This confuses our users because the error is buried between the long output printed twice so it is hard to find.

System Configuration

PHP 8.1.27 on Linux Mint 21

[greg-1-anderson]

Is this fixed by #1152?

[prudloff-insite]

It does not fix it.
#1152 avoids printing STDOUT twice at the end but it still prints and not STDERR.

Something like this fixes it:

diff --git a/src/Common/ExecTrait.php b/src/Common/ExecTrait.php
index 9959d304..b944cad0 100644
--- a/src/Common/ExecTrait.php
+++ b/src/Common/ExecTrait.php
@@ -395,11 +395,20 @@ trait ExecTrait
             $this->startTimer();
             $this->process->run($output_callback);
             $this->stopTimer();
-            return new ResultData(
-                $this->process->getExitCode(),
-                $this->process->getOutput(),
-                $this->getResultData()
-            );
+
+            if ($this->process->isSuccessful()) {
+                return new ResultData(
+                    $this->process->getExitCode(),
+                    $this->process->getOutput(),
+                    $this->getResultData()
+                );
+            } else {
+                return new ResultData(
+                    $this->process->getExitCode(),
+                    $this->process->getErrorOutput(),
+                    $this->getResultData()
+                );
+            }
         }
 
         try {

But I suppose people may rely on the ResultData always containing the output.
It might be cleaner to add a new errorOutput property on ResultData and Result so that Result::exitEarly() can then use it like this:

    private function exitEarly($status)
    {
        throw new TaskExitException($this->getTask(), $this->getErrorOutput(), $status);
    }

[greg-1-anderson]

Yes, I agree that switching the output in the result data from stdout to stderr in error conditions would be undesirable. I haven't evaluated the impact of changing the things stored in a the result data, or how such data would be handled, and the backwards-compatibility implications thereof, but a backwards compatible fix / improvement in this area would be welcome.