diff --git a/includes/Rest/EntryPoint.php b/includes/Rest/EntryPoint.php index 97f5d39ff6a..145282e84d2 100644 --- a/includes/Rest/EntryPoint.php +++ b/includes/Rest/EntryPoint.php @@ -9,6 +9,7 @@ use MediaWiki\Config\ServiceOptions; use MediaWiki\MediaWikiServices; use MediaWiki\Rest\BasicAccess\CompoundAuthorizer; use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer; +use MediaWiki\Rest\Reporter\MWErrorReporter; use MediaWiki\Rest\Validator\Validator; use RequestContext; use Title; @@ -75,6 +76,7 @@ class EntryPoint { $authority, $objectFactory, $restValidator, + new MWErrorReporter(), $services->getHookContainer() ) )->setCors( $cors ); } diff --git a/includes/Rest/Reporter/ErrorReporter.php b/includes/Rest/Reporter/ErrorReporter.php new file mode 100644 index 00000000000..55cf1fa4c0d --- /dev/null +++ b/includes/Rest/Reporter/ErrorReporter.php @@ -0,0 +1,24 @@ +level = $level; + } + + /** + * @param Throwable $error + * @param Handler $handler + * @param RequestInterface $request + */ + public function reportError( Throwable $error, Handler $handler, RequestInterface $request ) { + $firstLine = preg_split( '#$#m', (string)$error, 0 )[0]; + trigger_error( $firstLine, $this->level ); + } + +} diff --git a/includes/Rest/ResponseFactory.php b/includes/Rest/ResponseFactory.php index 22baa54bb46..7658eb68660 100644 --- a/includes/Rest/ResponseFactory.php +++ b/includes/Rest/ResponseFactory.php @@ -231,11 +231,12 @@ class ResponseFactory { } } else { $response = $this->createHttpError( 500, [ - 'message' => 'Error: exception of type ' . get_class( $exception ), + 'message' => 'Error: exception of type ' . get_class( $exception ) . ': ' + . $exception->getMessage(), 'exception' => MWExceptionHandler::getStructuredExceptionData( $exception ) ] ); - // FIXME should we try to do something useful with ILocalizedException? - // FIXME should we try to do something useful with common MediaWiki errors like ReadOnlyError? + // XXX: should we try to do something useful with ILocalizedException? + // XXX: should we try to do something useful with common MediaWiki errors like ReadOnlyError? } return $response; } diff --git a/includes/Rest/Router.php b/includes/Rest/Router.php index fae9f50e694..a89fdc35e04 100644 --- a/includes/Rest/Router.php +++ b/includes/Rest/Router.php @@ -8,7 +8,9 @@ use MediaWiki\HookContainer\HookContainer; use MediaWiki\Permissions\Authority; use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface; use MediaWiki\Rest\PathTemplateMatcher\PathMatcher; +use MediaWiki\Rest\Reporter\ErrorReporter; use MediaWiki\Rest\Validator\Validator; +use Throwable; use Wikimedia\Message\MessageValue; use Wikimedia\ObjectFactory; @@ -63,6 +65,9 @@ class Router { /** @var CorsUtils|null */ private $cors; + /** @var ErrorReporter */ + private $errorReporter; + /** @var HookContainer */ private $hookContainer; @@ -77,6 +82,7 @@ class Router { * @param Authority $authority * @param ObjectFactory $objectFactory * @param Validator $restValidator + * @param ErrorReporter $errorReporter * @param HookContainer $hookContainer * @internal */ @@ -91,6 +97,7 @@ class Router { Authority $authority, ObjectFactory $objectFactory, Validator $restValidator, + ErrorReporter $errorReporter, HookContainer $hookContainer ) { $this->routeFiles = $routeFiles; @@ -103,6 +110,7 @@ class Router { $this->authority = $authority; $this->objectFactory = $objectFactory; $this->restValidator = $restValidator; + $this->errorReporter = $errorReporter; $this->hookContainer = $hookContainer; } @@ -330,6 +338,9 @@ class Router { return $this->executeHandler( $handler ); } catch ( HttpException $e ) { return $this->responseFactory->createFromException( $e ); + } catch ( Throwable $e ) { + $this->errorReporter->reportError( $e, $handler, $request ); + return $this->responseFactory->createFromException( $e ); } } diff --git a/tests/phpunit/includes/Rest/EntryPointTest.php b/tests/phpunit/includes/Rest/EntryPointTest.php index a8f91207f93..ca199d16cc9 100644 --- a/tests/phpunit/includes/Rest/EntryPointTest.php +++ b/tests/phpunit/includes/Rest/EntryPointTest.php @@ -10,6 +10,7 @@ use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer; use MediaWiki\Rest\CorsUtils; use MediaWiki\Rest\EntryPoint; use MediaWiki\Rest\Handler; +use MediaWiki\Rest\Reporter\PHPErrorReporter; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\RequestInterface; use MediaWiki\Rest\ResponseFactory; @@ -47,6 +48,7 @@ class EntryPointTest extends \MediaWikiIntegrationTestCase { $authority, $objectFactory, new Validator( $objectFactory, $request, $authority ), + new PHPErrorReporter(), $this->createHookContainer() ); } diff --git a/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php b/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php index 3e38b2601ea..ec615a97419 100644 --- a/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php +++ b/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php @@ -6,6 +6,7 @@ use GuzzleHttp\Psr7\Uri; use MediaWiki\Permissions\SimpleAuthority; use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer; use MediaWiki\Rest\Handler; +use MediaWiki\Rest\Reporter\PHPErrorReporter; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\ResponseFactory; use MediaWiki\Rest\Router; @@ -41,6 +42,7 @@ class MWBasicRequestAuthorizerTest extends MediaWikiUnitTestCase { $authority, $objectFactory, new Validator( $objectFactory, $request, $authority ), + new PHPErrorReporter(), $this->createHookContainer() ); } diff --git a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php index bc84b9510a1..18baae57583 100644 --- a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php +++ b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php @@ -162,7 +162,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { $body->rewind(); $data = json_decode( $body->getContents(), true ); $this->assertSame( 500, $data['httpCode'] ); - $this->assertSame( 'Error: exception of type Exception', $data['message'] ); + $this->assertSame( 'Error: exception of type Exception: hello', $data['message'] ); } public function testCreateFromExceptionWrapped() { diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php index 2c26ff0b1bb..5c4f08b7a75 100644 --- a/tests/phpunit/unit/includes/Rest/RouterTest.php +++ b/tests/phpunit/unit/includes/Rest/RouterTest.php @@ -7,6 +7,7 @@ use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer; use MediaWiki\Rest\Handler; use MediaWiki\Rest\HttpException; use MediaWiki\Rest\RedirectException; +use MediaWiki\Rest\Reporter\ErrorReporter; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\RequestInterface; use MediaWiki\Rest\ResponseException; @@ -14,7 +15,10 @@ use MediaWiki\Rest\ResponseFactory; use MediaWiki\Rest\Router; use MediaWiki\Rest\Validator\Validator; use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Container\ContainerInterface; +use RuntimeException; +use Throwable; use Wikimedia\ObjectFactory; /** @@ -23,6 +27,9 @@ use Wikimedia\ObjectFactory; class RouterTest extends \MediaWikiUnitTestCase { use MockAuthorityTrait; + /** @var Throwable[] */ + private $reportedErrors = []; + /** * @param RequestInterface $request * @param string|null $authError @@ -39,6 +46,14 @@ class RouterTest extends \MediaWikiUnitTestCase { ); $routeFiles = array_merge( [ __DIR__ . '/testRoutes.json' ], $additionalRouteFiles ); $authority = $this->mockAnonUltimateAuthority(); + + /** @var MockObject|ErrorReporter $mockErrorReporter */ + $mockErrorReporter = $this->createNoOpMock( ErrorReporter::class, [ 'reportError' ] ); + $mockErrorReporter->method( 'reportError' ) + ->willReturnCallback( function ( $e ) { + $this->reportedErrors[] = $e; + } ); + return new Router( $routeFiles, [], @@ -50,6 +65,7 @@ class RouterTest extends \MediaWikiUnitTestCase { $authority, $objectFactory, new Validator( $objectFactory, $request, $authority ), + $mockErrorReporter, $this->createHookContainer() ); } @@ -99,6 +115,14 @@ class RouterTest extends \MediaWikiUnitTestCase { }; } + public static function fatalHandlerFactory() { + return new class extends Handler { + public function execute() { + throw new RuntimeException( 'Fatal mock error', 12345 ); + } + }; + } + public static function throwRedirectHandlerFactory() { return new class extends Handler { public function execute() { @@ -117,7 +141,7 @@ class RouterTest extends \MediaWikiUnitTestCase { }; } - public function testException() { + public function testHttpException() { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throw' ) ] ); $router = $this->createRouter( $request ); $response = $router->execute( $request ); @@ -128,6 +152,23 @@ class RouterTest extends \MediaWikiUnitTestCase { $this->assertSame( 'Mock error', $data['message'] ); } + public function testFatalException() { + $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/fatal' ) ] ); + $router = $this->createRouter( $request ); + $response = $router->execute( $request ); + $this->assertSame( 500, $response->getStatusCode() ); + $body = $response->getBody(); + $body->rewind(); + $data = json_decode( $body->getContents(), true ); + $this->assertStringContainsString( 'RuntimeException', $data['message'] ); + $this->assertStringContainsString( 'Fatal mock error', $data['message'] ); + $this->assertSame( RuntimeException::class, $data['exception']['type'] ); + $this->assertSame( 12345, $data['exception']['code'] ); + $this->assertSame( 'Fatal mock error', $data['exception']['message'] ); + $this->assertNotEmpty( $this->reportedErrors ); + $this->assertInstanceOf( RuntimeException::class, $this->reportedErrors[0] ); + } + public function testRedirectException() { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throwRedirect' ) ] ); $router = $this->createRouter( $request ); diff --git a/tests/phpunit/unit/includes/Rest/testRoutes.json b/tests/phpunit/unit/includes/Rest/testRoutes.json index 767d204833b..5a704a4ccc7 100644 --- a/tests/phpunit/unit/includes/Rest/testRoutes.json +++ b/tests/phpunit/unit/includes/Rest/testRoutes.json @@ -15,6 +15,10 @@ "path": "/mock/RouterTest/throw", "factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwHandlerFactory" }, + { + "path": "/mock/RouterTest/fatal", + "factory": "MediaWiki\\Tests\\Rest\\RouterTest::fatalHandlerFactory" + }, { "path": "/mock/RouterTest/throwRedirect", "factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwRedirectHandlerFactory"