diff --git a/src/DispatchContext.php b/src/DispatchContext.php index b97a2f5..4b5931b 100644 --- a/src/DispatchContext.php +++ b/src/DispatchContext.php @@ -18,6 +18,8 @@ use function in_array; use function is_a; +use function preg_quote; +use function preg_replace; use function rawurldecode; use function rtrim; use function set_error_handler; @@ -29,14 +31,12 @@ final class DispatchContext implements ContainerInterface { /** @var array */ - public array $params = []; + public private(set) array $params = []; - public AbstractRoute|null $route = null; + public private(set) AbstractRoute|null $route = null; /** @var array Headers to apply only when the response does not already have them */ - public array $defaultResponseHeaders = []; - - private RoutinePipeline|null $routinePipeline = null; + public private(set) array $defaultResponseHeaders = []; private Responder|null $responder = null; @@ -49,21 +49,32 @@ final class DispatchContext implements ContainerInterface private bool $hasStatusOverride = false; - private string $effectiveMethod = ''; - - private string $effectivePath = ''; + private string $effectiveMethod; - /** @var array */ - private array $handlers = []; + private string $effectivePath; private Resolver|null $resolver = null; + /** @param array $handlers */ public function __construct( - public ServerRequestInterface $request, - public ResponseFactoryInterface&StreamFactoryInterface $factory, + public private(set) ServerRequestInterface $request, + public private(set) ResponseFactoryInterface&StreamFactoryInterface $factory, + private RoutinePipeline $routinePipeline = new RoutinePipeline(), + private array $handlers = [], + string $basePath = '', ) { - $this->effectivePath = rtrim(rawurldecode($request->getUri()->getPath()), ' /'); + $path = rtrim(rawurldecode($request->getUri()->getPath()), ' /'); + if ($basePath !== '') { + $path = preg_replace( + '#^' . preg_quote($basePath, '#') . '#', + '', + $path, + ) ?? $path; + } + + $this->effectivePath = $path; $this->effectiveMethod = strtoupper($request->getMethod()); + $this->resetHandlerState(); } public function method(): string @@ -76,11 +87,6 @@ public function path(): string return $this->effectivePath; } - public function setPath(string $path): void - { - $this->effectivePath = $path; - } - public function hasPreparedResponse(): bool { return $this->hasPreparedResponse; @@ -125,6 +131,13 @@ public function prepareResponse(int $status, array $headers = []): void } } + /** @param array $params */ + public function configureRoute(AbstractRoute $route, array $params = []): void + { + $this->route = $route; + $this->params = $params; + } + /** Generates the PSR-7 response from the current route */ public function response(): ResponseInterface|null { @@ -146,7 +159,7 @@ public function response(): ResponseInterface|null $previousErrorHandler = $isHandler ? null : $this->installErrorHandler(); try { - $preRoutineResult = $this->routinePipeline()->processBy($this, $route); + $preRoutineResult = $this->routinePipeline->processBy($this, $route); if ($preRoutineResult instanceof AbstractRoute) { return $this->forward($preRoutineResult); @@ -166,7 +179,7 @@ public function response(): ResponseInterface|null return $this->forward($rawResult); } - $processedResult = $this->routinePipeline()->processThrough($this, $route, $rawResult); + $processedResult = $this->routinePipeline->processThrough($this, $route, $rawResult); if (!$isHandler) { $errorResponse = $this->forwardCollectedErrors(); @@ -192,27 +205,21 @@ public function response(): ResponseInterface|null } } - public function forward(AbstractRoute $route): ResponseInterface|null + public function withRequest(ServerRequestInterface $request): void { - $this->route = $route; - - return $this->response(); + $this->request = $request; } - public function setRoutinePipeline(RoutinePipeline $routinePipeline): void + public function setResponder(Responder $responder): void { - $this->routinePipeline = $routinePipeline; + $this->responder = $responder; } - /** @param array $handlers */ - public function setHandlers(array $handlers): void + public function forward(AbstractRoute $route): ResponseInterface|null { - $this->handlers = $handlers; - } + $this->route = $route; - public function setResponder(Responder $responder): void - { - $this->responder = $responder; + return $this->response(); } public function resolver(): Resolver @@ -239,7 +246,26 @@ public function get(string $id): mixed throw new NotFoundException(sprintf('No entry found for "%s"', $id)); } - /** @return callable|null The previous error handler, or null if no ErrorHandler is registered */ + private function resetHandlerState(): void + { + foreach ($this->handlers as $handler) { + if ($handler instanceof ErrorHandler) { + $handler->errors = []; + } elseif ($handler instanceof ExceptionHandler) { + $handler->exception = null; + } + } + } + + /** + * Safe only when requests are guaranteed not to overlap within the same PHP process + * (for example: PHP-FPM, Swoole workers, FrankenPHP workers, or ReactPHP when + * request handling/dispatch is strictly serialized). Not safe for coroutine- or + * event-loop-concurrent request handling within a single PHP process, since + * set_error_handler is global state. + * + * @return callable|null The previous error handler, or null if no ErrorHandler is registered + */ private function installErrorHandler(): callable|null { foreach ($this->handlers as $handler) { @@ -303,7 +329,7 @@ private function forwardToStatusRoute(ResponseInterface $preparedResponse): Resp // Run routine negotiation (e.g. Accept) before forwarding, // since the normal route-selection phase was skipped - $this->routinePipeline()->matches($this, $handler, $this->params); + $this->routinePipeline->matches($this, $handler, $this->params); $result = $this->forward($handler); @@ -327,27 +353,14 @@ private function finalizeResponse(mixed $response): ResponseInterface ); } - private function routinePipeline(): RoutinePipeline - { - return $this->routinePipeline ??= new RoutinePipeline(); - } - private function responder(): Responder { - if ($this->responder !== null) { - return $this->responder; - } - - return $this->responder = new Responder($this->factory); + return $this->responder ??= new Responder($this->factory); } private function ensureResponseDraft(): ResponseInterface { - if ($this->responseDraft !== null) { - return $this->responseDraft; - } - - return $this->responseDraft = $this->factory->createResponse(); + return $this->responseDraft ??= $this->factory->createResponse(); } public function __toString(): string diff --git a/src/DispatchEngine.php b/src/DispatchEngine.php index bedb724..cd58491 100644 --- a/src/DispatchEngine.php +++ b/src/DispatchEngine.php @@ -4,7 +4,6 @@ namespace Respect\Rest; -use Closure; use Psr\Http\Message\ResponseFactoryInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -19,28 +18,32 @@ use function count; use function implode; use function iterator_to_array; -use function preg_quote; -use function preg_replace; use function stripos; final class DispatchEngine implements RequestHandlerInterface { private RoutinePipeline $routinePipeline; - /** @param (Closure(DispatchContext): void)|null $onContextReady */ public function __construct( private RouteProvider $routeProvider, private ResponseFactoryInterface&StreamFactoryInterface $factory, - private Closure|null $onContextReady = null, ) { $this->routinePipeline = new RoutinePipeline(); } + public function routinePipeline(): RoutinePipeline + { + return $this->routinePipeline; + } + public function dispatch(ServerRequestInterface $serverRequest): DispatchContext { $context = new DispatchContext( $serverRequest, $this->factory, + $this->routinePipeline, + $this->routeProvider->getHandlers(), + $this->routeProvider->getBasePath(), ); return $this->dispatchContext($context); @@ -55,13 +58,6 @@ public function handle(ServerRequestInterface $request): ResponseInterface public function dispatchContext(DispatchContext $context): DispatchContext { - if ($this->onContextReady !== null) { - ($this->onContextReady)($context); - } - - $context->setRoutinePipeline($this->routinePipeline); - $context->setHandlers($this->routeProvider->getHandlers()); - if (!$this->isRoutelessDispatch($context) && $context->route === null) { $this->routeDispatch($context); } @@ -120,8 +116,6 @@ private function isRoutelessDispatch(DispatchContext $context): bool private function routeDispatch(DispatchContext $context): void { - $this->applyBasePath($context); - $matchedByPath = $this->getMatchedRoutesByPath($context); /** @var array $matchedArray */ $matchedArray = iterator_to_array($matchedByPath); @@ -140,30 +134,13 @@ private function routeDispatch(DispatchContext $context): void } } - private function applyBasePath(DispatchContext $context): void - { - $basePath = $this->routeProvider->getBasePath(); - if ($basePath === '') { - return; - } - - $context->setPath( - preg_replace( - '#^' . preg_quote($basePath) . '#', - '', - $context->path(), - ) ?? $context->path(), - ); - } - /** @param array $params */ private function configureContext( DispatchContext $context, AbstractRoute $route, array $params = [], ): DispatchContext { - $context->route = $route; - $context->params = $params; + $context->configureRoute($route, $params); return $context; } @@ -176,7 +153,7 @@ private function getMatchedRoutesByPath(DispatchContext $context): SplObjectStor foreach ($this->routeProvider->getRoutes() as $route) { $params = []; - if (!$this->matchRoute($context, $route, $params)) { + if (!$route->match($context, $params)) { continue; } @@ -265,21 +242,6 @@ private function hasExplicitOptionsRoute(SplObjectStorage $matchedByPath): bool return false; } - /** @param array $params */ - private function matchRoute( - DispatchContext $context, - AbstractRoute $route, - array &$params = [], - ): bool { - if (!$route->match($context, $params)) { - return false; - } - - $context->route = $route; - - return true; - } - /** @param SplObjectStorage> $matchedByPath */ private function routineMatch( DispatchContext $context, @@ -296,7 +258,6 @@ private function routineMatch( /** @var array $tempParams */ $tempParams = $matchedByPath[$route]; $context->clearResponseMeta(); - $context->route = $route; if ($this->routinePipeline->matches($context, $route, $tempParams)) { return $this->configureContext( $context, diff --git a/src/Router.php b/src/Router.php index b3ae4c2..deb2b23 100644 --- a/src/Router.php +++ b/src/Router.php @@ -15,7 +15,6 @@ use Respect\Fluent\Resolvers\Ucfirst; use Respect\Rest\Routes\AbstractRoute; use Respect\Rest\Routines\Routinable; -use Throwable; use function array_pop; use function assert; @@ -28,11 +27,8 @@ use function preg_match; use function rtrim; use function substr_count; -use function trigger_error; use function usort; -use const E_USER_ERROR; - /** * A router that contains many instances of routes. * @@ -47,8 +43,6 @@ */ final class Router implements MiddlewareInterface, RequestHandlerInterface, RouteProvider { - public DispatchContext|null $context = null; - /** @var array */ protected array $globalRoutines = []; @@ -178,6 +172,9 @@ public function createDispatchContext(ServerRequestInterface $serverRequest): Di return new DispatchContext( $serverRequest, $this->factory, + $this->dispatchEngine()->routinePipeline(), + $this->handlers, + $this->basePath, ); } @@ -256,9 +253,6 @@ public function dispatchEngine(): DispatchEngine return $this->dispatchEngine ??= new DispatchEngine( $this, $this->factory, - function (DispatchContext $ctx): void { - $this->context = $ctx; - }, ); } @@ -300,21 +294,6 @@ static function (AbstractRoute $a, AbstractRoute $b): int { ); } - public function __toString(): string - { - $string = ''; - try { - $response = $this->context?->response(); - if ($response !== null) { - $string = (string) $response->getBody(); - } - } catch (Throwable $exception) { - trigger_error($exception->getMessage(), E_USER_ERROR); - } - - return $string; - } - /** @param array $args */ public function __call(string $method, array $args): AbstractRoute { diff --git a/src/Routes/AbstractRoute.php b/src/Routes/AbstractRoute.php index 8c8fe47..7860682 100644 --- a/src/Routes/AbstractRoute.php +++ b/src/Routes/AbstractRoute.php @@ -193,10 +193,10 @@ public function match(DispatchContext $context, array &$params = []): bool } if ($suffix !== '') { - $context->request = $context->request->withAttribute( + $context->withRequest($context->request->withAttribute( 'respect.ext.remaining', $suffix, - ); + )); } } diff --git a/src/Routines/AbstractAccept.php b/src/Routines/AbstractAccept.php index 528880e..1b612c9 100644 --- a/src/Routines/AbstractAccept.php +++ b/src/Routines/AbstractAccept.php @@ -107,7 +107,7 @@ protected function notifyDeclined( DispatchContext $context, array $params, ): void { - $this->forgetNegotiatedCallback(); + $this->forgetNegotiatedCallback($context); $context->prepareResponse(406); } diff --git a/src/Routines/AbstractCallbackMediator.php b/src/Routines/AbstractCallbackMediator.php index 495e775..821de7f 100644 --- a/src/Routines/AbstractCallbackMediator.php +++ b/src/Routines/AbstractCallbackMediator.php @@ -5,7 +5,9 @@ namespace Respect\Rest\Routines; use Respect\Rest\DispatchContext; -use SplObjectStorage; + +use function is_callable; +use function spl_object_id; /** * Mediates the callback selection process when choosing the appropriate @@ -15,9 +17,6 @@ // phpcs:ignore SlevomatCodingStandard.Classes.SuperfluousAbstractClassNaming.SuperfluousPrefix abstract class AbstractCallbackMediator extends CallbackList implements ProxyableWhen { - /** @var SplObjectStorage|false|null */ - protected SplObjectStorage|false|null $negotiated = null; - /** @param array $params */ public function when(DispatchContext $context, array $params): mixed { @@ -76,27 +75,25 @@ abstract protected function notifyDeclined( protected function getNegotiatedCallback(DispatchContext $context): callable|null { - if (!$this->negotiated instanceof SplObjectStorage || !$this->negotiated->offsetExists($context)) { - return null; - } + $value = $context->request->getAttribute($this->negotiatedAttributeKey()); - return $this->negotiated[$context]; + return is_callable($value) ? $value : null; } protected function rememberNegotiatedCallback(DispatchContext $context, callable $callback): void { - if (!$this->negotiated instanceof SplObjectStorage) { - /** @var SplObjectStorage $storage */ - $storage = new SplObjectStorage(); - $this->negotiated = $storage; - } - - $this->negotiated[$context] = $callback; + $context->withRequest($context->request->withAttribute( + $this->negotiatedAttributeKey(), + $callback, + )); } - protected function forgetNegotiatedCallback(): void + protected function forgetNegotiatedCallback(DispatchContext $context): void { - $this->negotiated = false; + $context->withRequest($context->request->withAttribute( + $this->negotiatedAttributeKey(), + false, + )); } protected function authorize(string $requested, string $provided): mixed @@ -104,6 +101,11 @@ protected function authorize(string $requested, string $provided): mixed return $requested == $provided; } + private function negotiatedAttributeKey(): string + { + return 'rest.negotiated.' . spl_object_id($this); + } + /** @param array $params */ private function mediate(string &$requested, string &$provided, DispatchContext $context, array $params): bool { diff --git a/src/Routines/ContentType.php b/src/Routines/ContentType.php index c2fc636..602efea 100644 --- a/src/Routines/ContentType.php +++ b/src/Routines/ContentType.php @@ -33,7 +33,7 @@ public function by(DispatchContext $context, array $params): mixed $psrRequest = $psrRequest->withParsedBody($payload); } - $context->request = $psrRequest; + $context->withRequest($psrRequest); return null; } @@ -83,7 +83,7 @@ protected function notifyDeclined( DispatchContext $context, array $params, ): void { - $this->forgetNegotiatedCallback(); + $this->forgetNegotiatedCallback($context); $context->prepareResponse(415); } diff --git a/src/Routines/FileExtension.php b/src/Routines/FileExtension.php index ea720b4..d1bbbf6 100644 --- a/src/Routines/FileExtension.php +++ b/src/Routines/FileExtension.php @@ -5,8 +5,9 @@ namespace Respect\Rest\Routines; use Respect\Rest\DispatchContext; -use SplObjectStorage; +use function is_callable; +use function spl_object_id; use function str_ends_with; use function strlen; use function substr; @@ -19,9 +20,6 @@ final class FileExtension extends CallbackList implements { private const string REMAINING_ATTRIBUTE = 'respect.ext.remaining'; - /** @var SplObjectStorage|null */ - private SplObjectStorage|null $negotiated = null; - /** @return array */ public function getExtensions(): array { @@ -46,11 +44,14 @@ public function by(DispatchContext $context, array $params): mixed } $remaining = substr($remaining, 0, -strlen($ext)); - $context->request = $context->request->withAttribute( + $context->withRequest($context->request->withAttribute( self::REMAINING_ATTRIBUTE, $remaining, - ); - $this->remember($context, $this->getCallback($ext)); + )); + $context->withRequest($context->request->withAttribute( + $this->negotiatedAttributeKey(), + $this->getCallback($ext), + )); return null; } @@ -61,21 +62,13 @@ public function by(DispatchContext $context, array $params): mixed /** @param array $params */ public function through(DispatchContext $context, array $params): mixed { - if (!$this->negotiated instanceof SplObjectStorage || !$this->negotiated->offsetExists($context)) { - return null; - } + $value = $context->request->getAttribute($this->negotiatedAttributeKey()); - return $this->negotiated[$context]; + return is_callable($value) ? $value : null; } - private function remember(DispatchContext $context, callable $callback): void + private function negotiatedAttributeKey(): string { - if (!$this->negotiated instanceof SplObjectStorage) { - /** @var SplObjectStorage $storage */ - $storage = new SplObjectStorage(); - $this->negotiated = $storage; - } - - $this->negotiated[$context] = $callback; + return 'rest.fileext.' . spl_object_id($this); } } diff --git a/src/Routines/UserAgent.php b/src/Routines/UserAgent.php index f220de2..0cb0abe 100644 --- a/src/Routines/UserAgent.php +++ b/src/Routines/UserAgent.php @@ -38,10 +38,10 @@ public function by(DispatchContext $context, array $params): mixed return $result; } - $context->request = $context->request->withAttribute( + $context->withRequest($context->request->withAttribute( self::THROUGH_ATTRIBUTE, $this->normalizeThroughResult($result), - ); + )); return null; } @@ -95,7 +95,7 @@ protected function notifyDeclined( DispatchContext $context, array $params, ): void { - $this->forgetNegotiatedCallback(); + $this->forgetNegotiatedCallback($context); } protected function authorize(string $requested, string $provided): mixed diff --git a/tests/DispatchContextTest.php b/tests/DispatchContextTest.php index ac81132..258000d 100644 --- a/tests/DispatchContextTest.php +++ b/tests/DispatchContextTest.php @@ -136,11 +136,11 @@ public function testResponseIsNullWithoutSettingARoute(DispatchContext $context) #[Depends('testIsPossibleToConstructUsingValuesFromSuperglobals')] public function testContextIsAbleToDeliverAResponseWithoutSettingPathParams(DispatchContext $context): void { - $context->route = $this->getMockForRoute( + $context->configureRoute($this->getMockForRoute( 'GET', '/notebooks', ['Vaio', 'MacBook', 'ThinkPad'], - ); + )); $response = $context->response(); self::assertNotNull($response, 'Response should not be null'); @@ -155,14 +155,13 @@ public function testContextIsAbleToDeliverAResponseWithoutSettingPathParams(Disp #[Depends('testIsPossibleToConstructUsingValuesFromSuperglobals')] public function testContextIsAbleToDeliverAResponseUsingPreviouslySetPathParams(DispatchContext $context): void { - $context->route = $this->getMockForRoute( + $context->configureRoute($this->getMockForRoute( 'GET', '/printers', 'Some Printers Response', 'GET', ['dpi', 'price'], - ); - $context->params = ['dpi', 'price']; + ), ['dpi', 'price']); $response = $context->response(); self::assertNotNull($response); @@ -179,7 +178,7 @@ public function testForwardReplacesRouteAndReturnsResponse(): void $context = $this->newContext(new ServerRequest('GET', '/users/alganet/lists')); $inactiveRoute = $this->getMockForRoute('GET', '/users/alganet/lists'); $forwardedRoute = $this->getMockForRoute('GET', '/lists/12345', 'Some list items'); - $context->route = $inactiveRoute; + $context->configureRoute($inactiveRoute); $context->forward($forwardedRoute); self::assertNotSame( @@ -237,7 +236,7 @@ static function () use ($internallyForwardedRoute) { } $context = $this->newContext(new ServerRequest('GET', '/cupcakes')); - $context->route = $userImplementedRoute; + $context->configureRoute($userImplementedRoute); $response = $context->response(); self::assertNotNull($response); @@ -263,7 +262,7 @@ public function testDeveloperCanAbortRequestReturningFalseOnByRoutine(): void ->method('by') ->willReturn(false); $route->appendRoutine($routine); - $context->route = $route; + $context->configureRoute($route); $response = $context->response(); @@ -300,7 +299,7 @@ public function testDeveloperCanReturnResponseInterfacesOnByRoutine(): void ->withBody($factory->createStream('blocked by routine')), ); $route->appendRoutine($routine); - $context->route = $route; + $context->configureRoute($route); $response = $context->response(); @@ -328,7 +327,7 @@ public function testDeveloperCanReturnCallablesToProcessOutputAfterTargetRuns(): return str_replace('-', ' ', $thatLogStubReturnedAbove); }); $route->appendRoutine($routine); - $context->route = $route; + $context->configureRoute($route); $response = $context->response(); self::assertNotNull($response); @@ -342,7 +341,7 @@ public function testDeveloperCanReturnCallablesToProcessOutputAfterTargetRuns(): public function testConvertingToStringCallsResponse(): void { $context = $this->newContext(new ServerRequest('GET', '/users/alganet/lists')); - $context->route = $this->getMockForRoute('GET', '/users/alganet/lists', 'Some list items'); + $context->configureRoute($this->getMockForRoute('GET', '/users/alganet/lists', 'Some list items')); $toString = (string) $context; self::assertSame('Some list items', $toString); @@ -369,7 +368,7 @@ public function testContextAcceptsAnInjectedResponder(): void $context = $this->newContext(new ServerRequest('GET', '/users/alganet/lists')); $factory = new Psr17Factory(); $context->setResponder(new Responder($factory)); - $context->route = $this->getMockForRoute('GET', '/users/alganet/lists', 'Some list items'); + $context->configureRoute($this->getMockForRoute('GET', '/users/alganet/lists', 'Some list items')); $response = $context->response(); @@ -380,13 +379,13 @@ public function testContextAcceptsAnInjectedResponder(): void public function test_exception_rethrown_when_no_exception_route_matches(): void { $context = $this->newContext(new ServerRequest('GET', '/')); - $context->route = $this->getMockForRoute( + $context->configureRoute($this->getMockForRoute( 'GET', '/', static function (): never { throw new RuntimeException('boom'); }, - ); + )); $this->expectException(RuntimeException::class); $this->expectExceptionMessage('boom'); diff --git a/tests/DispatchEngineTest.php b/tests/DispatchEngineTest.php index 9dd8362..4f3e5b2 100644 --- a/tests/DispatchEngineTest.php +++ b/tests/DispatchEngineTest.php @@ -149,28 +149,6 @@ public function testHandlePropagatesUnhandledExceptions(): void $engine->handle(new ServerRequest('GET', '/boom')); } - public function testOnContextReadyCallbackIsInvoked(): void - { - $captured = null; - $provider = $this->createStub(RouteProvider::class); - $provider->method('getRoutes')->willReturn([ - new StaticValue('GET', '/test', 'ok'), - ]); - $provider->method('getBasePath')->willReturn(''); - - $engine = new DispatchEngine( - $provider, - $this->factory, - static function ($context) use (&$captured): void { - $captured = $context; - }, - ); - - $context = $engine->dispatch(new ServerRequest('GET', '/test')); - - self::assertSame($context, $captured); - } - public function testGlobalOptions404WhenNoRoutes(): void { $engine = $this->engine([]); diff --git a/tests/RouterTest.php b/tests/RouterTest.php index 7fa1fcf..c3a3c78 100644 --- a/tests/RouterTest.php +++ b/tests/RouterTest.php @@ -41,7 +41,6 @@ use function json_encode; use function mb_convert_encoding; use function range; -use function spl_object_hash; use function str_repeat; use function str_split; use function stream_filter_append; @@ -545,12 +544,12 @@ public function testOptionsRequestShouldReturnBadRequestWhenExplicitOptionsRoute public function testOptionsHandlerShouldMaterializeRoutelessResponseWhenNoExplicitRouteSurvives(): void { $router = self::newRouter(); - $router->context = self::newContextForRouter($router, new ServerRequest('OPTIONS', '/asian')); + $context = self::newContextForRouter($router, new ServerRequest('OPTIONS', '/asian')); $handleOptionsRequest = new ReflectionMethod($router->dispatchEngine(), 'handleOptionsRequest'); - $handleOptionsRequest->invoke($router->dispatchEngine(), $router->context, ['OPTIONS'], new SplObjectStorage()); + $handleOptionsRequest->invoke($router->dispatchEngine(), $context, ['OPTIONS'], new SplObjectStorage()); - $response = $router->context->response(); + $response = $context->response(); self::assertNotNull($response); self::assertSame(204, $response->getStatusCode()); @@ -561,19 +560,24 @@ public function testOptionsHandlerShouldMaterializeRoutelessResponseWhenNoExplic // OldRouterTest unique methods // ========================================================================= - /** @covers \Respect\Rest\Router */ - public function testNotRoutableController(): void + /** + * A controller (stdClass) with no HTTP method handlers produces 500 + * because the route path matches but no method can be dispatched. + * + * @covers \Respect\Rest\Router + */ + public function testNotRoutableControllerReturns500(): void { - self::expectException('InvalidArgumentException'); $this->router->instanceRoute('ANY', '/', new stdClass()); - self::responseBody($this->router->dispatch(new ServerRequest('get', '/'))); + $response = $this->router->handle(new ServerRequest('GET', '/')); + self::assertSame(500, $response->getStatusCode()); } - public function testNotRoutableControllerByName(): void + public function testNotRoutableControllerByNameReturns500(): void { - self::expectException('InvalidArgumentException'); $this->router->classRoute('ANY', '/', '\\stdClass'); - self::responseBody($this->router->dispatch(new ServerRequest('get', '/'))); + $response = $this->router->handle(new ServerRequest('GET', '/')); + self::assertSame(500, $response->getStatusCode()); } #[DataProvider('providerForSingleRoutes')] @@ -2565,31 +2569,14 @@ public function test_when_should_be_called_only_on_existent_methods(): void self::assertEquals('"blub"', $out); } - public function test_request_should_be_available_from_router_after_dispatching(): void + public function test_dispatch_context_is_passed_through_to_route_callback(): void { $router = self::newRouter(); - $request = self::newContextForRouter($router, new ServerRequest('get', '/foo')); - $phpunit = $this; - $router->get('/foo', static function () use ($router, $request, $phpunit) { - $phpunit->assertSame($request, $router->context); - - return spl_object_hash($router->context); - }); - $response = $router->dispatchContext($request)->response(); + $context = self::newContextForRouter($router, new ServerRequest('get', '/foo')); + $router->get('/foo', static fn() => 'dispatched'); + $response = $router->dispatchContext($context)->response(); self::assertNotNull($response); - $out = (string) $response->getBody(); - self::assertEquals($out, spl_object_hash($request)); - } - - /** @covers Respect\Rest\Router::__toString */ - public function testToStringReturnsCurrentDispatchResponseBody(): void - { - $router = self::newRouter(); - $router->get('/foo', 'bar'); - - $router->dispatch(new ServerRequest('GET', '/foo')); - - self::assertSame('bar', (string) $router); + self::assertSame('dispatched', (string) $response->getBody()); } private static function responseBody(DispatchContext $request): string diff --git a/tests/Routes/ErrorTest.php b/tests/Routes/ErrorTest.php index 7787f73..89477e8 100644 --- a/tests/Routes/ErrorTest.php +++ b/tests/Routes/ErrorTest.php @@ -10,6 +10,7 @@ use PHPUnit\Framework\TestCase; use Respect\Rest\Router; +use function count; use function trigger_error; use const E_USER_WARNING; @@ -53,4 +54,25 @@ public function testMagicConstuctorCanCreateRoutesToErrors(): void 'An error should be caught by the router and forwarded', ); } + + #[RunInSeparateProcess] + public function testErrorStateDoesNotLeakBetweenDispatches(): void + { + $router = new Router('', new Psr17Factory()); + $router->onError(static fn(array $errors) => 'errors: ' . count($errors)); + $router->get('/warn', static function (): string { + trigger_error('warning1', E_USER_WARNING); + + return 'warned'; + }); + $router->get('/ok', static fn() => 'clean'); + + // First dispatch triggers an error + $resp1 = $router->handle(new ServerRequest('GET', '/warn')); + self::assertSame('errors: 1', (string) $resp1->getBody()); + + // Second dispatch should not inherit the first request's errors + $resp2 = $router->handle(new ServerRequest('GET', '/ok')); + self::assertSame('clean', (string) $resp2->getBody()); + } } diff --git a/tests/Routes/ExceptionTest.php b/tests/Routes/ExceptionTest.php index 83e2901..f856bdf 100644 --- a/tests/Routes/ExceptionTest.php +++ b/tests/Routes/ExceptionTest.php @@ -89,4 +89,22 @@ public function testExceptionRouteWorksViaHandle(): void self::assertSame(200, $response->getStatusCode()); self::assertSame('handled: boom', (string) $response->getBody()); } + + public function testExceptionStateDoesNotLeakBetweenDispatches(): void + { + $router = new Router('', new Psr17Factory()); + $router->onException('Throwable', static fn(Throwable $e) => 'error: ' . $e->getMessage()); + $router->get('/fail', static function (): void { + throw new RuntimeException('first'); + }); + $router->get('/ok', static fn() => 'success'); + + // First dispatch throws + $resp1 = $router->handle(new ServerRequest('GET', '/fail')); + self::assertSame('error: first', (string) $resp1->getBody()); + + // Second dispatch should not see the first exception + $resp2 = $router->handle(new ServerRequest('GET', '/ok')); + self::assertSame('success', (string) $resp2->getBody()); + } } diff --git a/tests/RoutinePipelineTest.php b/tests/RoutinePipelineTest.php index a5e334f..3c6159d 100644 --- a/tests/RoutinePipelineTest.php +++ b/tests/RoutinePipelineTest.php @@ -32,7 +32,7 @@ public function testMatchesReturnsTrueWithNoWhenRoutines(): void { $route = new Callback('GET', '/test', static fn(): string => 'ok'); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); $params = []; self::assertTrue($this->pipeline->matches($context, $route, $params)); @@ -43,7 +43,7 @@ public function testMatchesReturnsFalseWhenWhenRoutineReturnsFalse(): void $route = new Callback('GET', '/test', static fn(): string => 'ok'); $route->appendRoutine(new When(static fn(): bool => false)); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); $params = []; self::assertFalse($this->pipeline->matches($context, $route, $params)); @@ -54,7 +54,7 @@ public function testMatchesReturnsTrueWhenWhenRoutineReturnsTrue(): void $route = new Callback('GET', '/test', static fn(): string => 'ok'); $route->appendRoutine(new When(static fn(): bool => true)); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); $params = []; self::assertTrue($this->pipeline->matches($context, $route, $params)); @@ -64,7 +64,7 @@ public function testProcessByReturnsNullWithNoByRoutines(): void { $route = new Callback('GET', '/test', static fn(): string => 'ok'); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); self::assertNull($this->pipeline->processBy($context, $route)); } @@ -75,7 +75,7 @@ public function testProcessByReturnsResponseWhenByReturnsResponse(): void $response = $this->factory->createResponse(401); $route->appendRoutine(new By(static fn() => $response)); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); $result = $this->pipeline->processBy($context, $route); @@ -88,7 +88,7 @@ public function testProcessByReturnsFalseWhenByReturnsFalse(): void $route = new Callback('GET', '/test', static fn(): string => 'ok'); $route->appendRoutine(new By(static fn(): bool => false)); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); self::assertFalse($this->pipeline->processBy($context, $route)); } @@ -99,7 +99,7 @@ public function testProcessThroughChainsCallableResults(): void $route->appendRoutine(new Through(static fn() => static fn(string $v): string => $v . '-A')); $route->appendRoutine(new Through(static fn() => static fn(string $v): string => $v . '-B')); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); $result = $this->pipeline->processThrough($context, $route, 'start'); @@ -111,7 +111,7 @@ public function testProcessThroughSkipsNonCallableResults(): void $route = new Callback('GET', '/test', static fn(): string => 'ok'); $route->appendRoutine(new Through(static fn(): null => null)); $context = $this->newContext(); - $context->route = $route; + $context->configureRoute($route); $result = $this->pipeline->processThrough($context, $route, 'unchanged'); diff --git a/tests/Routines/AuthBasicTest.php b/tests/Routines/AuthBasicTest.php index e39147a..5c5031e 100644 --- a/tests/Routines/AuthBasicTest.php +++ b/tests/Routines/AuthBasicTest.php @@ -52,7 +52,7 @@ public function test_pass_all_params_to_callback(): void ->withHeader('Authorization', 'Basic ' . base64_encode($user . ':' . $pass)); $factory = new Psr17Factory(); $context = new DispatchContext($serverRequest, $factory); - $context->route = $this->createRouteWithResponseFactory(); + $context->configureRoute($this->createRouteWithResponseFactory()); $routine = new AuthBasic('auth realm', [$this, 'shunt_wantedParams']); $routine->by($context, [$param1, $param2]);