Merge pull request #2257 from hafriedlander/fix/flush_30

FIX Nice errors and allows flush on module removal

core/startup/ErrorControlChain.php

@@ -3,16 +3,13 @@
 /**
  * Class ErrorControlChain
  *
- * Runs a set of steps, optionally suppressing (but recording) any errors (even fatal ones) that occur in each step.
- * If an error does occur, subsequent steps are normally skipped, but can optionally be run anyway
- *
- * Normal errors are suppressed even past the end of the chain. Fatal errors are only suppressed until the end
- * of the chain - the request will then die silently.
+ * Runs a set of steps, optionally suppressing uncaught errors or exceptions which would otherwise be fatal that
+ * occur in each step. If an error does occur, subsequent steps are normally skipped, but can optionally be run anyway.
  *
  * Usage:
  *
  * $chain = new ErrorControlChain();
- * $chain->then($callback1)->then($callback2)->then(true, $callback3)->execute();
+ * $chain->then($callback1)->then($callback2)->thenIfErrored($callback3)->execute();
  *
  * WARNING: This class is experimental and designed specifically for use pre-startup in main.php
  * It will likely be heavily refactored before the release of 3.2
@@ -28,6 +25,9 @@ class ErrorControlChain {
 	/** We can't unregister_shutdown_function, so this acts as a flag to enable handling */
 	protected $handleFatalErrors = false;
 
+	/** We overload display_errors to hide errors during execution, so we need to remember the original to restore to */
+	protected $originalDisplayErrors = null;
+
 	public function hasErrored() {
 		return $this->error;
 	}
