fix(router)!: return 404 when model binding fails (#2014)

Author: xHeaven

docs/1-essentials/01-routing.md

@@ -171,7 +171,7 @@ use Tempest\Database\IsDatabaseModel;
 
 final class Aircraft implements Bindable
 {
-    public static function resolve(string $input): self
+    public static function resolve(string $input): ?static
     {
         return query(self::class)->resolve($input);
     }
@@ -193,7 +193,7 @@ final class Aircraft implements Bindable
     #[IsBindingValue]
     public string $registrationNumber;
 
-    public static function resolve(string $input): self
+    public static function resolve(string $input): ?static
     {
         return query(self::class)
             ->where('registrationNumber', $input)

packages/database/src/Builder/QueryBuilders/QueryBuilder.php

@@ -169,9 +169,9 @@ public function findById(string|int|PrimaryKey $id): ?object
      * query(User::class)->resolve(1);
      * ```
      *
-     * @return TModel
+     * @return TModel|null
      */
-    public function resolve(string|int|PrimaryKey $id): object
+    public function resolve(string|int|PrimaryKey $id): ?object
     {
         if (! inspect($this->model)->hasPrimaryKey()) {
             throw ModelDidNotHavePrimaryColumn::neededForMethod($this->model, 'resolve');

packages/database/src/IsDatabaseModel.php

@@ -79,7 +79,7 @@ public static function findById(string|int|PrimaryKey $id): ?static
     /**
      * Finds a model instance by its ID. Use through {@see Tempest\Router\Bindable}.
      */
-    public static function resolve(string $input): static
+    public static function resolve(string $input): ?static
     {
         return static::queryBuilder()->resolve($input);
     }

packages/router/src/Bindable.php

@@ -10,5 +10,5 @@ interface Bindable
     /**
      * Resolves the implementing class through the given input.
      */
-    public static function resolve(string $input): self;
+    public static function resolve(string $input): ?static;
 }

packages/upgrade/config/sets/tempest30.php

@@ -4,6 +4,7 @@
 use Rector\Configuration\Option;
 use Rector\Configuration\Parameter\SimpleParameterProvider;
 use Tempest\Upgrade\Tempest3\UpdateArrMapFunctionRector;
+use Tempest\Upgrade\Tempest3\UpdateBindableResolveReturnTypeRector;
 use Tempest\Upgrade\Tempest3\UpdateCommandFunctionImportsRector;
 use Tempest\Upgrade\Tempest3\UpdateContainerFunctionImportsRector;
 use Tempest\Upgrade\Tempest3\UpdateEventFunctionImportsRector;
@@ -18,6 +19,7 @@
     SimpleParameterProvider::setParameter(Option::IMPORT_SHORT_CLASSES, value: true);
 
     $config->rule(UpdateArrMapFunctionRector::class);
+    $config->rule(UpdateBindableResolveReturnTypeRector::class);
     $config->rule(UpdateCommandFunctionImportsRector::class);
     $config->rule(UpdateContainerFunctionImportsRector::class);
     $config->rule(UpdateEventFunctionImportsRector::class);

packages/upgrade/src/Tempest3/UpdateBindableResolveReturnTypeRector.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Tempest\Upgrade\Tempest3;
+
+use PhpParser\Node;
+use PhpParser\Node\Identifier;
+use PhpParser\Node\Name;
+use PhpParser\Node\NullableType;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Interface_;
+use Rector\Rector\AbstractRector;
+
+final class UpdateBindableResolveReturnTypeRector extends AbstractRector
+{
+    public function getNodeTypes(): array
+    {
+        return [
+            Node\Stmt\Class_::class,
+            Interface_::class,
+        ];
+    }
+
+    public function refactor(Node $node): ?int
+    {
+        if ($node instanceof Interface_) {
+            if (! $this->hasBindableName($node->extends)) {
+                return null;
+            }
+
+            $this->refactorMethods($node->getMethods());
+
+            return null;
+        }
+
+        if (! $node instanceof Node\Stmt\Class_) {
+            return null;
+        }
+
+        if (! $this->hasBindableName($node->implements)) {
+            return null;
+        }
+
+        $this->refactorMethods($node->getMethods());
+
+        return null;
+    }
+
+    /**
+     * @param Name[] $names
+     */
+    private function hasBindableName(array $names): bool
+    {
+        foreach ($names as $name) {
+            $value = ltrim($name->toString(), '\\');
+
+            if ($value === 'Tempest\\Router\\Bindable' || $value === 'Bindable') {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * @param ClassMethod[] $methods
+     */
+    private function refactorMethods(array $methods): void
+    {
+        foreach ($methods as $method) {
+            if ($method->name->toString() !== 'resolve' || ! $method->isStatic()) {
+                continue;
+            }
+
+            if ($method->returnType instanceof NullableType && $method->returnType->type instanceof Identifier && $method->returnType->type->toString() === 'static') {
+                continue;
+            }
+
+            $method->returnType = new NullableType(new Identifier('static'));
+        }
+    }
+}

packages/upgrade/tests/Tempest3/Fixtures/BindableResolveReturnType.input.php

@@ -0,0 +1,11 @@
+<?php
+
+use Tempest\Router\Bindable;
+
+final class UserBinding implements Bindable
+{
+    public static function resolve(string $input): self
+    {
+        return new self();
+    }
+}

packages/upgrade/tests/Tempest3/Tempest3RectorTest.php

@@ -116,4 +116,12 @@ public function test_fully_qualified_map_iterable_call(): void
             ->assertContains('use Tempest\Support\Arr\map;')
             ->assertContains('return map($data, fn ($item) => $item * 2);');
     }
+
+    public function test_bindable_resolve_return_type_becomes_nullable(): void
+    {
+        $this->rector
+            ->runFixture(__DIR__ . '/Fixtures/BindableResolveReturnType.input.php')
+            ->assertContains('public static function resolve(string $input): ?static')
+            ->assertNotContains('public static function resolve(string $input): self');
+    }
 }

tests/Integration/Route/RouterTest.php

@@ -103,6 +103,20 @@ public function test_route_binding(): void
         $this->assertSame('Test', $response->body);
     }
 
+    public function test_route_binding_with_nonexistent_model_returns_404(): void
+    {
+        $this->database->migrate(
+            CreateMigrationsTable::class,
+            CreatePublishersTable::class,
+            CreateAuthorTable::class,
+            CreateBookTable::class,
+        );
+
+        $this->http
+            ->get('/books/999')
+            ->assertNotFound();
+    }
+
     public function test_middleware(): void
     {
         $this