bug symfony#10480 [Process] Fixed fatal errors in getOutput and getErrorOutput when process was not started (romainneutron)

This PR was merged into the 2.3 branch.

Discussion
----------

[Process] Fixed fatal errors in getOutput and getErrorOutput when process was not started

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#10022
| License       | MIT

This PR replaces symfony#9452 and address the latest changes.
Side note : I've not updated `getExitCode`, `getExitCodeText` and `isSuccessful` as they were explicitly tested to return null in case the process is not started or terminated. I think it would be a BC break to do that.

Commits
-------

449fe01 [Process] Trow exceptions in case a Process method is supposed to be called after termination
0ae6858 [Process] fixed fatal errors in getOutput and getErrorOutput when process was not started

Author: fabpot

src/Symfony/Component/Process/Process.php

@@ -300,9 +300,12 @@ public function restart($callback = null)
      *
      * @throws RuntimeException When process timed out
      * @throws RuntimeException When process stopped after receiving signal
+     * @throws LogicException   When process is not yet started
      */
     public function wait($callback = null)
     {
+        $this->requireProcessIsStarted(__FUNCTION__);
+
         $this->updateStatus(false);
         if (null !== $callback) {
             $this->callback = $this->buildCallback($callback);
@@ -370,10 +373,14 @@ public function signal($signal)
      *
      * @return string The process output
      *
+     * @throws LogicException In case the process is not started
+     *
      * @api
      */
     public function getOutput()
     {
+        $this->requireProcessIsStarted(__FUNCTION__);
+
         $this->readPipes(false, defined('PHP_WINDOWS_VERSION_BUILD') ? !$this->processInformation['running'] : true);
 
         return $this->stdout;
@@ -385,10 +392,14 @@ public function getOutput()
      * In comparison with the getOutput method which always return the whole
      * output, this one returns the new output since the last call.
      *
+     * @throws LogicException In case the process is not started
+     *
      * @return string The process output since the last call
      */
     public function getIncrementalOutput()
     {
+        $this->requireProcessIsStarted(__FUNCTION__);
+
         $data = $this->getOutput();
 
         $latest = substr($data, $this->incrementalOutputOffset);
@@ -402,10 +413,14 @@ public function getIncrementalOutput()
      *
      * @return string The process error output
      *
+     * @throws LogicException In case the process is not started
+     *
      * @api
      */
     public function getErrorOutput()
     {
+        $this->requireProcessIsStarted(__FUNCTION__);
+
         $this->readPipes(false, defined('PHP_WINDOWS_VERSION_BUILD') ? !$this->processInformation['running'] : true);
 
         return $this->stderr;
@@ -418,10 +433,14 @@ public function getErrorOutput()
      * whole error output, this one returns the new error output since the last
      * call.
      *
+     * @throws LogicException In case the process is not started
+     *
      * @return string The process error output since the last call
      */
     public function getIncrementalErrorOutput()
     {
+        $this->requireProcessIsStarted(__FUNCTION__);
+
         $data = $this->getErrorOutput();
 
         $latest = substr($data, $this->incrementalErrorOutputOffset);
@@ -433,7 +452,7 @@ public function getIncrementalErrorOutput()
     /**
      * Returns the exit code returned by the process.
      *
-     * @return integer The exit status code
+     * @return null|integer The exit status code, null if the Process is not terminated
      *
      * @throws RuntimeException In case --enable-sigchild is activated and the sigchild compatibility mode is disabled
      *
@@ -456,14 +475,18 @@ public function getExitCode()
      * This method relies on the Unix exit code status standardization
      * and might not be relevant for other operating systems.
      *
-     * @return string A string representation for the exit status code
+     * @return null|string A string representation for the exit status code, null if the Process is not terminated.
+     *
+     * @throws RuntimeException In case --enable-sigchild is activated and the sigchild compatibility mode is disabled
      *
      * @see http://tldp.org/LDP/abs/html/exitcodes.html
      * @see http://en.wikipedia.org/wiki/Unix_signal
      */
     public function getExitCodeText()
     {
-        $exitcode = $this->getExitCode();
+        if (null === $exitcode = $this->getExitCode()) {
+            return;
+        }
 
         return isset(self::$exitCodes[$exitcode]) ? self::$exitCodes[$exitcode] : 'Unknown error';
     }
@@ -488,11 +511,14 @@ public function isSuccessful()
      * @return Boolean
      *
      * @throws RuntimeException In case --enable-sigchild is activated
+     * @throws LogicException   In case the process is not terminated.
      *
      * @api
      */
     public function hasBeenSignaled()
     {
+        $this->requireProcessIsTerminated(__FUNCTION__);
+
         if ($this->isSigchildEnabled()) {
             throw new RuntimeException('This PHP has been compiled with --enable-sigchild. Term signal can not be retrieved.');
         }
@@ -510,11 +536,14 @@ public function hasBeenSignaled()
      * @return integer
      *
      * @throws RuntimeException In case --enable-sigchild is activated
+     * @throws LogicException   In case the process is not terminated.
      *
      * @api
      */
     public function getTermSignal()
     {
+        $this->requireProcessIsTerminated(__FUNCTION__);
+
         if ($this->isSigchildEnabled()) {
             throw new RuntimeException('This PHP has been compiled with --enable-sigchild. Term signal can not be retrieved.');
         }
@@ -531,10 +560,14 @@ public function getTermSignal()
      *
      * @return Boolean
      *
+     * @throws LogicException   In case the process is not terminated.
+     *
      * @api
      */
     public function hasBeenStopped()
     {
+        $this->requireProcessIsTerminated(__FUNCTION__);
+
         $this->updateStatus(false);
 
         return $this->processInformation['stopped'];
@@ -547,10 +580,14 @@ public function hasBeenStopped()
      *
      * @return integer
      *
+     * @throws LogicException   In case the process is not terminated.
+     *
      * @api
      */
     public function getStopSignal()
     {
+        $this->requireProcessIsTerminated(__FUNCTION__);
+
         $this->updateStatus(false);
 
         return $this->processInformation['stopsig'];
@@ -938,6 +975,10 @@ public function setEnhanceSigchildCompatibility($enhance)
      */
     public function checkTimeout()
     {
+        if ($this->status !== self::STATUS_STARTED) {
+            return;
+        }
+
         if (null !== $this->timeout && $this->timeout < microtime(true) - $this->starttime) {
             $this->stop(0);
 
@@ -1155,4 +1196,32 @@ private function doSignal($signal, $throwException)
 
         return true;
     }
+
+    /**
+     * Ensures the process is running or terminated, throws a LogicException if the process has a not started.
+     *
+     * @param $functionName The function name that was called.
+     *
+     * @throws LogicException If the process has not run.
+     */
+    private function requireProcessIsStarted($functionName)
+    {
+        if (!$this->isStarted()) {
+            throw new LogicException(sprintf('Process must be started before calling %s.', $functionName));
+        }
+    }
+
+    /**
+     * Ensures the process is terminated, throws a LogicException if the process has a status different than `terminated`.
+     *
+     * @param $functionName The function name that was called.
+     *
+     * @throws LogicException If the process is not yet terminated.
+     */
+    private function requireProcessIsTerminated($functionName)
+    {
+        if (!$this->isTerminated()) {
+            throw new LogicException(sprintf('Process must be terminated before calling %s.', $functionName));
+        }
+    }
 }

src/Symfony/Component/Process/Tests/AbstractProcessTest.php

@@ -272,6 +272,12 @@ public function testTTYCommandExitCode()
         $this->assertTrue($process->isSuccessful());
     }
 
+    public function testExitCodeTextIsNullWhenExitCodeIsNull()
+    {
+        $process = $this->getProcess('');
+        $this->assertNull($process->getExitCodeText());
+    }
+
     public function testExitCodeText()
     {
         $process = $this->getProcess('');
@@ -512,6 +518,19 @@ public function testRunProcessWithTimeout()
         $this->assertLessThan($timeout + Process::TIMEOUT_PRECISION, $duration);
     }
 
+    public function testCheckTimeoutOnNonStartedProcess()
+    {
+        $process = $this->getProcess('php -r "sleep(3);"');
+        $process->checkTimeout();
+    }
+
+    public function testCheckTimeoutOnTerminatedProcess()
+    {
+        $process = $this->getProcess('php -v');
+        $process->run();
+        $process->checkTimeout();
+    }
+
     public function testCheckTimeoutOnStartedProcess()
     {
         $timeout = 0.5;
@@ -617,6 +636,55 @@ public function testSignalProcessNotRunning()
         $process->signal(SIGHUP);
     }
 
+    /**
+     * @dataProvider provideMethodsThatNeedARunningProcess
+     */
+    public function testMethodsThatNeedARunningProcess($method)
+    {
+        $process = $this->getProcess('php -m');
+        $this->setExpectedException('Symfony\Component\Process\Exception\LogicException', sprintf('Process must be started before calling %s.', $method));
+        call_user_func(array($process, $method));
+    }
+
+    public function provideMethodsThatNeedARunningProcess()
+    {
+        return array(
+            array('getOutput'),
+            array('getIncrementalOutput'),
+            array('getErrorOutput'),
+            array('getIncrementalErrorOutput'),
+            array('wait'),
+        );
+    }
+
+    /**
+     * @dataProvider provideMethodsThatNeedATerminatedProcess
+     */
+    public function testMethodsThatNeedATerminatedProcess($method)
+    {
+        $process = $this->getProcess('php -r "sleep(1);"');
+        $process->start();
+        try {
+            call_user_func(array($process, $method));
+            $process->stop(0);
+            $this->fail('A LogicException must have been thrown');
+        } catch (\Exception $e) {
+            $this->assertInstanceOf('Symfony\Component\Process\Exception\LogicException', $e);
+            $this->assertEquals(sprintf('Process must be terminated before calling %s.', $method), $e->getMessage());
+        }
+        $process->stop(0);
+    }
+
+    public function provideMethodsThatNeedATerminatedProcess()
+    {
+        return array(
+            array('hasBeenSignaled'),
+            array('getTermSignal'),
+            array('hasBeenStopped'),
+            array('getStopSignal'),
+        );
+    }
+
     private function verifyPosixIsEnabled()
     {
         if (defined('PHP_WINDOWS_VERSION_BUILD')) {