Merge pull request #4402 from cakephp/nested-exception-fix

Refactor exception handling to return instead of echoing.

src/Error/BaseErrorHandler.php

@@ -184,15 +184,11 @@ abstract class BaseErrorHandler {
 		];
 		$this->_logError(LOG_ERR, $data);
 
-		if (ob_get_level()) {
-			ob_end_clean();
-		}
-
 		if (Configure::read('debug')) {
 			$this->handleException(new FatalErrorException($description, 500, $file, $line));
-		} else {
-			$this->handleException(new InternalErrorException());
+			return true;
 		}
+		$this->handleException(new InternalErrorException());
 		return true;
 	}
 

src/Error/ErrorHandler.php

@@ -135,7 +135,9 @@ class ErrorHandler extends BaseErrorHandler {
 				throw new \Exception("$renderer is an invalid class.");
 			}
 			$error = new $renderer($exception);
-			$error->render();
+			$response = $error->render();
+			$this->_clearOutput();
+			$this->_sendResponse($response);
 		} catch (\Exception $e) {
 			// Disable trace for internal errors.
 			$this->_options['trace'] = false;
@@ -148,4 +150,31 @@ class ErrorHandler extends BaseErrorHandler {
 		}
 	}
 
+/**
+ * Clear output buffers so error pages display properly.
+ *
+ * Easily stubbed in testing.
+ *
+ * @return void
+ */
+	protected function _clearOutput() {
+		while (ob_get_level()) {
+			ob_end_clean();
+		}
+	}
+
+/**
+ * Method that can be easily stubbed in testing.
+ *
+ * @param string|Cake\Network\Response $response Either the message or response object.
+ * @return void
+ */
+	protected function _sendResponse($response) {
+		if (is_string($response)) {
+			echo $response;
+			return;
+		}
+		echo $response->send();
+	}
+
 }

src/Error/ExceptionRenderer.php

