MediaWiki\Shell: log stderr

Change-Id: I1495fe2aba10102d7e36c3a3e5fdabf97f14546b

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 75ce8ec..0496b67 100644 (file)
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -442,6 +442,7 @@ return [
 
                $factory = new CommandFactory( $limits, $cgroup );
                $factory->setLogger( LoggerFactory::getInstance( 'exec' ) );
+               $factory->logStderr();
 
                return $factory;
        },

diff --git a/includes/shell/Command.php b/includes/shell/Command.php
index 1816c5a..9f080d5 100644 (file)
--- a/includes/shell/Command.php
+++ b/includes/shell/Command.php
@@ -57,7 +57,10 @@ class Command {
        private $method;
 
        /** @var bool */
-       private $useStderr = false;
+       private $doIncludeStderr = false;
+
+       /** @var bool */
+       private $doLogStderr = false;
 
        /** @var bool */
        private $everExecuted = false;
@@ -180,7 +183,19 @@ class Command {
         * @return $this
         */
        public function includeStderr( $yesno = true ) {
-               $this->useStderr = $yesno;
+               $this->doIncludeStderr = $yesno;
+
+               return $this;
+       }
+
+       /**
+        * When enabled, text sent to stderr will be logged with a level of 'error'.
+        *
+        * @param bool $yesno
+        * @return $this
+        */
+       public function logStderr( $yesno = true ) {
+               $this->doLogStderr = $yesno;
 
                return $this;
        }
@@ -235,7 +250,7 @@ class Command {
                                $cmd = '/bin/bash ' . escapeshellarg( __DIR__ . '/limit.sh' ) . ' ' .
                                        escapeshellarg( $cmd ) . ' ' .
                                        escapeshellarg(
-                                               "MW_INCLUDE_STDERR=" . ( $this->useStderr ? '1' : '' ) . ';' .
+                                               "MW_INCLUDE_STDERR=" . ( $this->doIncludeStderr ? '1' : '' ) . ';' .
                                                "MW_CPU_LIMIT=$time; " .
                                                'MW_CGROUP=' . escapeshellarg( $this->cgroup ) . '; ' .
                                                "MW_MEM_LIMIT=$mem; " .
@@ -246,7 +261,7 @@ class Command {
                                $useLogPipe = true;
                        }
                }
-               if ( !$useLogPipe && $this->useStderr ) {
+               if ( !$useLogPipe && $this->doIncludeStderr ) {
                        $cmd .= ' 2>&1';
                }
 
@@ -424,6 +439,15 @@ class Command {
                        $this->logger->warning( "$logMsg: {command}", [ 'command' => $cmd ] );
                }
 
+               if ( $errBuffer && $this->doLogStderr ) {
+                       $this->logger->error( "Error running {command}: {error}", [
+                               'command' => $cmd,
+                               'error' => $errBuffer,
+                               'exitcode' => $retval,
+                               'exception' => new Exception( 'Shell error' ),
+                       ] );
+               }
+
                return new Result( $retval, $outBuffer, $errBuffer );
        }
 }

diff --git a/includes/shell/CommandFactory.php b/includes/shell/CommandFactory.php
index c0b8f89..84dd50f 100644 (file)
--- a/includes/shell/CommandFactory.php
+++ b/includes/shell/CommandFactory.php
@@ -37,6 +37,9 @@ class CommandFactory {
        /** @var string|bool */
        private $cgroup;
 
+       /** @var bool */
+       private $doLogStderr = false;
+
        /**
         * Constructor
         *
@@ -49,6 +52,16 @@ class CommandFactory {
                $this->setLogger( new NullLogger() );
        }
 
+       /**
+        * When enabled, text sent to stderr will be logged with a level of 'error'.
+        *
+        * @param bool $yesno
+        * @see Command::logStderr
+        */
+       public function logStderr( $yesno = true ) {
+               $this->doLogStderr = $yesno;
+       }
+
        /**
         * Instantiates a new Command
         *
@@ -60,6 +73,7 @@ class CommandFactory {
 
                return $command
                        ->limits( $this->limits )
-                       ->cgroup( $this->cgroup );
+                       ->cgroup( $this->cgroup )
+                       ->logStderr( $this->doLogStderr );
        }
 }

diff --git a/tests/phpunit/includes/shell/CommandFactoryTest.php b/tests/phpunit/includes/shell/CommandFactoryTest.php
index aacfd43..f90e837 100644 (file)
--- a/tests/phpunit/includes/shell/CommandFactoryTest.php
+++ b/tests/phpunit/includes/shell/CommandFactoryTest.php
@@ -23,11 +23,13 @@ class CommandFactoryTest extends PHPUnit_Framework_TestCase {
 
                $factory = new CommandFactory( $limits, $cgroup );
                $factory->setLogger( $logger );
+               $factory->logStderr();
                $command = $factory->create();
 
                $wrapper = TestingAccessWrapper::newFromObject( $command );
                $this->assertSame( $logger, $wrapper->logger );
                $this->assertSame( $cgroup, $wrapper->cgroup );
                $this->assertSame( $limits, $wrapper->limits );
+               $this->assertTrue( $wrapper->doLogStderr );
        }
 }

diff --git a/tests/phpunit/includes/shell/CommandTest.php b/tests/phpunit/includes/shell/CommandTest.php
index 32d855e..81fae33 100644 (file)
--- a/tests/phpunit/includes/shell/CommandTest.php
+++ b/tests/phpunit/includes/shell/CommandTest.php
@@ -118,4 +118,25 @@ class CommandTest extends PHPUnit_Framework_TestCase {
                        $this->assertEquals( 333333, strlen( $output ) );
                }
        }
+
+       public function testLogStderr() {
+               $this->requirePosix();
+
+               $logger = new TestLogger( true, function ( $message, $level, $context ) {
+                       return $level === Psr\Log\LogLevel::ERROR ? '1' : null;
+               }, true );
+               $command = new Command();
+               $command->setLogger( $logger );
+               $command->params( 'bash', '-c', 'echo ThisIsStderr 1>&2' );
+               $command->execute();
+               $this->assertEmpty( $logger->getBuffer() );
+
+               $command = new Command();
+               $command->setLogger( $logger );
+               $command->logStderr();
+               $command->params( 'bash', '-c', 'echo ThisIsStderr 1>&2' );
+               $command->execute();
+               $this->assertSame( 1, count( $logger->getBuffer() ) );
+               $this->assertSame( trim( $logger->getBuffer()[0][2]['error'] ), 'ThisIsStderr' );
+       }
 }