From 3bf38cd3d1098868c86a06db7d9ccd3b74e52633 Mon Sep 17 00:00:00 2001 From: Caleb White Date: Sat, 8 Nov 2025 20:36:53 -0600 Subject: [PATCH] fix: deficiencies in the EloquentMagicMethodToQueryBuilderRector --- ...loquentMagicMethodToQueryBuilderRector.php | 129 +++++++++--------- .../Database/Eloquent/Factories/Factory.php | 7 +- stubs/Illuminate/Database/Query/Builder.php | 6 + ...entMagicMethodToQueryBuilderRectorTest.php | 3 - .../Fixture/factory_model.php.inc | 37 +++++ .../Fixture/fixture.php.inc | 4 +- .../model_interface_intersection.php.inc | 25 ++++ .../Fixture/model_scope.php.inc | 17 +++ .../skip_method_matching_scope.php.inc | 7 + .../Fixture/unknown_model.php.inc | 25 ++++ .../Fixture/with_exclude_methods.php.inc | 4 +- .../Source/FooInterface.php | 7 + .../Source/User.php | 20 +++ 13 files changed, 220 insertions(+), 71 deletions(-) create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/factory_model.php.inc create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_interface_intersection.php.inc create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_scope.php.inc create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/skip_method_matching_scope.php.inc create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/unknown_model.php.inc create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Source/FooInterface.php create mode 100644 tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Source/User.php diff --git a/src/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector.php b/src/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector.php index cf1238225..b9fabdc65 100644 --- a/src/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector.php +++ b/src/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector.php @@ -4,17 +4,18 @@ namespace RectorLaravel\Rector\StaticCall; -use Illuminate\Database\Eloquent\Builder as EloquentQueryBuilder; +use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Query\Builder as QueryBuilder; use PhpParser\Node; +use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Identifier; -use PhpParser\Node\Name; +use PHPStan\Analyser\OutOfClassScope; +use PHPStan\Reflection\ClassReflection; +use PHPStan\Reflection\ReflectionProvider; use Rector\Contract\Rector\ConfigurableRectorInterface; use RectorLaravel\AbstractRector; -use ReflectionException; -use ReflectionMethod; use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; use Webmozart\Assert\Assert; @@ -31,6 +32,10 @@ final class EloquentMagicMethodToQueryBuilderRector extends AbstractRector imple */ private array $excludeMethods = []; + public function __construct( + private readonly ReflectionProvider $reflectionProvider + ) {} + public function getRuleDefinition(): RuleDefinition { return new RuleDefinition( @@ -40,13 +45,15 @@ public function getRuleDefinition(): RuleDefinition <<<'CODE_SAMPLE' use App\Models\User; +$user = User::first(); $user = User::find(1); CODE_SAMPLE , <<<'CODE_SAMPLE' use App\Models\User; -$user = User::query()->find(1); +$user = User::query()->first(); +$user = User::find(1); CODE_SAMPLE , [ self::EXCLUDE_METHODS => ['find'], @@ -68,60 +75,50 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - $resolvedType = $this->nodeTypeResolver->getType($node->class); - - $classNames = $resolvedType->getObjectClassNames(); - - if ($classNames === []) { + if (! $node->name instanceof Identifier) { return null; } - $className = $classNames[0]; - - $originalClassName = $this->getName($node->class); // like "self" or "App\Models\User" + $methodName = $node->name->toString(); - if ($originalClassName === null) { + if ( + $methodName === 'query' // short circuit + || in_array($methodName, $this->excludeMethods, true) + ) { return null; } - // does not extend Eloquent Model - if (! is_subclass_of($className, Model::class)) { - return null; - } + $resolvedType = $this->nodeTypeResolver->getType($node->class); - if (! $node->name instanceof Identifier) { - return null; - } + $classNames = $resolvedType->isClassString()->yes() + ? $resolvedType->getClassStringObjectType()->getObjectClassNames() + : $resolvedType->getObjectClassNames(); - $methodName = $node->name->toString(); + $classReflection = null; - // if not a magic method - if (! $this->isMagicMethod($className, $methodName)) { - return null; - } + foreach ($classNames as $className) { + if (! $this->reflectionProvider->hasClass($className)) { + continue; + } - // if method belongs to Eloquent Query Builder or Query Builder - if (! $this->isPublicMethod(EloquentQueryBuilder::class, $methodName) && ! $this->isPublicMethod( - QueryBuilder::class, - $methodName - )) { - return null; - } + $classReflection = $this->reflectionProvider->getClass($className); - if ($node->class instanceof Name) { - $staticCall = $this->nodeFactory->createStaticCall($originalClassName, 'query'); + if (! $classReflection->is(Model::class)) { + continue; + } + + break; } - if (! $node->class instanceof Name) { - $staticCall = new StaticCall($node->class, 'query'); + if (! $classReflection instanceof ClassReflection) { + return null; } - $methodCall = $this->nodeFactory->createMethodCall($staticCall, $methodName); - foreach ($node->args as $arg) { - $methodCall->args[] = $arg; + if (! $this->isMagicMethod($classReflection, $methodName)) { + return null; } - return $methodCall; + return new MethodCall(new StaticCall($node->class, 'query'), $node->name, $node->args); } /** @@ -136,39 +133,45 @@ public function configure(array $configuration): void $this->excludeMethods = $excludeMethods; } - public function isMagicMethod(string $className, string $methodName): bool + private function isMagicMethod(ClassReflection $classReflection, string $methodName): bool { - if (in_array($methodName, $this->excludeMethods, true)) { - return false; + if (! $classReflection->hasMethod($methodName)) { + // if the class doesn't have the method then check if the method is a scope + if ($classReflection->hasMethod('scope' . ucfirst($methodName))) { + return true; + } + + // otherwise, need to check if the method is directly on the EloquentBuilder or QueryBuilder + return $this->isPublicMethod(EloquentBuilder::class, $methodName) + || $this->isPublicMethod(QueryBuilder::class, $methodName); } - try { - $reflectionMethod = new ReflectionMethod($className, $methodName); - } catch (ReflectionException) { - return true; // method does not exist => is magic method + $extendedMethodReflection = $classReflection->getMethod($methodName, new OutOfClassScope); + + if (! $extendedMethodReflection->isPublic() || $extendedMethodReflection->isStatic()) { + return false; } - return false; // not a magic method + $declaringClass = $extendedMethodReflection->getDeclaringClass(); + + // finally, make sure the method is on the builders or a subclass + return $declaringClass->is(EloquentBuilder::class) || $declaringClass->is(QueryBuilder::class); } - public function isPublicMethod(string $className, string $methodName): bool + private function isPublicMethod(string $className, string $methodName): bool { - try { - $reflectionMethod = new ReflectionMethod($className, $methodName); + if (! $this->reflectionProvider->hasClass($className)) { + return false; + } - // if not public - if (! $reflectionMethod->isPublic()) { - return false; - } + $classReflection = $this->reflectionProvider->getClass($className); - // if static - if ($reflectionMethod->isStatic()) { - return false; - } - } catch (ReflectionException) { - return false; // method does not exist => is magic method + if (! $classReflection->hasMethod($methodName)) { + return false; } - return true; // method exist + $extendedMethodReflection = $classReflection->getMethod($methodName, new OutOfClassScope); + + return $extendedMethodReflection->isPublic() && ! $extendedMethodReflection->isStatic(); } } diff --git a/stubs/Illuminate/Database/Eloquent/Factories/Factory.php b/stubs/Illuminate/Database/Eloquent/Factories/Factory.php index eddecf326..4ab7d17c9 100644 --- a/stubs/Illuminate/Database/Eloquent/Factories/Factory.php +++ b/stubs/Illuminate/Database/Eloquent/Factories/Factory.php @@ -6,4 +6,9 @@ return; } -abstract class Factory {} +/** @template TModel of \Illuminate\Database\Eloquent\Model */ +abstract class Factory +{ + /** @var class-string */ + protected $model; +} diff --git a/stubs/Illuminate/Database/Query/Builder.php b/stubs/Illuminate/Database/Query/Builder.php index 2f5925f3c..21447eacb 100644 --- a/stubs/Illuminate/Database/Query/Builder.php +++ b/stubs/Illuminate/Database/Query/Builder.php @@ -22,6 +22,12 @@ public function orderBy($column, string $direction): static {} */ public function orderByDesc($column): static {} + /** + * @param \Illuminate\Contracts\Database\Query\Expression|string $column + * @return mixed + */ + public function max($column) {} + protected function protectedMethodBelongsToQueryBuilder(): void {} private function privateMethodBelongsToQueryBuilder(): void {} diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/EloquentMagicMethodToQueryBuilderRectorTest.php b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/EloquentMagicMethodToQueryBuilderRectorTest.php index 2ba419ef0..27a4d3a5e 100644 --- a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/EloquentMagicMethodToQueryBuilderRectorTest.php +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/EloquentMagicMethodToQueryBuilderRectorTest.php @@ -4,13 +4,10 @@ namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector; -use Illuminate\Database\Eloquent\Model; use Iterator; use PHPUnit\Framework\Attributes\DataProvider; use Rector\Testing\PHPUnit\AbstractRectorTestCase; -final class User extends Model {} - final class EloquentMagicMethodToQueryBuilderRectorTest extends AbstractRectorTestCase { public static function provideData(): Iterator diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/factory_model.php.inc b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/factory_model.php.inc new file mode 100644 index 000000000..15946b7bc --- /dev/null +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/factory_model.php.inc @@ -0,0 +1,37 @@ + */ +class UserFactory extends Factory +{ + public function definition(): array + { + return [ + 'sort' => $this->model::max('sort') + 1, + ]; + } +} +?> +----- + */ +class UserFactory extends Factory +{ + public function definition(): array + { + return [ + 'sort' => $this->model::query()->max('sort') + 1, + ]; + } +} +?> diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/fixture.php.inc b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/fixture.php.inc index 581dc15cb..63e636299 100644 --- a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/fixture.php.inc +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/fixture.php.inc @@ -2,7 +2,7 @@ namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture; -use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\User; +use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User; class SomeController { @@ -37,7 +37,7 @@ class SomeController namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture; -use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\User; +use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User; class SomeController { diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_interface_intersection.php.inc b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_interface_intersection.php.inc new file mode 100644 index 000000000..071b7c3b5 --- /dev/null +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_interface_intersection.php.inc @@ -0,0 +1,25 @@ + +----- +max('sort') + 1; +}; + +?> diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_scope.php.inc b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_scope.php.inc new file mode 100644 index 000000000..49fdecd1b --- /dev/null +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_scope.php.inc @@ -0,0 +1,17 @@ + +----- +foo(); +?> diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/skip_method_matching_scope.php.inc b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/skip_method_matching_scope.php.inc new file mode 100644 index 000000000..5247d8635 --- /dev/null +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/skip_method_matching_scope.php.inc @@ -0,0 +1,7 @@ + $class */ + $class::max('sort') + 1; + $model::max('sort') + 1; +}; +?> +----- + $class */ + $class::query()->max('sort') + 1; + $model::query()->max('sort') + 1; +}; +?> diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/with_exclude_methods.php.inc b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/with_exclude_methods.php.inc index 5266f9f79..f5f6f2f84 100644 --- a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/with_exclude_methods.php.inc +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/with_exclude_methods.php.inc @@ -2,7 +2,7 @@ namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture; -use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\User; +use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User; class SomeController { @@ -29,7 +29,7 @@ class SomeController namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture; -use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\User; +use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User; class SomeController { diff --git a/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Source/FooInterface.php b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Source/FooInterface.php new file mode 100644 index 000000000..30ee1e8cf --- /dev/null +++ b/tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Source/FooInterface.php @@ -0,0 +1,7 @@ +