Skip to content

Commit 3bf38cd

Browse files
calebdwcalebdw
committed
fix: deficiencies in the EloquentMagicMethodToQueryBuilderRector
1 parent ca9c906 commit 3bf38cd

File tree

13 files changed

+220
-71
lines changed

13 files changed

+220
-71
lines changed

‎src/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector.php‎

Lines changed: 66 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,18 @@
44

55
namespace RectorLaravel\Rector\StaticCall;
66

7-
use Illuminate\Database\Eloquent\Builder as EloquentQueryBuilder;
7+
use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
88
use Illuminate\Database\Eloquent\Model;
99
use Illuminate\Database\Query\Builder as QueryBuilder;
1010
use PhpParser\Node;
11+
use PhpParser\Node\Expr\MethodCall;
1112
use PhpParser\Node\Expr\StaticCall;
1213
use PhpParser\Node\Identifier;
13-
use PhpParser\Node\Name;
14+
use PHPStan\Analyser\OutOfClassScope;
15+
use PHPStan\Reflection\ClassReflection;
16+
use PHPStan\Reflection\ReflectionProvider;
1417
use Rector\Contract\Rector\ConfigurableRectorInterface;
1518
use RectorLaravel\AbstractRector;
16-
use ReflectionException;
17-
use ReflectionMethod;
1819
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
1920
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
2021
use Webmozart\Assert\Assert;
@@ -31,6 +32,10 @@ final class EloquentMagicMethodToQueryBuilderRector extends AbstractRector imple
3132
*/
3233
private array $excludeMethods = [];
3334

