From 62bd26d11ab9c9bf5b91ba8abb776ab3a4813a18 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 18 May 2016 13:36:54 +1200 Subject: [PATCH] BUG Fix suppression of display_errors in ErrorControlChain --- core/startup/ErrorControlChain.php | 34 ++++++-- tests/core/startup/ErrorControlChainTest.php | 90 +++++++++++++------- 2 files changed, 86 insertions(+), 38 deletions(-) diff --git a/core/startup/ErrorControlChain.php b/core/startup/ErrorControlChain.php index a8cf7277f..0f6cb07de 100644 --- a/core/startup/ErrorControlChain.php +++ b/core/startup/ErrorControlChain.php @@ -46,13 +46,35 @@ class ErrorControlChain { */ public function setSuppression($suppression) { $this->suppression = (bool)$suppression; - // Don't modify errors unless handling fatal errors, and if errors were - // originally allowed to be displayed. - if ($this->handleFatalErrors && $this->originalDisplayErrors) { - ini_set('display_errors', !$suppression); + // If handling fatal errors, conditionally disable, or restore error display + // Note: original value of display_errors could also evaluate to "off" + if ($this->handleFatalErrors) { + if($suppression) { + $this->setDisplayErrors(0); + } else { + $this->setDisplayErrors($this->originalDisplayErrors); + } } } + /** + * Set display_errors + * + * @param mixed $errors + */ + protected function setDisplayErrors($errors) { + ini_set('display_errors', $errors); + } + + /** + * Get value of display_errors ini value + * + * @return mixed + */ + protected function getDisplayErrors() { + return ini_get('display_errors'); + } + /** * Add this callback to the chain of callbacks to call along with the state * that $error must be in this point in the chain for the callback to be called @@ -123,7 +145,7 @@ class ErrorControlChain { register_shutdown_function(array($this, 'handleFatalError')); $this->handleFatalErrors = true; - $this->originalDisplayErrors = ini_get('display_errors'); + $this->originalDisplayErrors = $this->getDisplayErrors(); $this->setSuppression($this->suppression); $this->step(); @@ -142,7 +164,7 @@ class ErrorControlChain { else { // Now clean up $this->handleFatalErrors = false; - ini_set('display_errors', $this->originalDisplayErrors); + $this->setDisplayErrors($this->originalDisplayErrors); } } } diff --git a/tests/core/startup/ErrorControlChainTest.php b/tests/core/startup/ErrorControlChainTest.php index d8e0df7f9..ec2c926e1 100644 --- a/tests/core/startup/ErrorControlChainTest.php +++ b/tests/core/startup/ErrorControlChainTest.php @@ -8,6 +8,30 @@ */ class ErrorControlChainTest_Chain extends ErrorControlChain { + protected $displayErrors = 'STDERR'; + + /** + * Modify method visibility to public for testing + * + * @return string + */ + public function getDisplayErrors() + { + // Protect manipulation of underlying php_ini values + return $this->displayErrors; + } + + /** + * Modify method visibility to public for testing + * + * @param mixed $errors + */ + public function setDisplayErrors($errors) + { + // Protect manipulation of underlying php_ini values + $this->displayErrors = $errors; + } + // Change function visibility to be testable directly public function translateMemstring($memstring) { return parent::translateMemstring($memstring); @@ -63,10 +87,7 @@ require_once '$classpath'; class ErrorControlChainTest extends SapphireTest { - protected $displayErrors = null; - function setUp() { - $this->displayErrors = (bool)ini_get('display_errors'); // Check we can run PHP at all $null = is_writeable('/dev/null') ? '/dev/null' : 'NUL'; @@ -80,50 +101,55 @@ class ErrorControlChainTest extends SapphireTest { parent::setUp(); } - public function tearDown() { - if($this->displayErrors !== null) { - ini_set('display_errors', $this->displayErrors); - $this->displayErrors = null; - } - parent::tearDown(); // TODO: Change the autogenerated stub - } - function testErrorSuppression() { // Errors disabled by default - ini_set('display_errors', false); - $chain = new ErrorControlChain(); + $chain = new ErrorControlChainTest_Chain(); + $chain->setDisplayErrors('Off'); // mocks display_errors: Off + $initialValue = null; $whenNotSuppressed = null; $whenSuppressed = null; - $chain->then(function($chain) use(&$whenNotSuppressed, &$whenSuppressed) { - $chain->setSuppression(true); - $whenSuppressed = ini_get('display_errors'); - $chain->setSuppression(false); - $whenNotSuppressed = ini_get('display_errors'); - })->execute(); + $chain->then( + function(ErrorControlChainTest_Chain $chain) + use(&$initialValue, &$whenNotSuppressed, &$whenSuppressed) { + $initialValue = $chain->getDisplayErrors(); + $chain->setSuppression(false); + $whenNotSuppressed = $chain->getDisplayErrors(); + $chain->setSuppression(true); + $whenSuppressed = $chain->getDisplayErrors(); + } + )->execute(); // Disabled errors never un-disable - $this->assertFalse((bool)$whenNotSuppressed); - $this->assertFalse((bool)$whenSuppressed); + $this->assertEquals(0, $initialValue); // Chain starts suppressed + $this->assertEquals(0, $whenSuppressed); // false value used internally when suppressed + $this->assertEquals('Off', $whenNotSuppressed); // false value set by php ini when suppression lifted + $this->assertEquals('Off', $chain->getDisplayErrors()); // Correctly restored after run // Errors enabled by default - ini_set('display_errors', true); - $chain = new ErrorControlChain(); + $chain = new ErrorControlChainTest_Chain(); + $chain->setDisplayErrors('Yes'); // non-falsey ini value + $initialValue = null; $whenNotSuppressed = null; $whenSuppressed = null; - $chain->then(function($chain) use(&$whenNotSuppressed, &$whenSuppressed) { - $chain->setSuppression(true); - $whenSuppressed = ini_get('display_errors'); - $chain->setSuppression(false); - $whenNotSuppressed = ini_get('display_errors'); - })->execute(); + $chain->then( + function(ErrorControlChainTest_Chain $chain) + use(&$initialValue, &$whenNotSuppressed, &$whenSuppressed) { + $initialValue = $chain->getDisplayErrors(); + $chain->setSuppression(true); + $whenSuppressed = $chain->getDisplayErrors(); + $chain->setSuppression(false); + $whenNotSuppressed = $chain->getDisplayErrors(); + } + )->execute(); // Errors can be suppressed an un-suppressed when initially enabled - $this->assertTrue((bool)$whenNotSuppressed); - $this->assertFalse((bool)$whenSuppressed); + $this->assertEquals(0, $initialValue); // Chain starts suppressed + $this->assertEquals(0, $whenSuppressed); // false value used internally when suppressed + $this->assertEquals('Yes', $whenNotSuppressed); // false value set by php ini when suppression lifted + $this->assertEquals('Yes', $chain->getDisplayErrors()); // Correctly restored after run // Fatal error - $chain = new ErrorControlChainTest_Chain(); list($out, $code) = $chain