From f621bb7fe5c6e648630ac615b4e90a08d290708f Mon Sep 17 00:00:00 2001 From: David Steinsland Date: Thu, 15 Jan 2015 19:20:28 +0100 Subject: [PATCH 1/6] Fixed nesting when ExceptionRenderer throws exception --- lib/Cake/Error/ErrorHandler.php | 13 +++++-- lib/Cake/Test/Case/Error/ErrorHandlerTest.php | 34 +++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/lib/Cake/Error/ErrorHandler.php b/lib/Cake/Error/ErrorHandler.php index e87c636d1..18efed6ce 100644 --- a/lib/Cake/Error/ErrorHandler.php +++ b/lib/Cake/Error/ErrorHandler.php @@ -125,6 +125,8 @@ class ErrorHandler { $e->getMessage(), $e->getTraceAsString() ); + + Configure::write('Exception.bail', true); trigger_error($message, E_USER_ERROR); } } @@ -249,10 +251,17 @@ class ErrorHandler { } if (Configure::read('debug')) { - call_user_func($exceptionHandler, new FatalErrorException($description, 500, $file, $line)); + $exception = new FatalErrorException($description, 500, $file, $line); } else { - call_user_func($exceptionHandler, new InternalErrorException()); + $exception = new InternalErrorException(); } + + if (Configure::read('Exception.bail')) { + throw $exception; + } + + call_user_func($exceptionHandler, $exception); + return false; } diff --git a/lib/Cake/Test/Case/Error/ErrorHandlerTest.php b/lib/Cake/Test/Case/Error/ErrorHandlerTest.php index 6ec656036..b2f87882d 100644 --- a/lib/Cake/Test/Case/Error/ErrorHandlerTest.php +++ b/lib/Cake/Test/Case/Error/ErrorHandlerTest.php @@ -20,6 +20,16 @@ App::uses('ErrorHandler', 'Error'); App::uses('Controller', 'Controller'); App::uses('Router', 'Routing'); +/** + * A faulty ExceptionRenderer to test nesting. + */ +class FaultyExceptionRenderer extends ExceptionRenderer { + + public function render() { + throw new Exception('Error from renderer.'); + } +} + /** * ErrorHandlerTest class * @@ -320,4 +330,28 @@ class ErrorHandlerTest extends CakeTestCase { $this->assertContains('[FatalErrorException] Something wrong', $log[1], 'message missing.'); } +/** + * testExceptionRendererNestingDebug method + * + * @expectedException FatalErrorException + * @return void + */ + public function testExceptionRendererNestingDebug() { + Configure::write('debug', 2); + Configure::write('Exception.renderer', 'FaultyExceptionRenderer'); + ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__ ,__LINE__); + } + +/** + * testExceptionRendererNestingProduction method + * + * @expectedException InternalErrorException + * @return void + */ + public function testExceptionRendererNestingProduction() { + Configure::write('debug', 0); + Configure::write('Exception.renderer', 'FaultyExceptionRenderer'); + ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__ ,__LINE__); + } + } From ed3b15f13b4b7e70c3d49d3aa05ce9a68a2ede4a Mon Sep 17 00:00:00 2001 From: David Steinsland Date: Thu, 15 Jan 2015 20:04:06 +0100 Subject: [PATCH 2/6] Fixed phpcs --- lib/Cake/Error/ErrorHandler.php | 2 ++ lib/Cake/Test/Case/Error/ErrorHandlerTest.php | 11 +++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/Cake/Error/ErrorHandler.php b/lib/Cake/Error/ErrorHandler.php index 18efed6ce..e7fe53da5 100644 --- a/lib/Cake/Error/ErrorHandler.php +++ b/lib/Cake/Error/ErrorHandler.php @@ -236,6 +236,8 @@ class ErrorHandler { * @param string $file File on which error occurred * @param int $line Line that triggered the error * @return bool + * @throws FatalErrorException If the Exception renderer threw an exception during rendering, and debug > 0. + * @throws InternalErrorException If the Exception renderer threw an exception during rendering, and debug is 0. */ public static function handleFatalError($code, $description, $file, $line) { $logMessage = 'Fatal Error (' . $code . '): ' . $description . ' in [' . $file . ', line ' . $line . ']'; diff --git a/lib/Cake/Test/Case/Error/ErrorHandlerTest.php b/lib/Cake/Test/Case/Error/ErrorHandlerTest.php index b2f87882d..2db99959f 100644 --- a/lib/Cake/Test/Case/Error/ErrorHandlerTest.php +++ b/lib/Cake/Test/Case/Error/ErrorHandlerTest.php @@ -25,9 +25,16 @@ App::uses('Router', 'Routing'); */ class FaultyExceptionRenderer extends ExceptionRenderer { +/** + * Dummy rendering implementation. + * + * @return void + * @throws Exception + */ public function render() { throw new Exception('Error from renderer.'); } + } /** @@ -339,7 +346,7 @@ class ErrorHandlerTest extends CakeTestCase { public function testExceptionRendererNestingDebug() { Configure::write('debug', 2); Configure::write('Exception.renderer', 'FaultyExceptionRenderer'); - ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__ ,__LINE__); + ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__, __LINE__); } /** @@ -351,7 +358,7 @@ class ErrorHandlerTest extends CakeTestCase { public function testExceptionRendererNestingProduction() { Configure::write('debug', 0); Configure::write('Exception.renderer', 'FaultyExceptionRenderer'); - ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__ ,__LINE__); + ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__, __LINE__); } } From e5134986e616a3bc137b97ba0b0424cbcd48c3ec Mon Sep 17 00:00:00 2001 From: David Steinsland Date: Thu, 15 Jan 2015 20:10:48 +0100 Subject: [PATCH 3/6] Reset Exception.bail just in case --- lib/Cake/Error/ErrorHandler.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Cake/Error/ErrorHandler.php b/lib/Cake/Error/ErrorHandler.php index e7fe53da5..33a7e764e 100644 --- a/lib/Cake/Error/ErrorHandler.php +++ b/lib/Cake/Error/ErrorHandler.php @@ -259,6 +259,7 @@ class ErrorHandler { } if (Configure::read('Exception.bail')) { + Configure::write('Exception.bail', false); throw $exception; } From a5e1be7abfb3b45d3b588e99105c782ccd17a259 Mon Sep 17 00:00:00 2001 From: David Steinsland Date: Thu, 15 Jan 2015 23:11:36 +0100 Subject: [PATCH 4/6] Fixed tests --- lib/Cake/Test/Case/Error/ErrorHandlerTest.php | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/lib/Cake/Test/Case/Error/ErrorHandlerTest.php b/lib/Cake/Test/Case/Error/ErrorHandlerTest.php index 2db99959f..aa7448d64 100644 --- a/lib/Cake/Test/Case/Error/ErrorHandlerTest.php +++ b/lib/Cake/Test/Case/Error/ErrorHandlerTest.php @@ -340,25 +340,45 @@ class ErrorHandlerTest extends CakeTestCase { /** * testExceptionRendererNestingDebug method * - * @expectedException FatalErrorException * @return void */ public function testExceptionRendererNestingDebug() { Configure::write('debug', 2); Configure::write('Exception.renderer', 'FaultyExceptionRenderer'); - ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__, __LINE__); + + $result = false; + try { + ob_start(); + ob_start(); + ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__, __LINE__); + } catch (Exception $e) { + $result = $e instanceof FatalErrorException; + } + + restore_error_handler(); + $this->assertTrue($result); } /** * testExceptionRendererNestingProduction method * - * @expectedException InternalErrorException * @return void */ public function testExceptionRendererNestingProduction() { Configure::write('debug', 0); Configure::write('Exception.renderer', 'FaultyExceptionRenderer'); - ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__, __LINE__); + + $result = false; + try { + ob_start(); + ob_start(); + ErrorHandler::handleFatalError(E_USER_ERROR, 'Initial error', __FILE__, __LINE__); + } catch (Exception $e) { + $result = $e instanceof InternalErrorException; + } + + restore_error_handler(); + $this->assertTrue($result); } } From e37db25ce0ece51a63a76a26ef08ad6781ca88dc Mon Sep 17 00:00:00 2001 From: David Steinsland Date: Fri, 16 Jan 2015 14:47:44 +0100 Subject: [PATCH 5/6] Change configuration property to static class property --- lib/Cake/Error/ErrorHandler.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/Cake/Error/ErrorHandler.php b/lib/Cake/Error/ErrorHandler.php index 33a7e764e..e89574a85 100644 --- a/lib/Cake/Error/ErrorHandler.php +++ b/lib/Cake/Error/ErrorHandler.php @@ -95,6 +95,14 @@ App::uses('Router', 'Routing'); */ class ErrorHandler { +/** + * Whether to give up rendering an exception, if the renderer itself is + * throwing exceptions. + * + * @var bool + */ + protected static $_bailExceptionRendering = false; + /** * Set as the default exception handler by the CakePHP bootstrap process. * @@ -126,7 +134,7 @@ class ErrorHandler { $e->getTraceAsString() ); - Configure::write('Exception.bail', true); + static::$_bailExceptionRendering = true; trigger_error($message, E_USER_ERROR); } } @@ -258,8 +266,8 @@ class ErrorHandler { $exception = new InternalErrorException(); } - if (Configure::read('Exception.bail')) { - Configure::write('Exception.bail', false); + if (static::$_bailExceptionRendering) { + static::$_bailExceptionRendering = false; throw $exception; } From e5d1846d4b59e1ebd960ddeca600d17b6cb091d0 Mon Sep 17 00:00:00 2001 From: David Steinsland Date: Fri, 16 Jan 2015 16:02:44 +0100 Subject: [PATCH 6/6] Fixed PHP 5.2 compatibility --- lib/Cake/Error/ErrorHandler.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Cake/Error/ErrorHandler.php b/lib/Cake/Error/ErrorHandler.php index e89574a85..5ecba0960 100644 --- a/lib/Cake/Error/ErrorHandler.php +++ b/lib/Cake/Error/ErrorHandler.php @@ -134,7 +134,7 @@ class ErrorHandler { $e->getTraceAsString() ); - static::$_bailExceptionRendering = true; + self::$_bailExceptionRendering = true; trigger_error($message, E_USER_ERROR); } } @@ -266,8 +266,8 @@ class ErrorHandler { $exception = new InternalErrorException(); } - if (static::$_bailExceptionRendering) { - static::$_bailExceptionRendering = false; + if (self::$_bailExceptionRendering) { + self::$_bailExceptionRendering = false; throw $exception; }