feat: handle review feedback - basePath forwarding, type coercion, inspector hrefs, test improvements

josbeir · josbeir · commit 8fff4dbd3fef · 2026-03-14T18:58:52.000+01:00
diff --git a/resources/templates/inspector/routes.sugar.php b/resources/templates/inspector/routes.sugar.php
@@ -1,6 +1,7 @@
 <?php
 /**
  * @var array<\Glaze\Content\ContentPage> $pages
+ * @var string $basePath Configured site base path (empty string when not set).
  */
 ?>
 <!DOCTYPE html>
@@ -43,7 +44,7 @@
                     <tbody>
                         <tr s:foreach="$pages as $page">
                             <td>
-                                <a href="<?= $page->urlPath ?>" class="glaze-link">
+                                <a href="<?= $basePath . $page->urlPath ?>" class="glaze-link">
                                     <?= $page->urlPath ?>
                                 </a>
                             </td>
diff --git a/src/Http/Controller/InspectorController.php b/src/Http/Controller/InspectorController.php
@@ -52,6 +52,9 @@ public function routes(): array
             $this->config->contentTypes,
         );
 
-        return ['pages' => $pages];
+        return [
+            'pages' => $pages,
+            'basePath' => $this->config->site->basePath ?? '',
+        ];
     }
 }
diff --git a/src/Http/Middleware/ControllerMiddleware.php b/src/Http/Middleware/ControllerMiddleware.php
@@ -74,11 +74,14 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
     {
         $this->discoverOnce();
 
-        $request = $request->withUri(
-            $request->getUri()->withPath($this->stripBasePathFromRequestPath($request->getUri()->getPath())),
-        );
+        // Use a stripped copy for route matching so the original request (with any
+        // basePath prefix intact) is forwarded unchanged when no route matches.
+        // Downstream handlers such as DevPageRequestHandler rely on the original
+        // path for canonical redirects and Location headers.
+        $strippedPath = $this->stripBasePathFromRequestPath($request->getUri()->getPath());
+        $routingRequest = $request->withUri($request->getUri()->withPath($strippedPath));
 
-        $match = $this->router->match($request);
+        $match = $this->router->match($routingRequest);
         if (!$match instanceof MatchedRoute) {
             return $handler->handle($request);
         }
@@ -87,7 +90,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
         $controller = $this->container->get($match->controllerClass);
 
         $reflectionMethod = new ReflectionMethod($controller, $match->actionMethod);
-        $args = $this->resolveArguments($reflectionMethod, $request, $match->params);
+        $args = $this->resolveArguments($reflectionMethod, $routingRequest, $match->params);
 
         $result = $reflectionMethod->invokeArgs($controller, $args);
 
@@ -165,9 +168,20 @@ protected function resolveArguments(
                 continue;
             }
 
-            // 2. Path parameter by name.
+            // 2. Path parameter by name — coerced to the declared builtin type when possible.
             if (array_key_exists($name, $params)) {
-                $args[] = $params[$name];
+                $rawValue = $params[$name];
+
+                if ($type instanceof ReflectionNamedType && $type->isBuiltin()) {
+                    $rawValue = $this->coercePathParam(
+                        $rawValue,
+                        $type->getName(),
+                        $name,
+                        $method,
+                    );
+                }
+
+                $args[] = $rawValue;
                 continue;
             }
 
@@ -193,4 +207,56 @@ protected function resolveArguments(
 
         return $args;
     }
+
+    /**
+     * Coerce a raw path-parameter string to the declared PHP builtin type.
+     *
+     * Handles `int`, `float`, and `bool`; passes all other types through as-is.
+     *
+     * @param string $raw Raw string extracted from the URL path.
+     * @param string $typeName PHP builtin type name (e.g. 'int', 'float', 'bool', 'string').
+     * @param string $paramName Parameter name (used in error messages).
+     * @param \ReflectionMethod $method Reflected action method (used in error messages).
+     * @return string|float|int|bool Coerced value.
+     * @throws \RuntimeException When the value cannot be converted to the required type.
+     */
+    private function coercePathParam(
+        string $raw,
+        string $typeName,
+        string $paramName,
+        ReflectionMethod $method,
+    ): int|float|bool|string {
+        return match ($typeName) {
+            'int' => is_numeric($raw) && !str_contains($raw, '.')
+                ? (int)$raw
+                : throw new RuntimeException(sprintf(
+                    'Path parameter "$%s" for "%s::%s" expects int, got "%s".',
+                    $paramName,
+                    $method->getDeclaringClass()->getName(),
+                    $method->getName(),
+                    $raw,
+                )),
+            'float' => is_numeric($raw)
+                ? (float)$raw
+                : throw new RuntimeException(sprintf(
+                    'Path parameter "$%s" for "%s::%s" expects float, got "%s".',
+                    $paramName,
+                    $method->getDeclaringClass()->getName(),
+                    $method->getName(),
+                    $raw,
+                )),
+            'bool' => match (strtolower($raw)) {
+                '1', 'true', 'yes', 'on' => true,
+                '0', 'false', 'no', 'off', '' => false,
+                default => throw new RuntimeException(sprintf(
+                    'Path parameter "$%s" for "%s::%s" expects bool, got "%s".',
+                    $paramName,
+                    $method->getDeclaringClass()->getName(),
+                    $method->getName(),
+                    $raw,
+                )),
+            },
+            default => $raw,
+        };
+    }
 }
