Refactor exception handling to return instead of echoing.

By returning a response/string instead of directly echoing we get a few benefits: 1. We can more easily test the code without relying on ob_start(). 2. Application exception handlers can have full control over the response headers. 3. We can fix issues related to nested error pages. Refs #4130
cakephp · Aug 26, 2014 · 178ad34 · 178ad34
1 parent d3e760f
commit 178ad34
Show file tree

Hide file tree

Showing 6 changed files with 146 additions and 163 deletions.
diff --git a/src/Error/BaseErrorHandler.php b/src/Error/BaseErrorHandler.php
@@ -184,15 +184,11 @@ public function handleFatalError($code, $description, $file, $line) {
 		];
 		$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;
 	}
 

diff --git a/src/Error/ErrorHandler.php b/src/Error/ErrorHandler.php
@@ -135,7 +135,9 @@ protected function _displayException($exception) {
 				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 @@ protected function _displayException($exception) {
 		}
 	}
 
+/**
+ * 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();
+	}
+
 }
diff --git a/src/Error/ExceptionRenderer.php b/src/Error/ExceptionRenderer.php
@@ -118,7 +118,7 @@ protected function _getController() {
 /**
  * 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 @@ public function render() {
 		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,9 +151,24 @@ public function render() {
 		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
  *
@@ -246,29 +260,28 @@ protected function _code(\Exception $exception) {
  * 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 @@ protected function _outputMessageSafe($template) {
 		$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;
 	}
 
 }
diff --git a/tests/TestCase/Error/ErrorHandlerTest.php b/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\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 @@ public function testErrorMapping($error, $expected) {
 
 		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 @@ public function testHandleErrorLoggingTrace() {
  */
 	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 @@ public function testHandleException() {
  * @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 @@ public function testHandleExceptionLog() {
 				$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 @@ public function testHandleExceptionLogSkipping() {
 				$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 @@ public function testHandleExceptionLogSkipping() {
  */
 	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 @@ public function testLoadPluginHandler() {
  */
 	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,21 +331,16 @@ public function testHandleFatalErrorLog() {
 		$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')
 			));
 		$this->_logger->expects($this->at(1))
 			->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();
 	}
 
 }