From b00d02496e9ec8281a6e0d6637772c55a7011c60 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 22 Nov 2014 23:10:19 +0100 Subject: [PATCH 1/5] [ticket/13361] Improve the exception listener PHPBB3-13361 --- phpBB/phpbb/controller/helper.php | 2 + .../event/kernel_exception_subscriber.php | 21 ++++-- phpBB/phpbb/exception/exception.php | 52 ++++++++++++++ phpBB/phpbb/exception/exception_interface.php | 29 ++++++++ phpBB/phpbb/exception/http_exception.php | 68 +++++++++++++++++++ 5 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 phpBB/phpbb/exception/exception.php create mode 100644 phpBB/phpbb/exception/exception_interface.php create mode 100644 phpBB/phpbb/exception/http_exception.php diff --git a/phpBB/phpbb/controller/helper.php b/phpBB/phpbb/controller/helper.php index 52e6947c2c..7ee90b10ba 100644 --- a/phpBB/phpbb/controller/helper.php +++ b/phpBB/phpbb/controller/helper.php @@ -184,6 +184,8 @@ class helper * @param string $message The error message * @param int $code The error code (e.g. 404, 500, 503, etc.) * @return Response A Response instance + * + * @deprecated 3.1.3 (To be removed: 3.3.0) Use exceptions instead. */ public function error($message, $code = 500) { diff --git a/phpBB/phpbb/event/kernel_exception_subscriber.php b/phpBB/phpbb/event/kernel_exception_subscriber.php index 44e87507f9..1e536c1b8f 100644 --- a/phpBB/phpbb/event/kernel_exception_subscriber.php +++ b/phpBB/phpbb/event/kernel_exception_subscriber.php @@ -14,9 +14,9 @@ namespace phpbb\event; use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; -use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpFoundation\Response; class kernel_exception_subscriber implements EventSubscriberInterface @@ -57,9 +57,16 @@ class kernel_exception_subscriber implements EventSubscriberInterface $exception = $event->getException(); + $message = $exception->getMessage(); + + if ($exception instanceof \phpbb\exception\exception_interface) + { + $message = call_user_func_array(array($this->user, 'lang'), array_merge(array($message), $exception->get_parameters())); + } + $this->template->assign_vars(array( 'MESSAGE_TITLE' => $this->user->lang('INFORMATION'), - 'MESSAGE_TEXT' => $exception->getMessage(), + 'MESSAGE_TEXT' => $message, )); $this->template->set_filenames(array( @@ -68,8 +75,14 @@ class kernel_exception_subscriber implements EventSubscriberInterface page_footer(true, false, false); - $status_code = $exception instanceof HttpException ? $exception->getStatusCode() : 500; - $response = new Response($this->template->assign_display('body'), $status_code); + $response = new Response($this->template->assign_display('body'), 500); + + if ($exception instanceof HttpExceptionInterface) + { + $response->setStatusCode($exception->getStatusCode()); + $response->headers->add($exception->getHeaders()); + } + $event->setResponse($response); } diff --git a/phpBB/phpbb/exception/exception.php b/phpBB/phpbb/exception/exception.php new file mode 100644 index 0000000000..077d0d258a --- /dev/null +++ b/phpBB/phpbb/exception/exception.php @@ -0,0 +1,52 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\exception; + +/** + * Class exception + * + * Define an exception which support a language var as message. + */ +class exception extends \RuntimeException implements exception_interface +{ + /** + * Parameters to use with the language var. + * + * @var array + */ + private $parameters; + + /** + * Constructor + * + * @param string $message The Exception message to throw (must be a language variable). + * @param array $parameters The parameters to use with the language var. + * @param \Exception $previous The previous exception used for the exception chaining. + * @param integer $code The Exception code. + */ + public function __construct($message = "", array $parameters = array(), \Exception $previous = null, $code = 0) + { + $this->parameters = $parameters; + + parent::__construct($message, $code, $previous); + } + + /** + * {@inheritdoc} + */ + public function get_parameters() + { + return $this->parameters; + } +} diff --git a/phpBB/phpbb/exception/exception_interface.php b/phpBB/phpbb/exception/exception_interface.php new file mode 100644 index 0000000000..e8526a35f5 --- /dev/null +++ b/phpBB/phpbb/exception/exception_interface.php @@ -0,0 +1,29 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\exception; + +/** + * Interface exception_interface + * + * Define an exception which support a language var as message. + */ +interface exception_interface +{ + /** + * Return the arguments associated with the message if it's a language var. + * + * @return array + */ + public function get_parameters(); +} diff --git a/phpBB/phpbb/exception/http_exception.php b/phpBB/phpbb/exception/http_exception.php new file mode 100644 index 0000000000..57604d49d4 --- /dev/null +++ b/phpBB/phpbb/exception/http_exception.php @@ -0,0 +1,68 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\exception; + +/** + * Class http_exception + */ +class http_exception extends exception implements exception_interface +{ + /** + * Http status code. + * + * @var integer + */ + private $status_code; + + /** + * Additional headers to set in the response. + * + * @var array + */ + private $headers; + + /** + * Constructor + * + * @param integer $status_code The http status code. + * @param string $message The Exception message to throw (must be a language variable). + * @param array $parameters The parameters to use with the language var. + * @param \Exception $previous The previous exception used for the exception chaining. + * @param array $headers Additional headers to set in the response. + * @param integer $code The Exception code. + */ + public function __construct($status_code, $message = "", array $parameters = array(), \Exception $previous = null, array $headers = array(), $code = 0) + { + $this->status_code = $status_code; + $this->headers = $headers; + + parent::__construct($message, $code, $previous); + } + + /** + * {@inheritdoc} + */ + public function getStatusCode() + { + return $this->status_code; + } + + /** + * {@inheritdoc} + */ + public function getHeaders() + { + return $this->headers; + } +} From 74e8f9bd4e6c55dddb473e1a301d2cae9edb6e50 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 10 Jan 2015 17:14:14 +0100 Subject: [PATCH 2/5] [ticket/13361] Support ajax request (send a json response) PHPBB3-13361 --- .../event/kernel_exception_subscriber.php | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/phpBB/phpbb/event/kernel_exception_subscriber.php b/phpBB/phpbb/event/kernel_exception_subscriber.php index 1e536c1b8f..b6b34675f0 100644 --- a/phpBB/phpbb/event/kernel_exception_subscriber.php +++ b/phpBB/phpbb/event/kernel_exception_subscriber.php @@ -14,6 +14,7 @@ namespace phpbb\event; use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; @@ -64,18 +65,33 @@ class kernel_exception_subscriber implements EventSubscriberInterface $message = call_user_func_array(array($this->user, 'lang'), array_merge(array($message), $exception->get_parameters())); } - $this->template->assign_vars(array( - 'MESSAGE_TITLE' => $this->user->lang('INFORMATION'), - 'MESSAGE_TEXT' => $message, - )); + if (!$event->getRequest()->isXmlHttpRequest()) + { + $this->template->assign_vars(array( + 'MESSAGE_TITLE' => $this->user->lang('INFORMATION'), + 'MESSAGE_TEXT' => $message, + )); - $this->template->set_filenames(array( - 'body' => 'message_body.html', - )); + $this->template->set_filenames(array( + 'body' => 'message_body.html', + )); - page_footer(true, false, false); + page_footer(true, false, false); - $response = new Response($this->template->assign_display('body'), 500); + $response = new Response($this->template->assign_display('body'), 500); + } + else + { + $data = array(); + $data['message'] = $message; + + if (defined('DEBUG')) + { + $data['trace'] = $exception->getTrace(); + } + + $response = new JsonResponse($message, 500); + } if ($exception instanceof HttpExceptionInterface) { From 1f4bb2c1495e8d2641abe9606f14281e810feee4 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 10 Jan 2015 18:15:19 +0100 Subject: [PATCH 3/5] [ticket/13361] Add tests PHPBB3-13361 --- .../event/kernel_exception_subscriber.php | 10 +- phpBB/phpbb/exception/http_exception.php | 6 +- tests/event/exception_listener.php | 100 ++++++++++++++++++ 3 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 tests/event/exception_listener.php diff --git a/phpBB/phpbb/event/kernel_exception_subscriber.php b/phpBB/phpbb/event/kernel_exception_subscriber.php index b6b34675f0..01940a4fa9 100644 --- a/phpBB/phpbb/event/kernel_exception_subscriber.php +++ b/phpBB/phpbb/event/kernel_exception_subscriber.php @@ -54,8 +54,6 @@ class kernel_exception_subscriber implements EventSubscriberInterface */ public function on_kernel_exception(GetResponseForExceptionEvent $event) { - page_header($this->user->lang('INFORMATION')); - $exception = $event->getException(); $message = $exception->getMessage(); @@ -67,6 +65,8 @@ class kernel_exception_subscriber implements EventSubscriberInterface if (!$event->getRequest()->isXmlHttpRequest()) { + page_header($this->user->lang('INFORMATION')); + $this->template->assign_vars(array( 'MESSAGE_TITLE' => $this->user->lang('INFORMATION'), 'MESSAGE_TEXT' => $message, @@ -83,7 +83,11 @@ class kernel_exception_subscriber implements EventSubscriberInterface else { $data = array(); - $data['message'] = $message; + + if (!empty($message)) + { + $data['message'] = $message; + } if (defined('DEBUG')) { diff --git a/phpBB/phpbb/exception/http_exception.php b/phpBB/phpbb/exception/http_exception.php index 57604d49d4..f733462a05 100644 --- a/phpBB/phpbb/exception/http_exception.php +++ b/phpBB/phpbb/exception/http_exception.php @@ -13,10 +13,12 @@ namespace phpbb\exception; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; + /** * Class http_exception */ -class http_exception extends exception implements exception_interface +class http_exception extends exception implements HttpExceptionInterface { /** * Http status code. @@ -47,7 +49,7 @@ class http_exception extends exception implements exception_interface $this->status_code = $status_code; $this->headers = $headers; - parent::__construct($message, $code, $previous); + parent::__construct($message, $parameters, $previous, $code); } /** diff --git a/tests/event/exception_listener.php b/tests/event/exception_listener.php new file mode 100644 index 0000000000..9d6b70e2d3 --- /dev/null +++ b/tests/event/exception_listener.php @@ -0,0 +1,100 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php'; + +class exception_listener extends phpbb_test_case +{ + public function phpbb_exception_data() + { + return array( + array( + true, + new \Exception(), + array( + 'status_code' => 500, + ), + ), + array( + true, + new \Exception('AJAX_ERROR_TEXT'), + array( + 'status_code' => 500, + 'content' => 'AJAX_ERROR_TEXT', + ), + ), + array( + true, + new \phpbb\exception\exception('AJAX_ERROR_TEXT'), + array( + 'status_code' => 500, + 'content' => 'Something went wrong when processing your request.', + ), + ), + array( + true, + new \Symfony\Component\HttpKernel\Exception\HttpException(404, 'AJAX_ERROR_TEXT'), + array( + 'status_code' => 404, + 'content' => 'AJAX_ERROR_TEXT', + ), + ), + array( + true, + new \phpbb\exception\http_exception(404, 'AJAX_ERROR_TEXT'), + array( + 'status_code' => 404, + 'content' => 'Something went wrong when processing your request.', + ), + ), + array( + true, + new \phpbb\exception\http_exception(404, 'CURRENT_TIME', array('today')), + array( + 'status_code' => 404, + 'content' => 'It is currently today', + ), + ), + ); + } + + /** + * @dataProvider phpbb_exception_data + */ + public function test_phpbb_exception($is_ajax, $exception, $expected) + { + $request = \Symfony\Component\HttpFoundation\Request::create('test.php', 'GET', array(), array(), array(), $is_ajax ? array('HTTP_X_REQUESTED_WITH' => 'XMLHttpRequest') : array()); + + $template = $this->getMockBuilder('\phpbb\template\twig\twig') + ->disableOriginalConstructor() + ->getMock(); + + $user = new \phpbb\user('\phpbb\datetime'); + $user->add_lang('common'); + + $exception_listener = new \phpbb\event\kernel_exception_subscriber($template, $user); + + $event = new \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent(new \phpbb\kernel('test'), $request, \Symfony\Component\HttpKernel\HttpKernelInterface::MASTER_REQUEST, $exception); + $exception_listener->on_kernel_exception($event); + + $response = $event->getResponse(); + + $this->assertEquals($expected['status_code'], $response->getStatusCode()); + $this->assertEquals($is_ajax, $response instanceof \Symfony\Component\HttpFoundation\JsonResponse); + + if (isset($expected['content'])) + { + $this->assertContains($expected['content'], $response->getContent()); + } + } +} From 8ef238ea73b481a717d8b68fb1fd7d1b8799d7f2 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Wed, 14 Jan 2015 11:13:28 +0100 Subject: [PATCH 4/5] [ticket/13361] Fix the JsonResponse in the exception listener PHPBB3-13361 --- phpBB/phpbb/event/kernel_exception_subscriber.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/phpbb/event/kernel_exception_subscriber.php b/phpBB/phpbb/event/kernel_exception_subscriber.php index 01940a4fa9..eb7831ad34 100644 --- a/phpBB/phpbb/event/kernel_exception_subscriber.php +++ b/phpBB/phpbb/event/kernel_exception_subscriber.php @@ -94,7 +94,7 @@ class kernel_exception_subscriber implements EventSubscriberInterface $data['trace'] = $exception->getTrace(); } - $response = new JsonResponse($message, 500); + $response = new JsonResponse($data, 500); } if ($exception instanceof HttpExceptionInterface) From 7fef91afd28290ce268f671eb525dedf5ade944d Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Tue, 20 Jan 2015 23:08:30 +0100 Subject: [PATCH 5/5] [ticket/13361] Rename exception to runtime_exception PHPBB3-13361 --- phpBB/phpbb/exception/http_exception.php | 2 +- .../exception/{exception.php => runtime_exception.php} | 6 +++--- .../{exception_listener.php => exception_listener_test.php} | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) rename phpBB/phpbb/exception/{exception.php => runtime_exception.php} (82%) rename tests/event/{exception_listener.php => exception_listener_test.php} (91%) diff --git a/phpBB/phpbb/exception/http_exception.php b/phpBB/phpbb/exception/http_exception.php index f733462a05..0e6ffe4f59 100644 --- a/phpBB/phpbb/exception/http_exception.php +++ b/phpBB/phpbb/exception/http_exception.php @@ -18,7 +18,7 @@ use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; /** * Class http_exception */ -class http_exception extends exception implements HttpExceptionInterface +class http_exception extends runtime_exception implements HttpExceptionInterface { /** * Http status code. diff --git a/phpBB/phpbb/exception/exception.php b/phpBB/phpbb/exception/runtime_exception.php similarity index 82% rename from phpBB/phpbb/exception/exception.php rename to phpBB/phpbb/exception/runtime_exception.php index 077d0d258a..6568bbf86f 100644 --- a/phpBB/phpbb/exception/exception.php +++ b/phpBB/phpbb/exception/runtime_exception.php @@ -14,11 +14,11 @@ namespace phpbb\exception; /** - * Class exception + * Class runtime_exception * * Define an exception which support a language var as message. */ -class exception extends \RuntimeException implements exception_interface +class runtime_exception extends \RuntimeException implements exception_interface { /** * Parameters to use with the language var. @@ -32,7 +32,7 @@ class exception extends \RuntimeException implements exception_interface * * @param string $message The Exception message to throw (must be a language variable). * @param array $parameters The parameters to use with the language var. - * @param \Exception $previous The previous exception used for the exception chaining. + * @param \Exception $previous The previous runtime_exception used for the runtime_exception chaining. * @param integer $code The Exception code. */ public function __construct($message = "", array $parameters = array(), \Exception $previous = null, $code = 0) diff --git a/tests/event/exception_listener.php b/tests/event/exception_listener_test.php similarity index 91% rename from tests/event/exception_listener.php rename to tests/event/exception_listener_test.php index 9d6b70e2d3..4d3453cd83 100644 --- a/tests/event/exception_listener.php +++ b/tests/event/exception_listener_test.php @@ -35,7 +35,7 @@ class exception_listener extends phpbb_test_case ), array( true, - new \phpbb\exception\exception('AJAX_ERROR_TEXT'), + new \phpbb\exception\runtime_exception('AJAX_ERROR_TEXT'), array( 'status_code' => 500, 'content' => 'Something went wrong when processing your request.', @@ -84,7 +84,7 @@ class exception_listener extends phpbb_test_case $exception_listener = new \phpbb\event\kernel_exception_subscriber($template, $user); - $event = new \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent(new \phpbb\kernel('test'), $request, \Symfony\Component\HttpKernel\HttpKernelInterface::MASTER_REQUEST, $exception); + $event = new \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent($this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'), $request, \Symfony\Component\HttpKernel\HttpKernelInterface::MASTER_REQUEST, $exception); $exception_listener->on_kernel_exception($event); $response = $event->getResponse();