@@ -118,7 +118,7 @@ class ExceptionRenderer {
 /**
  * Renders the response for the exception.
  *
- * @return void
+ * @return Cake\Network\Response The response to be sent.
  */
 	public function render() {
 		$exception = $this->error;
@@ -130,8 +130,7 @@ class ExceptionRenderer {
 		if (($isDebug || $exception instanceof Error\HttpException) &&
 			method_exists($this, $method)
 		) {
-			call_user_func_array(array($this, $method), array($exception));
-			return;
+			return $this->_customMethod($method, $exception);
 		}
 
 		$message = $this->_message($exception, $code);
@@ -152,10 +151,25 @@ class ExceptionRenderer {
 		if ($exception instanceof Error\Exception && $isDebug) {
 			$this->controller->set($this->error->getAttributes());
 		}
-		$this->_outputMessage($template);
+		return $this->_outputMessage($template);
 	}
 
 /**
+ * Render a custom error method/template.
+ *
+ * @param string $method The method name to invoke.
+ * @param \Exception $exception The exception to render.
+ * @return \Cake\Network\Response The response to send.
+ */
+	protected function _customMethod($method, $exception) {
+		$result = call_user_func([$this, $method], $exception);
+		if (is_string($result)) {
+			$this->controller->response->body($result);
+			$result = $this->controller->response;
+		}
+		return $result;
+	}
+/**
  * Get method name
  *
  * @param \Exception $exception Exception instance.
@@ -246,29 +260,28 @@ class ExceptionRenderer {
  * Generate the response using the controller object.
  *
  * @param string $template The template to render.
- * @return void
+ * @return Cake\Network\Response A response object that can be sent.
  */
 	protected function _outputMessage($template) {
 		try {
 			$this->controller->render($template);
 			$event = new Event('Controller.shutdown', $this->controller);
 			$this->controller->afterFilter($event);
-			$this->controller->response->send();
+			return $this->controller->response;
 		} catch (MissingViewException $e) {
 			$attributes = $e->getAttributes();
 			if (isset($attributes['file']) && strpos($attributes['file'], 'error500') !== false) {
-				$this->_outputMessageSafe('error500');
-			} else {
-				$this->_outputMessage('error500');
+				return $this->_outputMessageSafe('error500');
 			}
+			return $this->_outputMessage('error500');
 		} catch (MissingPluginException $e) {
 			$attributes = $e->getAttributes();
 			if (isset($attributes['plugin']) && $attributes['plugin'] === $this->controller->plugin) {
 				$this->controller->plugin = null;
 			}
-			$this->_outputMessageSafe('error500');
+			return $this->_outputMessageSafe('error500');
 		} catch (\Exception $e) {
-			$this->_outputMessageSafe('error500');
+			return $this->_outputMessageSafe('error500');
 		}
 	}
 
@@ -289,7 +302,7 @@ class ExceptionRenderer {
 		$view = $this->controller->createView();
 		$this->controller->response->body($view->render($template, 'error'));
 		$this->controller->response->type('html');
-		$this->controller->response->send();
+		return $this->controller->response;
 	}
 
 }

tests/TestCase/Error/ErrorHandlerTest.php

@@ -1,7 +1,5 @@
 <?php
 /**
- * ErrorHandlerTest file
- *
  * CakePHP(tm) Tests <http://book.cakephp.org/2.0/en/development/testing.html>
  * Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
  *
@@ -30,24 +28,34 @@ use Cake\Routing\Router;
 use Cake\TestSuite\TestCase;
 
 /**
- * Test exception renderer
+ * Testing stub.
  */
-class TestExceptionRenderer extends ExceptionRenderer {
+class TestErrorHandler extends ErrorHandler {
 
 /**
- * Constructor for mocking Response::_sendHeader()
+ * Access the response used.
  *
- * @param \Exception $exception
+ * @var \Cake\Network\Response
  */
-	public function __construct(\Exception $exception) {
-		parent::__construct($exception);
-		$testCase = new ErrorHandlerTest();
-		$this->controller->response = $testCase->getMock(
-			'Cake\Network\Response',
-			['_sendHeader']
-		);
+	public $response;
+
+/**
+ * Stub output clearing in tests.
+ *
+ * @return void
+ */
+	protected function _clearOutput() {
+		// noop
 	}
 
+/**
+ * Stub sending responses
+ *
+ * @return void
+ */
+	protected function _sendResponse($response) {
+		$this->response = $response;
+	}
 }
 
 /**
@@ -137,9 +145,9 @@ class ErrorHandlerTest extends TestCase {
 
 		ob_start();
 		trigger_error('Test error', $error);
-
 		$result = ob_get_clean();
-		$this->assertRegExp('/<b>' . $expected . '<\/b>/', $result);
+
+		$this->assertContains('<b>' . $expected . '</b>', $result);
 	}
 
 /**
@@ -213,14 +221,10 @@ class ErrorHandlerTest extends TestCase {
  */
 	public function testHandleException() {
 		$error = new Error\NotFoundException('Kaboom!');
-		$errorHandler = new ErrorHandler([
-			'exceptionRenderer' => 'Cake\Test\TestCase\Error\TestExceptionRenderer'
-		]);
+		$errorHandler = new TestErrorHandler();
 
-		ob_start();
 		$errorHandler->handleException($error);
-		$result = ob_get_clean();
-		$this->assertRegExp('/Kaboom!/', $result, 'message missing.');
+		$this->assertContains('Kaboom!', $errorHandler->response->body(), 'message missing.');
 	}
 
 /**
@@ -229,9 +233,8 @@ class ErrorHandlerTest extends TestCase {
  * @return void
  */
 	public function testHandleExceptionLog() {
-		$errorHandler = new ErrorHandler([
+		$errorHandler = new TestErrorHandler([
 			'log' => true,
-			'exceptionRenderer' => 'Cake\Test\TestCase\Error\TestExceptionRenderer'
 		]);
 
 		$error = new Error\NotFoundException('Kaboom!');
@@ -243,10 +246,8 @@ class ErrorHandlerTest extends TestCase {
 				$this->stringContains('ErrorHandlerTest->testHandleExceptionLog')
 			));
 
-		ob_start();
 		$errorHandler->handleException($error);
-		$result = ob_get_clean();
-		$this->assertRegExp('/Kaboom!/', $result, 'message missing.');
+		$this->assertContains('Kaboom!', $errorHandler->response->body(), 'message missing.');
 	}
 
 /**
@@ -265,21 +266,16 @@ class ErrorHandlerTest extends TestCase {
 				$this->stringContains('[Cake\Error\ForbiddenException] Fooled you!')
 			);
 
-		$errorHandler = new ErrorHandler([
+		$errorHandler = new TestErrorHandler([
 			'log' => true,
 			'skipLog' => ['Cake\Error\NotFoundException'],
-			'exceptionRenderer' => 'Cake\Test\TestCase\Error\TestExceptionRenderer'
 		]);
 
-		ob_start();
 		$errorHandler->handleException($notFound);
-		$result = ob_get_clean();
-		$this->assertRegExp('/Kaboom!/', $result, 'message missing.');
+		$this->assertContains('Kaboom!', $errorHandler->response->body(), 'message missing.');
 
-		ob_start();
 		$errorHandler->handleException($forbidden);
-		$result = ob_get_clean();
-		$this->assertRegExp('/Fooled you!/', $result, 'message missing.');
+		$this->assertContains('Fooled you!', $errorHandler->response->body(), 'message missing.');
 	}
 
 /**
@@ -289,16 +285,15 @@ class ErrorHandlerTest extends TestCase {
  */
 	public function testLoadPluginHandler() {
 		Plugin::load('TestPlugin');
-		$errorHandler = new ErrorHandler([
+		$errorHandler = new TestErrorHandler([
 			'exceptionRenderer' => 'TestPlugin.TestPluginExceptionRenderer',
 		]);
 
 		$error = new Error\NotFoundException('Kaboom!');
-		ob_start();
 		$errorHandler->handleException($error);
-		$result = ob_get_clean();
+
+		$result = $errorHandler->response;
 		$this->assertEquals('Rendered by test plugin', $result);
-		Plugin::unload();
 	}
 
 /**
@@ -310,23 +305,18 @@ class ErrorHandlerTest extends TestCase {
  */
 	public function testHandleFatalErrorPage() {
 		$line = __LINE__;
-		$errorHandler = new ErrorHandler([
-			'exceptionRenderer' => 'Cake\Test\TestCase\Error\TestExceptionRenderer'
-		]);
+		$errorHandler = new TestErrorHandler();
 		Configure::write('debug', true);
-		ob_start();
-		ob_start();
+
 		$errorHandler->handleFatalError(E_ERROR, 'Something wrong', __FILE__, $line);
-		$result = ob_get_clean();
+		$result = $errorHandler->response->body();
 		$this->assertContains('Something wrong', $result, 'message missing.');
 		$this->assertContains(__FILE__, $result, 'filename missing.');
 		$this->assertContains((string)$line, $result, 'line missing.');
 
-		ob_start();
-		ob_start();
 		Configure::write('debug', false);
 		$errorHandler->handleFatalError(E_ERROR, 'Something wrong', __FILE__, $line);
-		$result = ob_get_clean();
+		$result = $errorHandler->response->body();
 		$this->assertNotContains('Something wrong', $result, 'message must not appear.');
 		$this->assertNotContains(__FILE__, $result, 'filename must not appear.');
 		$this->assertContains('An Internal Error Has Occurred', $result);
@@ -341,7 +331,7 @@ class ErrorHandlerTest extends TestCase {
 		$this->_logger->expects($this->at(0))
 			->method('write')
 			->with('error', $this->logicalAnd(
-				$this->stringContains(__FILE__ . ', line ' . (__LINE__ + 13)),
+				$this->stringContains(__FILE__ . ', line ' . (__LINE__ + 9)),
 				$this->stringContains('Fatal Error (1)'),
 				$this->stringContains('Something wrong')
 			));
@@ -349,13 +339,8 @@ class ErrorHandlerTest extends TestCase {
 			->method('write')
 			->with('error', $this->stringContains('[Cake\Error\FatalErrorException] Something wrong'));
 
-		$errorHandler = new ErrorHandler([
-			'log' => true,
-			'exceptionRenderer' => 'Cake\Test\TestCase\Error\TestExceptionRenderer'
-		]);
-		ob_start();
+		$errorHandler = new TestErrorHandler(['log' => true]);
 		$errorHandler->handleFatalError(E_ERROR, 'Something wrong', __FILE__, __LINE__);
-		ob_clean();
 	}
 
 }

tests/TestCase/Error/ExceptionRendererTest.php

@@ -116,7 +116,7 @@ class MyCustomExceptionRenderer extends ExceptionRenderer {
  * @return void
  */
 	public function missingWidgetThing() {
-		echo 'widget thing is missing';
+		return 'widget thing is missing';
 	}
 
 }
@@ -184,16 +184,12 @@ class ExceptionRendererTest extends TestCase {
  * @return void
  */
 	public function testSubclassMethodsNotBeingConvertedToError() {
-		Configure::write('debug', true);
-
 		$exception = new MissingWidgetThingException('Widget not found');
 		$ExceptionRenderer = $this->_mockResponse(new MyCustomExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render();
 
-		$this->assertEquals('widget thing is missing', $result);
+		$this->assertEquals('widget thing is missing', $result->body());
 	}
 
 /**
@@ -206,12 +202,14 @@ class ExceptionRendererTest extends TestCase {
 		$exception = new MissingWidgetThingException('Widget not found');
 		$ExceptionRenderer = $this->_mockResponse(new MyCustomExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render();
 
 		$this->assertEquals('missingWidgetThing', $ExceptionRenderer->method);
-		$this->assertEquals('widget thing is missing', $result, 'Method declared in subclass converted to error400');
+		$this->assertEquals(
+			'widget thing is missing',
+			$result->body(),
+			'Method declared in subclass converted to error400'
+		);
 	}
 
 /**
@@ -225,11 +223,13 @@ class ExceptionRendererTest extends TestCase {
 		$exception = new MissingControllerException('PostsController');
 		$ExceptionRenderer = $this->_mockResponse(new MyCustomExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render();
 
-		$this->assertRegExp('/Not Found/', $result, 'Method declared in error handler not converted to error400. %s');
+		$this->assertRegExp(
+			'/Not Found/',
+			$result->body(),
+			'Method declared in error handler not converted to error400. %s'
+		);
 	}
 
 /**
@@ -258,9 +258,7 @@ class ExceptionRendererTest extends TestCase {
 		$this->assertInstanceOf('Cake\Controller\ErrorController', $ExceptionRenderer->controller);
 		$this->assertEquals($exception, $ExceptionRenderer->error);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
 		$this->assertEquals('error400', $ExceptionRenderer->template);
 		$this->assertContains('Not Found', $result);
@@ -277,10 +275,8 @@ class ExceptionRendererTest extends TestCase {
 		$exception = new SocketException('socket exception');
 		$renderer = $this->_mockResponse(new \TestApp\Error\TestAppsExceptionRenderer($exception));
 
-		ob_start();
-		$renderer->render();
-		$result = ob_get_clean();
-		$this->assertContains('<b>peeled</b>', $result);
+		$result = $renderer->render();
+		$this->assertContains('<b>peeled</b>', $result->body());
 	}
 
 /**
@@ -294,12 +290,10 @@ class ExceptionRendererTest extends TestCase {
 		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('statusCode', '_sendHeader'));
 		$ExceptionRenderer->controller->response->expects($this->once())->method('statusCode')->with(404);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render();
 
 		$this->assertFalse(method_exists($ExceptionRenderer, 'missingWidgetThing'), 'no method should exist.');
-		$this->assertContains('coding fail', $result, 'Text should show up.');
+		$this->assertContains('coding fail', $result->body(), 'Text should show up.');
 	}
 
 /**
@@ -315,11 +309,9 @@ class ExceptionRendererTest extends TestCase {
 			->method('statusCode')
 			->with(500);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render();
 
-		$this->assertContains('foul ball.', $result, 'Text should show up as its debug mode.');
+		$this->assertContains('foul ball.', $result->body(), 'Text should show up as its debug mode.');
 	}
 
 /**
@@ -337,9 +329,7 @@ class ExceptionRendererTest extends TestCase {
 			->method('statusCode')
 			->with(500);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
 		$this->assertNotContains('foul ball.', $result, 'Text should no show up.');
 		$this->assertContains('Internal Error', $result, 'Generic message only.');
@@ -356,11 +346,9 @@ class ExceptionRendererTest extends TestCase {
 		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('statusCode', '_sendHeader'));
 		$ExceptionRenderer->controller->response->expects($this->once())->method('statusCode')->with(501);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render();
 
-		$this->assertContains('foul ball.', $result, 'Text should show up as its debug mode.');
+		$this->assertContains('foul ball.', $result->body(), 'Text should show up as its debug mode.');
 	}
 
 /**
@@ -379,11 +367,9 @@ class ExceptionRendererTest extends TestCase {
 		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('statusCode', '_sendHeader'));
 		$ExceptionRenderer->controller->response->expects($this->once())->method('statusCode')->with(404);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
-		$this->assertRegExp('/<h2>Custom message<\/h2>/', $result);
+		$this->assertContains('<h2>Custom message</h2>', $result);
 		$this->assertRegExp("/<strong>'.*?\/posts\/view\/1000'<\/strong>/", $result);
 	}
 
@@ -398,18 +384,14 @@ class ExceptionRendererTest extends TestCase {
 		$exception = new Error\NotFoundException('Custom message');
 		$ExceptionRenderer = $this->_mockResponse(new ExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
-		$this->assertContains('Custom message', $result);
+		$result = $ExceptionRenderer->render();
+		$this->assertContains('Custom message', $result->body());
 
 		$exception = new MissingActionException(array('controller' => 'PostsController', 'action' => 'index'));
 		$ExceptionRenderer = $this->_mockResponse(new ExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
-		$this->assertContains('Not Found', $result);
+		$result = $ExceptionRenderer->render();
+		$this->assertContains('Not Found', $result->body());
 	}
 
 /**
@@ -426,12 +408,10 @@ class ExceptionRendererTest extends TestCase {
 		$exception = new Error\NotFoundException('Custom message');
 		$ExceptionRenderer = $this->_mockResponse(new ExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
-		$this->assertNotRegExp('#<script>document#', $result);
-		$this->assertNotRegExp('#alert\(t\);</script>#', $result);
+		$this->assertNotContains('<script>document', $result);
+		$this->assertNotContains('alert(t);</script>', $result);
 	}
 
 /**
@@ -445,11 +425,8 @@ class ExceptionRendererTest extends TestCase {
 		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('statusCode', '_sendHeader'));
 		$ExceptionRenderer->controller->response->expects($this->once())->method('statusCode')->with(500);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
-
-		$this->assertRegExp('/<h2>An Internal Error Has Occurred<\/h2>/', $result);
+		$result = $ExceptionRenderer->render();
+		$this->assertContains('<h2>An Internal Error Has Occurred</h2>', $result->body());
 	}
 
 /**
@@ -462,15 +439,10 @@ class ExceptionRendererTest extends TestCase {
 		$exception->responseHeader(array('Allow: POST, DELETE'));
 		$ExceptionRenderer = new ExceptionRenderer($exception);
 
-		//Replace response object with mocked object add back the original headers which had been set in ExceptionRenderer constructor
-		$headers = $ExceptionRenderer->controller->response->header();
-		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('_sendHeader'));
-		$ExceptionRenderer->controller->response->header($headers);
-
-		$ExceptionRenderer->controller->response->expects($this->at(1))->method('_sendHeader')->with('Allow', 'POST, DELETE');
-		ob_start();
-		$ExceptionRenderer->render();
-		ob_get_clean();
+		$result = $ExceptionRenderer->render();
+		$headers = $result->header();
+		$this->assertArrayHasKey('Allow', $headers);
+		$this->assertEquals('POST, DELETE', $headers['Allow']);
 	}
 
 /**
@@ -486,13 +458,11 @@ class ExceptionRendererTest extends TestCase {
 		));
 		$ExceptionRenderer = $this->_mockResponse(new MyCustomExceptionRenderer($exception));
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
 		$this->assertEquals('missingController', $ExceptionRenderer->template);
-		$this->assertRegExp('/<h2>Missing Controller<\/h2>/', $result);
-		$this->assertRegExp('/<em>PostsController<\/em>/', $result);
+		$this->assertContains('<h2>Missing Controller</h2>', $result);
+		$this->assertContains('<em>PostsController</em>', $result);
 	}
 
 /**
@@ -617,9 +587,7 @@ class ExceptionRendererTest extends TestCase {
 			->method('statusCode')
 			->with($code);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
 		foreach ($patterns as $pattern) {
 			$this->assertRegExp($pattern, $result);
@@ -790,16 +758,10 @@ class ExceptionRendererTest extends TestCase {
 
 		$exception = new \Exception('Terrible');
 		$ExceptionRenderer = new ExceptionRenderer($exception);
-		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('statusCode', '_sendHeader'));
-		$ExceptionRenderer->controller->response->expects($this->once())
-			->method('statusCode')
-			->with(500);
+		$result = $ExceptionRenderer->render();
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
-
-		$this->assertContains('Internal Error', $result);
+		$this->assertContains('Internal Error', $result->body());
+		$this->assertEquals(500, $result->statusCode());
 	}
 
 /**
@@ -815,9 +777,7 @@ class ExceptionRendererTest extends TestCase {
 		$ExceptionRenderer->controller->response = $this->getMock('Cake\Network\Response', array('statusCode', '_sendHeader'));
 		$ExceptionRenderer->controller->response->expects($this->once())->method('statusCode')->with(500);
 
-		ob_start();
-		$ExceptionRenderer->render();
-		$result = ob_get_clean();
+		$result = $ExceptionRenderer->render()->body();
 
 		$this->assertContains('<h2>Database Error</h2>', $result);
 		$this->assertContains('There was an error in the SQL query', $result);

tests/test_app/Plugin/TestPlugin/src/Error/TestPluginExceptionRenderer.php

@@ -30,9 +30,9 @@ class TestPluginExceptionRenderer extends ExceptionRenderer {
 /**
  * Renders the response for the exception.
  *
- * @return void
+ * @return string
  */
 	public function render() {
-		echo 'Rendered by test plugin';
+		return 'Rendered by test plugin';
 	}
 }