diff --git a/tests/Unit/Http/Middleware/ControllerMiddlewareTest.php b/tests/Unit/Http/Middleware/ControllerMiddlewareTest.php
@@ -16,6 +16,7 @@
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\RequestHandlerInterface;
+use ReflectionProperty;
 
 /**
  * Tests for ControllerMiddleware routing and injection behavior.
@@ -174,8 +175,9 @@ public function check(ServerRequestInterface $request): ResponseInterface {
     /**
      * Ensure controller discovery only runs once even across multiple requests.
      *
-     * A temp directory is created with one controller file. Both requests match
-     * the same route, confirming discovery persisted after the first request.
+     * After the first request triggers discovery, the route table is inspected
+     * via reflection before and after a second request to confirm the entries
+     * were not duplicated or cleared.
      */
     public function testDiscoveryRunsOnlyOnce(): void
     {
@@ -213,9 +215,113 @@ public function index(): ResponseInterface {
         $response1 = $middleware->process($request, $this->fallbackHandler());
         $this->assertSame(200, $response1->getStatusCode());
 
-        // Second request — discovery must NOT clear and re-run (route still matched).
+        // Capture route table size immediately after discovery.
+        $routesProp = new ReflectionProperty($router, 'routes');
+        /** @var array<mixed> $routesAfterFirst */
+        $routesAfterFirst = $routesProp->getValue($router);
+        $routeCountAfterFirstRequest = count($routesAfterFirst);
+        $this->assertGreaterThan(0, $routeCountAfterFirstRequest, 'Routes must be registered after first request.');
+
+        // Second request — discovery must NOT re-run; the route table must be unchanged.
         $response2 = $middleware->process($request, $this->fallbackHandler());
         $this->assertSame(200, $response2->getStatusCode());
+        /** @var array<mixed> $routesAfterSecond */
+        $routesAfterSecond = $routesProp->getValue($router);
+        $this->assertCount(
+            $routeCountAfterFirstRequest,
+            $routesAfterSecond,
+            'Route count must not change after the second request; discoverOnce must not have re-run.',
+        );
+    }
+
+    /**
+     * Ensure the original (basePath-prefixed) request is forwarded unchanged when no route matches.
+     *
+     * Downstream handlers such as DevPageRequestHandler rely on the full original
+     * path (including any basePath prefix) for canonical redirects and Location headers.
+     */
+    public function testProcessForwardsOriginalRequestPathOnMiss(): void
+    {
+        $projectRoot = $this->createProjectRoot();
+        file_put_contents($projectRoot . '/glaze.neon', "site:\n  basePath: /app\n");
+        $config = BuildConfig::fromProjectRoot($projectRoot, true);
+
+        $router = new ControllerRouter();
+        $middleware = new ControllerMiddleware(
+            $router,
+            $this->makeViewRenderer($config),
+            $this->container(),
+            $config,
+            $this->createTempDirectory(),
+        );
+
+        $capturedPath = null;
+        $capturingHandler = new class ($capturedPath) implements RequestHandlerInterface {
+            public function __construct(public ?string &$path)
+            {
+            }
+
+            /**
+             * @inheritDoc
+             */
+            public function handle(ServerRequestInterface $request): ResponseInterface
+            {
+                $this->path = $request->getUri()->getPath();
+
+                return (new Response(['charset' => 'UTF-8']))->withStatus(404)->withStringBody('miss');
+            }
+        };
+
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/app/about/');
+        $middleware->process($request, $capturingHandler);
+
+        $this->assertSame('/app/about/', $capturedPath, 'The original basePath-prefixed path must be forwarded unchanged.');
+    }
+
+    /**
+     * Ensure path parameters are coerced to declared scalar types (int, float, bool).
+     */
+    public function testProcessCoercesScalarPathParams(): void
+    {
+        $projectRoot = $this->createProjectRoot();
+        $config = BuildConfig::fromProjectRoot($projectRoot, true);
+
+        $controllersDir = $projectRoot . '/controllers';
+        mkdir($controllersDir, 0755, true);
+        file_put_contents($controllersDir . '/TypedController.php', <<<'PHP'
+        <?php
+        declare(strict_types=1);
+        use Cake\Http\Response;
+        use Glaze\Http\Attribute\Route;
+        use Psr\Http\Message\ResponseInterface;
+        final class TypedController {
+            #[Route('/items/{id}/{score}/{active}')]
+            public function show(int $id, float $score, bool $active): ResponseInterface {
+                return (new Response(['charset' => 'UTF-8']))
+                    ->withStatus(200)
+                    ->withStringBody(json_encode(['id' => $id, 'score' => $score, 'active' => $active]));
+            }
+        }
+        PHP);
+
+        $router = new ControllerRouter();
+        $middleware = new ControllerMiddleware(
+            $router,
+            $this->makeViewRenderer($config),
+            $this->container(),
+            $config,
+            $controllersDir,
+        );
+
+        $request = (new ServerRequestFactory())->createServerRequest('GET', '/items/42/3.14/true');
+        $response = $middleware->process($request, $this->fallbackHandler());
+
+        $this->assertSame(200, $response->getStatusCode());
+        $decoded = json_decode((string)$response->getBody(), true);
+        $this->assertIsArray($decoded);
+        $this->assertSame(42, $decoded['id']);
+        $this->assertEqualsWithDelta(3.14, $decoded['score'], PHP_FLOAT_EPSILON);
+        $this->assertTrue($decoded['active']);
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -52,6 +52,9 @@ public function routes(): array`
`52`	`52`	`$this->config->contentTypes,`
`53`	`53`	`);`
`54`	`54`
`55`		`- return ['pages' => $pages];`
	`55`	`+ return [`
	`56`	`+ 'pages' => $pages,`
	`57`	`+ 'basePath' => $this->config->site->basePath ?? '',`
	`58`	`+ ];`
`56`	`59`	`}`
`57`	`60`	`}`