From 39952f4a5ca313491984f9ef7d4d00ee4a41b23a Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Thu, 27 Sep 2012 12:26:25 +1200 Subject: [PATCH] API: Added 'onBeforeHTTPError' and 'onBeforeHTTPError' extension points to RequestHandler::httpError(). These APIs are primarily intended to let developers write custom 404 handlers. They can define an onBeforeHTTPError404() method on an Extension that gets added to Controller or RequestHandler. The SS_HTTPResponse_Exception object has also been tidied up to override the status info of any SS_HTTPResponse object that might get passed. This is mainly to make it easier for callers (such as ContentController and ModelAsController) to use RequestHandler::httpError() more consistently. --- control/HTTPResponse.php | 16 ++++++++++++-- control/RequestHandler.php | 10 +++++---- tests/control/HTTPResponseTest.php | 31 +++++++++++++++++++++++++++ tests/control/RequestHandlingTest.php | 26 ++++++++++++++++++++++ 4 files changed, 77 insertions(+), 6 deletions(-) diff --git a/control/HTTPResponse.php b/control/HTTPResponse.php index 1894f49b6..3f01bc5cf 100644 --- a/control/HTTPResponse.php +++ b/control/HTTPResponse.php @@ -293,13 +293,25 @@ class SS_HTTPResponse_Exception extends Exception { protected $response; /** + * @param string|SS_HTTPResponse body Either the plaintext content of the error message, or an SS_HTTPResponse + * object representing it. In either case, the $statusCode and + * $statusDescription will be the HTTP status of the resulting response. * @see SS_HTTPResponse::__construct(); */ public function __construct($body = null, $statusCode = null, $statusDescription = null) { if($body instanceof SS_HTTPResponse) { - $this->setResponse($body); + // statusCode and statusDescription should override whatever is passed in the body + if($statusCode) $body->setStatusCode($statusCode); + if($statusDescription) $body->setStatusDescription($statusDescription); + + $this->setResponse($body); } else { - $this->setResponse(new SS_HTTPResponse($body, $statusCode, $statusDescription)); + $response = new SS_HTTPResponse($body, $statusCode, $statusDescription); + + // Error responses should always be considered plaintext, for security reasons + $response->addHeader('Content-Type', 'text/plain'); + + $this->setResponse($response); } parent::__construct($this->getResponse()->getBody(), $this->getResponse()->getStatusCode()); diff --git a/control/RequestHandler.php b/control/RequestHandler.php index f25d148c4..3b3272845 100644 --- a/control/RequestHandler.php +++ b/control/RequestHandler.php @@ -332,12 +332,14 @@ class RequestHandler extends ViewableData { * @uses SS_HTTPResponse_Exception */ public function httpError($errorCode, $errorMessage = null) { - $e = new SS_HTTPResponse_Exception($errorMessage, $errorCode); + // Call a handler method such as onBeforeHTTPError404 + $this->extend('onBeforeHTTPError' . $errorCode, $this->request); - // Error responses should always be considered plaintext, for security reasons - $e->getResponse()->addHeader('Content-Type', 'text/plain'); + // Call a handler method such as onBeforeHTTPError, passing 404 as the first arg + $this->extend('onBeforeHTTPError', $errorCode, $this->request); - throw $e; + // Throw a new exception + throw new SS_HTTPResponse_Exception($errorMessage, $errorCode); } /** diff --git a/tests/control/HTTPResponseTest.php b/tests/control/HTTPResponseTest.php index f2f37c693..f1b02e048 100644 --- a/tests/control/HTTPResponseTest.php +++ b/tests/control/HTTPResponseTest.php @@ -30,4 +30,35 @@ class HTTPResponseTest extends SapphireTest { ); } + public function testHTTPResponseException() { + $response = new SS_HTTPResponse("Test", 200, 'OK'); + + // Confirm that the exception's statusCode and statusDescription take precedence + try { + throw new SS_HTTPResponse_Exception($response, 404, 'not even found'); + + } catch(SS_HTTPResponse_Exception $e) { + $this->assertEquals(404, $e->getResponse()->getStatusCode()); + $this->assertEquals('not even found', $e->getResponse()->getStatusDescription()); + return; + } + // Fail if we get to here + $this->assertFalse(true, 'Something went wrong with our test exception'); + + } + + public function testExceptionContentPlainByDefault() { + + // Confirm that the exception's statusCode and statusDescription take precedence + try { + throw new SS_HTTPResponse_Exception("Some content that may be from a hacker", 404, 'not even found'); + + } catch(SS_HTTPResponse_Exception $e) { + $this->assertEquals("text/plain", $e->getResponse()->getHeader("Content-Type")); + return; + } + // Fail if we get to here + $this->assertFalse(true, 'Something went wrong with our test exception'); + + } } diff --git a/tests/control/RequestHandlingTest.php b/tests/control/RequestHandlingTest.php index 9c210f72e..9ea10b9b8 100644 --- a/tests/control/RequestHandlingTest.php +++ b/tests/control/RequestHandlingTest.php @@ -146,9 +146,17 @@ class RequestHandlingTest extends FunctionalTest { } public function testHTTPError() { + RequestHandlingTest_ControllerExtension::$called_error = false; + RequestHandlingTest_ControllerExtension::$called_404_error = false; + $response = Director::test('RequestHandlingTest_Controller/throwhttperror'); $this->assertEquals(404, $response->getStatusCode()); $this->assertEquals('This page does not exist.', $response->getBody()); + + // Confirm that RequestHandlingTest_ControllerExtension::onBeforeHTTPError() called + $this->assertTrue(RequestHandlingTest_ControllerExtension::$called_error); + // Confirm that RequestHandlingTest_ControllerExtension::onBeforeHTTPError404() called + $this->assertTrue(RequestHandlingTest_ControllerExtension::$called_404_error); } public function testMethodsOnParentClassesOfRequestHandlerDeclined() { @@ -401,9 +409,27 @@ class RequestHandlingTest_FormActionController extends Controller { * Simple extension for the test controller */ class RequestHandlingTest_ControllerExtension extends Extension { + public static $called_error = false; + public static $called_404_error = false; + public function extendedMethod() { return "extendedMethod"; } + + /** + * Called whenever there is an HTTP error + */ + public function onBeforeHTTPError() { + self::$called_error = true; + } + + /** + * Called whenever there is an 404 error + */ + public function onBeforeHTTPError404() { + self::$called_404_error = true; + } + } /**