From 51e4784db5e090101a28e221552c0c6baf9de067 Mon Sep 17 00:00:00 2001 From: kodeart Date: Thu, 19 Oct 2023 00:04:27 +0200 Subject: [PATCH] Improvements - removes the previous error handling logic in route() method (wip) - moves the locale folder from src/ to project root - adds translation strings to error messages - adds error messages for en_US locale - do not load Woops if ENV var is 'PRODUCTION' - adds tests for DIModule - improved tests for error handlers - adds a test for route parameters - updates dependencies in composer - do not try to find the Config instance path - check if .env is readable/exists for configuration - uses `CurlyFormatter` for i18n messages (updates from [kodedphp/i18n](https://github.com/kodedphp/i18n/releases/tag/0.9.2)) - fixes the default translation.dir path (updates from [kodedphp/stdlib](https://github.com/kodedphp/stdlib/commit/331dabc3ed56c81e62010feceae56f2f7ef327ec)) --- .gitattributes | 1 + .gitignore | 5 ++- composer.json | 7 ++-- locale/en_US.php | 22 ++++++++++ src/App.php | 44 +++++++++----------- src/Middleware/AuthMiddleware.php | 12 ++---- src/Module.php | 12 +++--- src/Router.php | 30 +++++--------- src/locale/en_US.php | 7 ---- tests/AppTest.php | 21 ++++++++++ tests/DIModuleConfigurationTest.php | 47 +++++++++++++++++++++ tests/ErrorHandlersTest.php | 62 +++++++++++++++++++--------- tests/Fixtures/.env | 1 + tests/Fixtures/test-autoloader.php | 3 ++ tests/Fixtures/test-conf.php | 9 ++++ tests/HTTPErrorSerializationTest.php | 8 ++-- tests/NoAppRoutesTest.php | 9 ++-- tests/RouterBasicTest.php | 11 +++++ tests/RouterComplexTest.php | 7 ++-- tests/RouterTemplateTypesTest.php | 9 ++-- 20 files changed, 222 insertions(+), 105 deletions(-) create mode 100644 locale/en_US.php delete mode 100644 src/locale/en_US.php create mode 100644 tests/AppTest.php create mode 100644 tests/DIModuleConfigurationTest.php create mode 100644 tests/Fixtures/.env create mode 100644 tests/Fixtures/test-autoloader.php create mode 100644 tests/Fixtures/test-conf.php diff --git a/.gitattributes b/.gitattributes index 0183c7b..5dc882b 100644 --- a/.gitattributes +++ b/.gitattributes @@ -4,6 +4,7 @@ *.md diff=markdown .git* export-ignore +bench/ export-ignore docs/ export-ignore tests/ export-ignore *.dist export-ignore diff --git a/.gitignore b/.gitignore index dd9f443..27cc8e6 100644 --- a/.gitignore +++ b/.gitignore @@ -5,7 +5,8 @@ nbproject/ public/ site/ vendor/ -.DS_Store/ +tmp/ + .idea/ .fleet/ .vscode/ @@ -13,6 +14,8 @@ vendor/ .project/ .buildpath/ .directory/ + +.DS_Store Thumbs.db Desktop.ini ehthumbs.db diff --git a/composer.json b/composer.json index 0551d74..eb725c0 100644 --- a/composer.json +++ b/composer.json @@ -18,12 +18,11 @@ "php": "^8.1", "psr/http-server-middleware": "^1", "koded/container": "^3", - "koded/cache-simple": "^3", + "koded/cache-simple": "^3.1", "koded/http": "4.*", - "koded/i18n": "^0.9", - "koded/logging": "^3.1", + "koded/i18n": "^0.9.2", + "koded/logging": "^3.3", "koded/session": "2.*", - "koded/stdlib": "6.2.1 as 5.1.1", "filp/whoops": "^2.15", "ext-json": "*", "ext-mbstring": "*" diff --git a/locale/en_US.php b/locale/en_US.php new file mode 100644 index 0000000..cc646b9 --- /dev/null +++ b/locale/en_US.php @@ -0,0 +1,22 @@ + 'English', + 'messages' => [ + 'koded.handler.wrong.type' => '"type" must be an exception type', + 'koded.handler.missing' => 'Error handler must either be specified explicitly, or defined as a static method named "handle" that is a member of the given exception type', + 'koded.middleware.implements' => 'A middleware class {0} must implement {1}', + + 'koded.router.noSlash' => 'URI template must begin with \'/\'', + 'koded.router.duplicateSlashes' => 'URI template has duplicate slashes', + 'koded.router.pcre.compilation' => 'PCRE compilation error. {0}', + 'koded.router.invalidRoute.title' => 'Invalid route. No regular expression provided', + 'koded.router.invalidRoute.detail' => 'Provide a proper PCRE regular expression', + 'koded.router.invalidParam.title' => "Invalid route parameter type '{0}'", + 'koded.router.invalidParam.detail' => 'Use one of the supported parameter types', + 'koded.router.duplicateRoute.title' => 'Duplicate route', + 'koded.router.duplicateRoute.detail' => 'Detected a multiple route definitions. The URI template for "%s" conflicts with an already registered route "%s".', + 'koded.router.multiPaths.title' => 'Invalid route. Multiple path parameters in the route template detected', + 'koded.router.multiPaths.detail' => 'Only one "path" type is allowed as URI parameter', + ] +]; diff --git a/src/App.php b/src/App.php index bb2be4f..f82a5b6 100644 --- a/src/App.php +++ b/src/App.php @@ -29,15 +29,18 @@ use function get_class; use function get_debug_type; use function get_parent_class; +use function getenv; use function is_a; use function is_callable; use function method_exists; use function rawurldecode; use function sprintf; -(new WoopsRunner) - ->prependHandler(new PrettyPageHandler) - ->register(); +if (false === getenv('PRODUCTION')) { + (new WoopsRunner) + ->prependHandler(new PrettyPageHandler) + ->register(); +} class App implements RequestHandlerInterface { @@ -125,22 +128,9 @@ public function route( array $middleware = [], bool $explicit = false): App { - try { - $this->container->get(Router::class)->route($uriTemplate, $resource); - $this->explicit[$uriTemplate] = [$explicit, $middleware]; - return $this; - } catch (Throwable $exception) { - $response = $this->container->get(ResponseInterface::class); - if ($this->handleException( - $request = $this->container->get(ServerRequestInterface::class), - $response, - $exception - )) { - ($this->container)($this->renderer, [$request, $response]); - exit; - } - throw $exception; - } + $this->container->get(Router::class)->route($uriTemplate, $resource); + $this->explicit[$uriTemplate] = [$explicit, $middleware]; + return $this; } /** @@ -161,7 +151,11 @@ public function group( /**[ template, resource, middleware, explicit ]**/ $route += ['', '', [], false]; [$uriTemplate, $resource, $mw, $explicit] = $route; - $this->route($prefix . $uriTemplate, $resource, array_merge($middleware, $mw), $explicit); + $this->route($prefix . $uriTemplate, + $resource, + array_merge($middleware, $mw), + $explicit + ); } return $this; } @@ -169,12 +163,10 @@ public function group( public function withErrorHandler(string $type, callable|null $handler): App { if (false === is_a($type, Throwable::class, true)) { - throw new TypeError('"type" must be an exception type', HttpStatus::CONFLICT); + throw new TypeError(__('koded.handler.wrong.type'), HttpStatus::CONFLICT); } if (null === $handler && false === method_exists($type, 'handle')) { - throw new TypeError('Error handler must either be specified explicitly,' . - ' or defined as a static method named "handle" that is a member of' . - ' the given exception type', HttpStatus::NOT_IMPLEMENTED); + throw new TypeError(__('koded.handler.missing'), HttpStatus::NOT_IMPLEMENTED); } $this->handlers[$type] = $handler; return $this; @@ -216,7 +208,9 @@ private function initialize(?string $uriTemplate): void is_a($middleware, MiddlewareInterface::class, true) => $this->container->new($middleware), is_callable($middleware) => new CallableMiddleware($middleware), default => throw new InvalidArgumentException( - sprintf('Middleware "%s" must implement %s', $class, MiddlewareInterface::class) + __('koded.middleware.implements', [ + $class, MiddlewareInterface::class + ]) ) }; } diff --git a/src/Middleware/AuthMiddleware.php b/src/Middleware/AuthMiddleware.php index 4136d78..ca48758 100644 --- a/src/Middleware/AuthMiddleware.php +++ b/src/Middleware/AuthMiddleware.php @@ -8,14 +8,10 @@ class AuthMiddleware implements MiddlewareInterface { - private readonly AuthProcessor $processor; - private readonly AuthBackend $backend; - - public function __construct(AuthProcessor $processor, AuthBackend $backend) - { - $this->processor = $processor; - $this->backend = $backend; - } + public function __construct( + private readonly AuthProcessor $processor, + private readonly AuthBackend $backend + ) {} public function process( ServerRequestInterface $request, diff --git a/src/Module.php b/src/Module.php index 7b68d36..81c4c1a 100644 --- a/src/Module.php +++ b/src/Module.php @@ -6,14 +6,13 @@ use Koded\Framework\Auth\{AuthBackend, AuthProcessor, BearerAuthProcessor, SessionAuthBackend}; use Koded\Http\{ServerRequest, ServerResponse}; use Koded\Http\Interfaces\{Request, Response}; -use Koded\I18n\{I18n, I18nCatalog}; +use Koded\I18n\{I18n, I18nCatalog, CurlyFormatter}; use Koded\Logging\Log; use Koded\Logging\Processors\Cli; use Koded\Stdlib\{Config, Configuration, Immutable}; use Psr\Http\Message\{ResponseInterface, ServerRequestInterface}; use Psr\Log\LoggerInterface; use Psr\SimpleCache\CacheInterface; -use ReflectionClass; use function dirname; use function is_a; use function is_readable; @@ -55,14 +54,14 @@ private function configuration(): Configuration } if (is_a($this->configuration, Configuration::class, true)) { $factory->fromObject($this->configuration); - $factory->rootPath = dirname((new ReflectionClass($this->configuration))->getFileName()); + //$factory->root = dirname((new ReflectionClass($this->configuration))->getFileName()); } elseif (is_readable($this->configuration)) { putenv(self::ENV_KEY . '=' . $this->configuration); $factory->fromEnvVariable(self::ENV_KEY); - $factory->rootPath = dirname($this->configuration); + $factory->root = dirname($this->configuration); } load: - @$factory->fromEnvFile('.env'); + is_readable("$factory->root/.env") and $factory->fromEnvFile("$factory->root/.env"); foreach ($factory->get('autoloaders', []) as $autoloader) { include_once $autoloader; } @@ -74,7 +73,8 @@ private function defaultConfiguration(): Immutable return new Immutable( [ // I18n directives - 'translation.dir' => __DIR__ . '/locale', + 'translation.dir' => __DIR__ . '/../locale', + 'translation.formatter' => CurlyFormatter::class, // CORS overrides (all values are scalar) 'cors.disable' => false, diff --git a/src/Router.php b/src/Router.php index 3f34594..7596cf4 100644 --- a/src/Router.php +++ b/src/Router.php @@ -16,7 +16,6 @@ use function Koded\Stdlib\json_unserialize; use function preg_match; use function preg_match_all; -use function sprintf; use function str_contains; use function str_replace; @@ -45,8 +44,8 @@ public function __destruct() public function route(string $template, object|string $resource): void { - assert('/' === $template[0], 'URI template must begin with "/"'); - assert(false === str_contains($template, '//'), 'URI template has duplicate slashes'); + assert('/' === ($template[0] ?? ''), __('koded.router.noSlash')); + assert(false === str_contains($template, '//'), __('koded.router.duplicateSlashes')); $id = $this->id($template); if ($this->index[$id] ?? false) { @@ -74,10 +73,6 @@ public function match(string $path): array private function normalizeParams(array $route, array $params): array { - if (empty($params)) { - $route['params'] = []; - return $route; - } $route['params'] = json_unserialize(json_serialize( array_filter($params, 'is_string', ARRAY_FILTER_USE_KEY), JSON_NUMERIC_CHECK) @@ -162,7 +157,7 @@ private function compileTemplate(string $template): array ]; } catch (Throwable $ex) { throw new HTTPConflict( - title: 'PCRE compilation error. ' . $ex->getMessage(), + title: __('koded.router.pcre.compilation', [$ex->getMessage()]), detail: $ex->getMessage(), instance: $template, ); @@ -176,13 +171,13 @@ private function assertSupportedType( string $filter): void { ('regex' === $type and empty($filter)) and throw new HTTPConflict( - title: 'Invalid route. No regular expression provided', - detail: 'Provide a proper PCRE regular expression', + title: __('koded.router.invalidRoute.title'), + detail: __('koded.router.invalidRoute.detail'), instance: $template, ); isset($types[$type]) or throw (new HTTPConflict( - title: "Invalid route parameter type '$type'", - detail: 'Use one of the supported parameter types', + title: __('koded.router.invalidParam.title', [$type]), + detail: __('koded.router.invalidParam.detail'), instance: $template, ))->setMember('supported-types', array_keys($types)); } @@ -194,16 +189,13 @@ private function assertIdentityAndPaths( { isset($this->identity[$identity]) and throw (new HTTPConflict( instance: $template, - title: 'Duplicate route', - detail: sprintf( - 'Detected a multiple route definitions. The URI template ' . - 'for "%s" conflicts with an already registered route "%s".', - $template, $this->identity[$identity]) + title: __('koded.router.duplicateRoute.title'), + detail: __('koded.router.duplicateRoute.detail', [$template, $this->identity[$identity]]) ))->setMember('conflict-route', [$template => $this->identity[$identity]]); $paths > 1 and throw new HTTPConflict( - title: 'Invalid route. Multiple path parameters in the route template detected', - detail: 'Only one "path" type is allowed as URI parameter', + title: __('koded.router.multiPaths.title'), + detail: __('koded.router.multiPaths.detail'), instance: $template, ); } diff --git a/src/locale/en_US.php b/src/locale/en_US.php deleted file mode 100644 index 7581e96..0000000 --- a/src/locale/en_US.php +++ /dev/null @@ -1,7 +0,0 @@ - 'English', - 'messages' => [ - ] -]; diff --git a/tests/AppTest.php b/tests/AppTest.php new file mode 100644 index 0000000..e087265 --- /dev/null +++ b/tests/AppTest.php @@ -0,0 +1,21 @@ +assertEquals('UTC', date_default_timezone_get(), + 'App instance always sets the default timezone to UTC'); + } +} diff --git a/tests/DIModuleConfigurationTest.php b/tests/DIModuleConfigurationTest.php new file mode 100644 index 0000000..c34e12e --- /dev/null +++ b/tests/DIModuleConfigurationTest.php @@ -0,0 +1,47 @@ +objectProperty($app, 'container') + ->get(Config::class); + + $this->assertSame('bar', $config->get('test.foo')); + $this->assertSame('BAR', env('TEST_FOO'), + 'ENV variables are loaded from .env in the configured root directory'); + + $this->assertSame([ + __DIR__ . '/Fixtures/test-autoloader.php', + ], $config->get('autoloaders')); + + $this->assertTrue(class_exists(\TestAutoloadedClass::class), + 'Classes are loaded from "autoloaders" directive'); + } + + public function configs() + { + return [ + [__DIR__ . '/Fixtures/test-conf.php'], + [(new Config(__DIR__ . '/Fixtures'))->fromPhpFile(__DIR__ . '/Fixtures/test-conf.php')] + ]; + } +} diff --git a/tests/ErrorHandlersTest.php b/tests/ErrorHandlersTest.php index 50fbf92..8f82572 100644 --- a/tests/ErrorHandlersTest.php +++ b/tests/ErrorHandlersTest.php @@ -2,8 +2,8 @@ namespace Tests\Koded\Framework; +use Exception; use Koded\Framework\App; -use Koded\Framework\Middleware\XPoweredByMiddleware; use Koded\Http\Interfaces\HttpStatus; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ResponseInterface; @@ -13,20 +13,15 @@ class ErrorHandlersTest extends TestCase { public function test_default_exception_handler() { - ini_set('error_log', '/dev/null'); + $_SERVER['HTTP_X_REQUEST_TEST_HEADER'] = 'x request test'; /** * @var ServerRequestInterface $request * @var ResponseInterface $response */ - $app = new App( - renderer: [$this, '_renderer'], - middleware: [XPoweredByMiddleware::class] - ); - - $app->route('/', fn() => throw new \Exception('boooom', 400)); - + $app = new App(renderer: [$this, '_renderer']); + $app->route('/', fn() => throw new Exception('boooom', 400)); [$request, $response] = call_user_func($app); // Request object @@ -68,25 +63,54 @@ public function test_without_exception_handler() { $exceptionMessage = 'boooom'; - $this->expectException(\Exception::class); - $this->expectExceptionCode(400); + $this->expectException(Exception::class); + $this->expectExceptionCode(HttpStatus::BAD_REQUEST); $this->expectExceptionMessage($exceptionMessage); - $app = new App( - renderer: [$this, '_renderer'], - middleware: [XPoweredByMiddleware::class] - ); + $app = new App(renderer: [$this, '_renderer']); + $app->withoutErrorHandler(Exception::class); + $app->route('/', fn() => throw new Exception($exceptionMessage, 400)); + call_user_func($app); + } - $app->withoutErrorHandler(\Exception::class); + public function test_throwable_for_route_method() + { + if (getenv('CI')) { + $this->markTestSkipped('WIP'); + } - $app->route('/', fn() => throw new \Exception($exceptionMessage, 400)); - call_user_func($app); + $this->expectException(\AssertionError::class); + $this->expectExceptionCode(1); + $this->expectExceptionMessage('URI template has duplicate slashes'); + + (new App) + ->route('//', fn(ResponseInterface $r) => $r); + + $this->assertJsonStringEqualsJsonString( + <<<'JSON' + { + "status":409, + "instance":"/", + "detail":"URI template has duplicate slashes", + "title":"Conflict", + "type":"https://httpstatuses.com/409" + } + JSON, + $this->getActualOutput(), + 'The exception is captured in the response payload (JSON by default)' + ); } protected function setUp(): void { $_SERVER['REQUEST_URI'] = '/'; - $_SERVER['HTTP_X_REQUEST_TEST_HEADER'] = 'x request test'; + //ini_set('error_log', '/dev/null'); + } + + protected function tearDown(): void + { + unset($_SERVER['HTTP_X_REQUEST_TEST_HEADER']); + //ini_set('error_log', ''); } public function _renderer(ServerRequestInterface $request, ResponseInterface $response): array diff --git a/tests/Fixtures/.env b/tests/Fixtures/.env new file mode 100644 index 0000000..9bfe423 --- /dev/null +++ b/tests/Fixtures/.env @@ -0,0 +1 @@ +TEST_FOO=BAR diff --git a/tests/Fixtures/test-autoloader.php b/tests/Fixtures/test-autoloader.php new file mode 100644 index 0000000..b3e795c --- /dev/null +++ b/tests/Fixtures/test-autoloader.php @@ -0,0 +1,3 @@ + [ + __DIR__ . '/test-autoloader.php', + ], + + 'test.foo' => 'bar', +]; diff --git a/tests/HTTPErrorSerializationTest.php b/tests/HTTPErrorSerializationTest.php index d10b15b..52ac024 100644 --- a/tests/HTTPErrorSerializationTest.php +++ b/tests/HTTPErrorSerializationTest.php @@ -2,7 +2,9 @@ namespace Tests\Koded\Framework; -use Koded\Http\{HTTPError, HTTPMethodNotAllowed}; +use Koded\Http\HTTPError; +use Koded\Http\HTTPMethodNotAllowed; +use Koded\Http\Interfaces\HttpStatus; use PHPUnit\Framework\TestCase; use function serialize; use function unserialize; @@ -11,11 +13,11 @@ class HTTPErrorSerializationTest extends TestCase { public function test_default_object_serialization() { - $expected = new HTTPError(424); + $expected = new HTTPError(HttpStatus::METHOD_NOT_ALLOWED); $actual = unserialize(serialize($expected)); $this->assertEquals($expected, $actual); - $this->assertNotSame($expected, $actual, '(just test for obvious reasons)'); + $this->assertNotSame($expected, $actual, '(the instances are not same)'); } public function test_full_object_serialization() diff --git a/tests/NoAppRoutesTest.php b/tests/NoAppRoutesTest.php index fe18957..e9bf9cd 100644 --- a/tests/NoAppRoutesTest.php +++ b/tests/NoAppRoutesTest.php @@ -11,7 +11,9 @@ class NoAppRoutesTest extends TestCase { public function test_no_responder() { - $app = new App(renderer: [$this, '_renderer']); + $app = new App( + renderer: fn(ResponseInterface $response) => $response + ); /** @var ResponseInterface $response */ $response = call_user_func($app); @@ -23,9 +25,4 @@ public function test_no_responder() 'No Routes', (string)$response->getBody()); } - - public function _renderer(ResponseInterface $response): ResponseInterface - { - return $response; - } } diff --git a/tests/RouterBasicTest.php b/tests/RouterBasicTest.php index ea4b036..baca7d2 100644 --- a/tests/RouterBasicTest.php +++ b/tests/RouterBasicTest.php @@ -137,4 +137,15 @@ public function test_resource_class_cache() $cache->get($templateId)['resource'], 'Resource classes are stored by FQCN'); } + + public function test_match_without_named_parameters() + { + $router = new Router(new MemoryClient); + $router->route('/\d+', new \stdClass); + $match = $router->match('/123'); + + $this->assertArrayHasKey('params', $match); + $this->assertSame([], $match['params'], + 'Path matches, but there are no named parameters in URI template'); + } } diff --git a/tests/RouterComplexTest.php b/tests/RouterComplexTest.php index 8d2b2aa..b938c7a 100644 --- a/tests/RouterComplexTest.php +++ b/tests/RouterComplexTest.php @@ -7,6 +7,7 @@ use Koded\Framework\Router; use Koded\Stdlib\UUID; use PHPUnit\Framework\TestCase; +use Throwable; class RouterComplexTest extends TestCase { @@ -55,7 +56,7 @@ public function test_duplicate_parameter_names() { try { $this->router->route('/{id:int}/{id:float}', function() {}); - } catch (\Throwable $ex) { + } catch (Throwable $ex) { $this->assertInstanceOf(HTTPConflict::class, $ex); $this->assertStringContainsString( 'PCRE compilation error', @@ -67,7 +68,7 @@ public function test_duplicate_parameter_path_type() { try { $this->router->route('/{path1:path}/{path2:path}', function() {}); - } catch (\Throwable $ex) { + } catch (Throwable $ex) { $this->assertInstanceOf(HTTPConflict::class, $ex); $this->assertStringContainsString( 'Only one "path"', @@ -147,7 +148,7 @@ public function test_invalid_parameter_regex_value() $this->router->route('/{param:regex:}', function() {}); } catch (HTTPConflict $ex) { $this->assertStringContainsString( - 'Invalid route. No regular expression', + 'Invalid route. No regular expression provided', $ex->getTitle()); $this->assertStringContainsString( diff --git a/tests/RouterTemplateTypesTest.php b/tests/RouterTemplateTypesTest.php index a8ad2ba..bc07623 100644 --- a/tests/RouterTemplateTypesTest.php +++ b/tests/RouterTemplateTypesTest.php @@ -6,6 +6,7 @@ use Koded\Framework\Router; use Koded\Stdlib\UUID; use PHPUnit\Framework\TestCase; +use stdClass; class RouterTemplateTypesTest extends TestCase { @@ -34,7 +35,7 @@ public function test_float_type() $match = $this->router->match('/12.345/67.890'); // resolved parameters - $expected = new \stdClass; + $expected = new stdClass; $expected->lon = 12.345; $expected->lat = 67.89; @@ -47,7 +48,7 @@ public function test_regex_type() $match = $this->router->match('/1.234/5.678'); // resolved parameters - $expected = new \stdClass; + $expected = new stdClass; $expected->lon = 1.234; $expected->lat = 5.678; @@ -60,7 +61,7 @@ public function test_path_type() $match = $this->router->match('/foo/bar-123/baz.json'); // resolved parameters - $expected = new \stdClass; + $expected = new stdClass; $expected->uri = 'foo/bar-123/baz.json'; $this->assertEquals($expected, $match['params']); @@ -72,7 +73,7 @@ public function test_parameter_without_type() $match = $this->router->match('/foo-bar-baz.xml'); // resolved parameters - $expected = new \stdClass; + $expected = new stdClass; $expected->param = 'foo-bar-baz.xml'; $this->assertEquals($expected, $match['params']);