Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

FIX Nice errors and allows flush on module removal #2257

Merged
merged 1 commit into from

2 participants

Hamish Friedlander Sam Minnée
Hamish Friedlander

No description provided.

Sam Minnée
Owner

It looks like your test is now out of date:

1) ErrorControlChainTest::testErrorSuppression
This error should be suppressed
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:10
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:95
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:87
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:12
/home/travis/.phpenv/versions/5.3.26/bin/phpunit:46
2) ErrorControlChainTest::testMultipleErrorSuppression
This error should be suppressed
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:22
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:95
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:87
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:27
/home/travis/.phpenv/versions/5.3.26/bin/phpunit:46
3) ErrorControlChainTest::testExceptionSuppression
Exception: This exception should be suppressed
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:37
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:95
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:87
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:39
/home/travis/.phpenv/versions/5.3.26/bin/phpunit:46
4) ErrorControlChainTest::testMultipleExceptionSuppression
Exception: This exception should be suppressed
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:49
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:95
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:87
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:54
/home/travis/.phpenv/versions/5.3.26/bin/phpunit:46
5) ErrorControlChainTest::testErrorControl
An error
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:69
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:95
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:98
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:98
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:98
/home/travis/builds/ss/framework/core/startup/ErrorControlChain.php:87
/home/travis/builds/ss/framework/tests/core/startup/ErrorControlChainTest.php:75
/home/travis/.phpenv/versions/5.3.26/bin/phpunit:46

Hamish Friedlander

Yup, suspected that might happen - just wanted camspier to confirm the fix was good. I'll fix tests in a sec.

Sam Minnée
Owner

Happy to merge this once the tests are fixed.

Sam Minnée
Owner

Ooh, while you're at it, you might want to add a test that this works when called within a ErrorControl block:

$array = null;
$this->assertNull(@$array['key']);

Sam Minnée sminnee merged commit 3c6ba1c into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
39 core/startup/ErrorControlChain.php
View
@@ -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 @@ public function setErrored($error) {
public function setSuppression($suppression) {
$this->suppression = (bool)$suppression;
+ if ($this->handleFatalErrors) ini_set('display_errors', !$suppression);
}
/**
@@ -68,24 +69,14 @@ public function thenAlways($callback) {
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 @@ public function handleFatalError() {
}
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 @@ protected function step() {
$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 @@ protected function step() {
else {
// Now clean up
$this->handleFatalErrors = false;
- restore_error_handler();
+ ini_set('display_errors', $this->originalDisplayErrors);
}
}
}
245 tests/core/startup/ErrorControlChainTest.php
View
@@ -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 setUp() {
+ // Check we can run PHP at all
+ $null = is_writeable('/dev/null') ? '/dev/null' : 'NUL';
+ exec("php -v 2> $null", $out, $rv);
+
+ if ($rv != 0) {
+ $this->markTestSkipped("Can't run PHP from the command line - is it in your path?");
+ $this->skipTest = true;
+ }
+
+ parent::setUp();
+ }
+
function testErrorSuppression() {
- $chain = new ErrorControlChain();
- $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
+ })
+ ->thenIfErrored(function(){
+ echo "Done";
})
- ->execute();
+ ->executeInSubprocess();
- $this->assertTrue($chain->hasErrored());
- }
+ $this->assertEquals('Done', $out);
+
+ // User error
- function testMultipleErrorSuppression() {
- $chain = new ErrorControlChain();
+ $chain = new ErrorControlChainTest_Chain();
- $chain
+ list($out, $code) = $chain
->then(function(){
- user_error('This error should be suppressed', E_USER_ERROR);
+ user_error('Error', E_USER_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);
- function testExceptionSuppression() {
- $chain = new ErrorControlChain();
+ // Recoverable error
- $chain
+ $chain = new ErrorControlChainTest_Chain();
+
+ list($out, $code) = $chain
->then(function(){
- throw new Exception('This exception should be suppressed');
+ $x = function(ErrorControlChain $foo){ };
+ $x(1); // Calling against type
})
- ->execute();
+ ->thenIfErrored(function(){
+ echo "Done";
+ })
+ ->executeInSubprocess();
- $this->assertTrue($chain->hasErrored());
+ $this->assertEquals('Done', $out);
}
- function testMultipleExceptionSuppression() {
- $chain = new ErrorControlChain();
+ function testExceptionSuppression() {
+ $chain = new ErrorControlChainTest_Chain();
- $chain
+ list($out, $code) = $chain
->then(function(){
throw new Exception('This exception should be suppressed');
})
- ->thenAlways(function(){
- throw new Exception('This exception should also be suppressed');
+ ->thenIfErrored(function(){
+ echo "Done";
})
- ->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'
+ "preThen,preThenAlways,postThenIfErrored,postThenAlways,",
+ $out
);
+ }
- $this->assertEquals(
- array('then' => false, 'thenIfErrored' => true, 'thenAlways' => true),
- $postError,
- 'thenIfErrored and thenAlways callbacks called after error, then callback not called'
- );
+ 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);
+ }
}
Something went wrong with that request. Please try again.