35+
public function __construct(
36+
private readonly ReflectionProvider $reflectionProvider
37+
) {}
38+
3439
public function getRuleDefinition(): RuleDefinition
3540
{
3641
return new RuleDefinition(
@@ -40,13 +45,15 @@ public function getRuleDefinition(): RuleDefinition
4045
<<<'CODE_SAMPLE'
4146
use App\Models\User;
4247
48+
$user = User::first();
4349
$user = User::find(1);
4450
CODE_SAMPLE
4551
,
4652
<<<'CODE_SAMPLE'
4753
use App\Models\User;
4854
49-
$user = User::query()->find(1);
55+
$user = User::query()->first();
56+
$user = User::find(1);
5057
CODE_SAMPLE
5158
, [
5259
self::EXCLUDE_METHODS => ['find'],
@@ -68,60 +75,50 @@ public function getNodeTypes(): array
6875
*/
6976
public function refactor(Node $node): ?Node
7077
{
71-
$resolvedType = $this->nodeTypeResolver->getType($node->class);
72-
73-
$classNames = $resolvedType->getObjectClassNames();
74-
75-
if ($classNames === []) {
78+
if (! $node->name instanceof Identifier) {
7679
return null;
7780
}
7881

79-
$className = $classNames[0];
80-
81-
$originalClassName = $this->getName($node->class); // like "self" or "App\Models\User"
82+
$methodName = $node->name->toString();
8283

83-
if ($originalClassName === null) {
84+
if (
85+
$methodName === 'query' // short circuit
86+
|| in_array($methodName, $this->excludeMethods, true)
87+
) {
8488
return null;
8589
}
8690

87-
// does not extend Eloquent Model
88-
if (! is_subclass_of($className, Model::class)) {
89-
return null;
90-
}
91+
$resolvedType = $this->nodeTypeResolver->getType($node->class);
9192

92-
if (! $node->name instanceof Identifier) {
93-
return null;
94-
}
93+
$classNames = $resolvedType->isClassString()->yes()
94+
? $resolvedType->getClassStringObjectType()->getObjectClassNames()
95+
: $resolvedType->getObjectClassNames();
9596

96-
$methodName = $node->name->toString();
97+
$classReflection = null;
9798

98-
// if not a magic method
99-
if (! $this->isMagicMethod($className, $methodName)) {
100-
return null;
101-
}
99+
foreach ($classNames as $className) {
100+
if (! $this->reflectionProvider->hasClass($className)) {
101+
continue;
102+
}
102103

103-
// if method belongs to Eloquent Query Builder or Query Builder
104-
if (! $this->isPublicMethod(EloquentQueryBuilder::class, $methodName) && ! $this->isPublicMethod(
105-
QueryBuilder::class,
106-
$methodName
107-
)) {
108-
return null;
109-
}
104+
$classReflection = $this->reflectionProvider->getClass($className);
110105

111-
if ($node->class instanceof Name) {
112-
$staticCall = $this->nodeFactory->createStaticCall($originalClassName, 'query');
106+
if (! $classReflection->is(Model::class)) {
107+
continue;
108+
}
109+
110+
break;
113111
}
114112

115-
if (! $node->class instanceof Name) {
116-
$staticCall = new StaticCall($node->class, 'query');
113+
if (! $classReflection instanceof ClassReflection) {
114+
return null;
117115
}
118116

119-
$methodCall = $this->nodeFactory->createMethodCall($staticCall, $methodName);
120-
foreach ($node->args as $arg) {
121-
$methodCall->args[] = $arg;
117+
if (! $this->isMagicMethod($classReflection, $methodName)) {
118+
return null;
122119
}
123120

124-
return $methodCall;
121+
return new MethodCall(new StaticCall($node->class, 'query'), $node->name, $node->args);
125122
}
126123

127124
/**
@@ -136,39 +133,45 @@ public function configure(array $configuration): void
136133
$this->excludeMethods = $excludeMethods;
137134
}
138135

139-
public function isMagicMethod(string $className, string $methodName): bool
136+
private function isMagicMethod(ClassReflection $classReflection, string $methodName): bool
140137
{
141-
if (in_array($methodName, $this->excludeMethods, true)) {
142-
return false;
138+
if (! $classReflection->hasMethod($methodName)) {
139+
// if the class doesn't have the method then check if the method is a scope
140+
if ($classReflection->hasMethod('scope' . ucfirst($methodName))) {
141+
return true;
142+
}
143+
144+
// otherwise, need to check if the method is directly on the EloquentBuilder or QueryBuilder
145+
return $this->isPublicMethod(EloquentBuilder::class, $methodName)
146+
|| $this->isPublicMethod(QueryBuilder::class, $methodName);
143147
}
144148

145-
try {
146-
$reflectionMethod = new ReflectionMethod($className, $methodName);
147-
} catch (ReflectionException) {
148-
return true; // method does not exist => is magic method
149+
$extendedMethodReflection = $classReflection->getMethod($methodName, new OutOfClassScope);
150+
151+
if (! $extendedMethodReflection->isPublic() || $extendedMethodReflection->isStatic()) {
152+
return false;
149153
}
150154

151-
return false; // not a magic method
155+
$declaringClass = $extendedMethodReflection->getDeclaringClass();
156+
157+
// finally, make sure the method is on the builders or a subclass
158+
return $declaringClass->is(EloquentBuilder::class) || $declaringClass->is(QueryBuilder::class);
152159
}
153160

154-
public function isPublicMethod(string $className, string $methodName): bool
161+
private function isPublicMethod(string $className, string $methodName): bool
155162
{
156-
try {
157-
$reflectionMethod = new ReflectionMethod($className, $methodName);
163+
if (! $this->reflectionProvider->hasClass($className)) {
164+
return false;
165+
}
158166

159-
// if not public
160-
if (! $reflectionMethod->isPublic()) {
161-
return false;
162-
}
167+
$classReflection = $this->reflectionProvider->getClass($className);
163168

164-
// if static
165-
if ($reflectionMethod->isStatic()) {
166-
return false;
167-
}
168-
} catch (ReflectionException) {
169-
return false; // method does not exist => is magic method
169+
if (! $classReflection->hasMethod($methodName)) {
170+
return false;
170171
}
171172

172-
return true; // method exist
173+
$extendedMethodReflection = $classReflection->getMethod($methodName, new OutOfClassScope);
174+
175+
return $extendedMethodReflection->isPublic() && ! $extendedMethodReflection->isStatic();
173176
}
174177
}

‎stubs/Illuminate/Database/Eloquent/Factories/Factory.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,9 @@
66
return;
77
}
88

9-
abstract class Factory {}
9+
/** @template TModel of \Illuminate\Database\Eloquent\Model */
10+
abstract class Factory
11+
{
12+
/** @var class-string<TModel> */
13+
protected $model;
14+
}

‎stubs/Illuminate/Database/Query/Builder.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ public function orderBy($column, string $direction): static {}
2222
*/
2323
public function orderByDesc($column): static {}
2424

25+
/**
26+
* @param \Illuminate\Contracts\Database\Query\Expression|string $column
27+
* @return mixed
28+
*/
29+
public function max($column) {}
30+
2531
protected function protectedMethodBelongsToQueryBuilder(): void {}
2632

2733
private function privateMethodBelongsToQueryBuilder(): void {}

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/EloquentMagicMethodToQueryBuilderRectorTest.php‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,10 @@
44

55
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector;
66

7-
use Illuminate\Database\Eloquent\Model;
87
use Iterator;
98
use PHPUnit\Framework\Attributes\DataProvider;
109
use Rector\Testing\PHPUnit\AbstractRectorTestCase;
1110

12-
final class User extends Model {}
13-
1411
final class EloquentMagicMethodToQueryBuilderRectorTest extends AbstractRectorTestCase
1512
{
1613
public static function provideData(): Iterator

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/factory_model.php.inc‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
4+
5+
use Illuminate\Database\Eloquent\Factories\Factory;
6+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
7+
8+
/** @extends Factory<User> */
9+
class UserFactory extends Factory
10+
{
11+
public function definition(): array
12+
{
13+
return [
14+
'sort' => $this->model::max('sort') + 1,
15+
];
16+
}
17+
}
18+
?>
19+
-----
20+
<?php
21+
22+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
23+
24+
use Illuminate\Database\Eloquent\Factories\Factory;
25+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
26+
27+
/** @extends Factory<User> */
28+
class UserFactory extends Factory
29+
{
30+
public function definition(): array
31+
{
32+
return [
33+
'sort' => $this->model::query()->max('sort') + 1,
34+
];
35+
}
36+
}
37+
?>

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/fixture.php.inc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
44

5-
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\User;
5+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
66

77
class SomeController
88
{
@@ -37,7 +37,7 @@ class SomeController
3737

3838
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
3939

40-
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\User;
40+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
4141

4242
class SomeController
4343
{

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_interface_intersection.php.inc‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
4+
5+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\FooInterface;
6+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
7+
8+
function (User&FooInterface $user) {
9+
$user::max('sort') + 1;
10+
};
11+
12+
?>
13+
-----
14+
<?php
15+
16+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
17+
18+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\FooInterface;
19+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
20+
21+
function (User&FooInterface $user) {
22+
$user::query()->max('sort') + 1;
23+
};
24+
25+
?>

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/model_scope.php.inc‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
4+
5+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
6+
7+
User::foo();
8+
?>
9+
-----
10+
<?php
11+
12+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
13+
14+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
15+
16+
User::query()->foo();
17+
?>

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/skip_method_matching_scope.php.inc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
4+
5+
use RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Source\User;
6+
7+
User::conflict();

‎tests/Rector/StaticCall/EloquentMagicMethodToQueryBuilderRector/Fixture/unknown_model.php.inc‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
4+
5+
use Illuminate\Database\Eloquent\Model;
6+
7+
function (string $class, Model $model) {
8+
/** @var class-string<Model> $class */
9+
$class::max('sort') + 1;
10+
$model::max('sort') + 1;
11+
};
12+
?>
13+
-----
14+
<?php
15+
16+
namespace RectorLaravel\Tests\Rector\StaticCall\EloquentMagicMethodToQueryBuilderRector\Fixture;
17+
18+
use Illuminate\Database\Eloquent\Model;
19+
20+
function (string $class, Model $model) {
21+
/** @var class-string<Model> $class */
22+
$class::query()->max('sort') + 1;
23+
$model::query()->max('sort') + 1;
24+
};
25+
?>

0 commit comments

Comments
 (0)