@@ -38,6 +38,7 @@ class ErrorControlChain {
 
 	public function setSuppression($suppression) {
 		$this->suppression = (bool)$suppression;
+		if ($this->handleFatalErrors) ini_set('display_errors', !$suppression);
 	}
 
 	/**
@@ -68,24 +69,14 @@ class ErrorControlChain {
 		return $this->then($callback, null);
 	}
 
-	public function handleError($errno, $errstr) {
-		if ((error_reporting() & self::$fatal_errors & $errno) != 0 && $this->suppression) {
-			throw new Exception('Generic Error');
-		}
-		else {
-			return false;
-		}
-	}
-
 	protected function lastErrorWasFatal() {
 		$error = error_get_last();
-		return $error && $error['type'] == 1;
+		return $error && ($error['type'] & self::$fatal_errors) != 0;
 	}
 
 	public function handleFatalError() {
 		if ($this->handleFatalErrors && $this->suppression) {
 			if ($this->lastErrorWasFatal()) {
-				ob_clean();
 				$this->error = true;
 				$this->step();
 			}
@@ -93,10 +84,12 @@ class ErrorControlChain {
 	}
 
 	public function execute() {
-		set_error_handler(array($this, 'handleError'));
 		register_shutdown_function(array($this, 'handleFatalError'));
 		$this->handleFatalErrors = true;
 
+		$this->originalDisplayErrors = ini_get('display_errors');
+		ini_set('display_errors', !$this->suppression);
+
 		$this->step();
 	}
 
@@ -105,13 +98,7 @@ class ErrorControlChain {
 			$step = array_shift($this->steps);
 
 			if ($step['onErrorState'] === null || $step['onErrorState'] === $this->error) {
-				try {
-					call_user_func($step['callback'], $this);
-				}
-				catch (Exception $e) {
-					if ($this->suppression) $this->error = true;
-					else throw $e;
-				}
+				call_user_func($step['callback'], $this);
 			}
 
 			$this->step();
@@ -119,7 +106,7 @@ class ErrorControlChain {
 		else {
 			// Now clean up
 			$this->handleFatalErrors = false;
-			restore_error_handler();
+			ini_set('display_errors', $this->originalDisplayErrors);
 		}
 	}
 }

tests/core/startup/ErrorControlChainTest.php

@@ -1,90 +1,241 @@
 <?php
 
+/**
+ * An extension of ErrorControlChain that runs the chain in a subprocess.
+ *
+ * We need this because ErrorControlChain only suppresses uncaught fatal errors, and
+ * that would kill PHPUnit execution
+ */
+class ErrorControlChainTest_Chain extends ErrorControlChain {
+
+	function executeInSubprocess() {
+		// Get the path to the ErrorControlChain class
+		$classpath = SS_ClassLoader::instance()->getItemPath('ErrorControlChain');
+		$suppression = $this->suppression ? 'true' : 'false';
+
+		// Start building a PHP file that will execute the chain
+		$src = '<'."?php
+require_once '$classpath';
+
+\$chain = new ErrorControlChain();
+
+\$chain->setSuppression($suppression);
+
+\$chain
+";
+
+		// For each step, use reflection to pull out the call, stick in the the PHP source we're building
+		foreach ($this->steps as $step) {
+			$func = new ReflectionFunction($step['callback']);
+			$source = file($func->getFileName());
+
+			$start_line = $func->getStartLine() - 1;
+			$end_line = $func->getEndLine();
+			$length = $end_line - $start_line;
+
+			$src .= implode("", array_slice($source, $start_line, $length)) . "\n";
+		}
+
+		// Finally add a line to execute the chain
+		$src .= "->execute();";
+
+		// Now stick it in a temporary file & run it
+		$codepath = TEMP_FOLDER.'/ErrorControlChainTest_'.sha1($src).'.php';
+
+		$null = is_writeable('/dev/null') ? '/dev/null' : 'NUL';
+
+		file_put_contents($codepath, $src);
+		exec("php $codepath 2> $null", $stdout, $errcode);
+		unlink($codepath);
+
+		return array(implode("\n", $stdout), $errcode);
+	}
+}
+
 class ErrorControlChainTest extends SapphireTest {
 
-	function testErrorSuppression() {
-		$chain = new ErrorControlChain();
+	function setUp() {
+		// Check we can run PHP at all
+		$null = is_writeable('/dev/null') ? '/dev/null' : 'NUL';
+		exec("php -v 2> $null", $out, $rv);
 
-		$chain
-			->then(function(){
-				user_error('This error should be suppressed', E_USER_ERROR);
-			})
-			->execute();
+		if ($rv != 0) {
+			$this->markTestSkipped("Can't run PHP from the command line - is it in your path?");
+			$this->skipTest = true;
+		}
 
-		$this->assertTrue($chain->hasErrored());
+		parent::setUp();
 	}
 
-	function testMultipleErrorSuppression() {
-		$chain = new ErrorControlChain();
+	function testErrorSuppression() {
 
-		$chain
+		// Fatal error
+
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
 			->then(function(){
-				user_error('This error should be suppressed', E_USER_ERROR);
+				Foo::bar(); // Non-existant class causes fatal error
 			})
-			->thenAlways(function(){
-				user_error('This error should also be suppressed', E_USER_ERROR);
+			->thenIfErrored(function(){
+				echo "Done";
 			})
-			->execute();
+			->executeInSubprocess();
 
-		$this->assertTrue($chain->hasErrored());
+		$this->assertEquals('Done', $out);
+
+		// User error
+
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
+			->then(function(){
+				user_error('Error', E_USER_ERROR);
+			})
+			->thenIfErrored(function(){
+				echo "Done";
+			})
+			->executeInSubprocess();
+
+		$this->assertEquals('Done', $out);
+
+		// Recoverable error
+
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
+			->then(function(){
+				$x = function(ErrorControlChain $foo){ };
+				$x(1); // Calling against type
+			})
+			->thenIfErrored(function(){
+				echo "Done";
+			})
+			->executeInSubprocess();
+
+		$this->assertEquals('Done', $out);
 	}
 
 	function testExceptionSuppression() {
-		$chain = new ErrorControlChain();
+		$chain = new ErrorControlChainTest_Chain();
 
-		$chain
+		list($out, $code) = $chain
 			->then(function(){
 				throw new Exception('This exception should be suppressed');
 			})
-			->execute();
-
-		$this->assertTrue($chain->hasErrored());
-	}
-
-	function testMultipleExceptionSuppression() {
-		$chain = new ErrorControlChain();
-
-		$chain
-			->then(function(){
-				throw new Exception('This exception should be suppressed');
+			->thenIfErrored(function(){
+				echo "Done";
 			})
-			->thenAlways(function(){
-				throw new Exception('This exception should also be suppressed');
-			})
-			->execute();
+			->executeInSubprocess();
 
-		$this->assertTrue($chain->hasErrored());
+		$this->assertEquals('Done', $out);
 	}
 
 	function testErrorControl() {
-		$preError = $postError = array('then' => false, 'thenIfErrored' => false, 'thenAlways' => false);
+		$chain = new ErrorControlChainTest_Chain();
 
-		$chain = new ErrorControlChain();
-
-		$chain
-			->then(function() use (&$preError) { $preError['then'] = true; })
-			->thenIfErrored(function() use (&$preError) { $preError['thenIfErrored'] = true; })
-			->thenAlways(function() use (&$preError) { $preError['thenAlways'] = true; })
+		list($out, $code) = $chain
+			->then(function() { echo 'preThen,'; })
+			->thenIfErrored(function() { echo 'preThenIfErrored,'; })
+			->thenAlways(function() { echo 'preThenAlways,'; })
 
 			->then(function(){ user_error('An error', E_USER_ERROR); })
 
-			->then(function() use (&$postError) { $postError['then'] = true; })
-			->thenIfErrored(function() use (&$postError) { $postError['thenIfErrored'] = true; })
-			->thenAlways(function() use (&$postError) { $postError['thenAlways'] = true; })
+			->then(function() { echo 'postThen,'; })
+			->thenIfErrored(function() { echo 'postThenIfErrored,'; })
+			->thenAlways(function() { echo 'postThenAlways,'; })
 
-			->execute();
+			->executeInSubprocess();
 
 		$this->assertEquals(
-			array('then' => true, 'thenIfErrored' => false, 'thenAlways' => true),
-			$preError,
-			'Then and thenAlways callbacks called before error, thenIfErrored callback not called'
-		);
-
-		$this->assertEquals(
-			array('then' => false, 'thenIfErrored' => true, 'thenAlways' => true),
-			$postError,
-			'thenIfErrored and thenAlways callbacks called after error, then callback not called'
+			"preThen,preThenAlways,postThenIfErrored,postThenAlways,",
+			$out
 		);
 	}
 
+	function testSuppressionControl() {
+		// Turning off suppression before execution
+
+		$chain = new ErrorControlChainTest_Chain();
+		$chain->setSuppression(false);
+
+		list($out, $code) = $chain
+			->then(function($chain){
+				Foo::bar(); // Non-existant class causes fatal error
+			})
+			->executeInSubprocess();
+
+		$this->assertContains("Fatal error: Class 'Foo' not found", $out);
+
+		// Turning off suppression during execution
+
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
+			->then(function($chain){
+				$chain->setSuppression(false);
+				Foo::bar(); // Non-existent class causes fatal error
+			})
+			->executeInSubprocess();
+
+		$this->assertContains("Fatal error: Class 'Foo' not found", $out);
+	}
+
+	function testDoesntAffectNonFatalErrors() {
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
+			->then(function(){
+				$array = null;
+				if (@$array['key'] !== null) user_error('Error', E_USER_ERROR);
+			})
+			->then(function(){
+				echo "Good";
+			})
+			->thenIfErrored(function(){
+				echo "Bad";
+			})
+			->executeInSubprocess();
+
+		$this->assertContains("Good", $out);
+	}
+
+	function testDoesntAffectCaughtExceptions() {
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
+			->then(function(){
+				try {
+					throw new Exception('Error');
+				}
+				catch (Exception $e) {
+					echo "Good";
+				}
+			})
+			->thenIfErrored(function(){
+				echo "Bad";
+			})
+			->executeInSubprocess();
+
+		$this->assertContains("Good", $out);
+	}
+
+	function testDoesntAffectHandledErrors() {
+		$chain = new ErrorControlChainTest_Chain();
+
+		list($out, $code) = $chain
+			->then(function(){
+				set_error_handler(function(){ /* NOP */ });
+				user_error('Error', E_USER_ERROR);
+			})
+			->then(function(){
+				echo "Good";
+			})
+			->thenIfErrored(function(){
+				echo "Bad";
+			})
+			->executeInSubprocess();
+
+		$this->assertContains("Good", $out);
+	}
 }