Skip to content

Commit b301369

Browse files
paulbalandanpaulbalandan
committed
Refactor ServicesFunctionArgumentTypeRule
1 parent 0734f0c commit b301369

File tree

6 files changed

+117
-28
lines changed

6 files changed

+117
-28
lines changed

‎README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ parameters:
5555
5656
```
5757

58-
For the `service()` and `single_service()` functions, you can declare your own service implementation classes.
58+
For the `service()` and `single_service()` functions, you can instruct PHPStan to consider your own
59+
services factory classes. **Please note that it should be a valid class extending `CodeIgniter\Config\BaseService`!**
5960

6061
```yml
6162
parameters:

‎src/Rules/Functions/ServicesFunctionArgumentTypeRule.php‎

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,11 @@
1616
use CodeIgniter\PHPStan\Type\ServicesReturnTypeHelper;
1717
use PhpParser\Node;
1818
use PHPStan\Analyser\Scope;
19+
use PHPStan\Reflection\ClassReflection;
1920
use PHPStan\Reflection\ReflectionProvider;
2021
use PHPStan\Rules\Rule;
2122
use PHPStan\Rules\RuleErrorBuilder;
23+
use PHPStan\Type\MixedType;
2224
use PHPStan\Type\VerbosityLevel;
2325

2426
/**
@@ -66,23 +68,50 @@ public function processNode(Node $node, Scope $scope): array
6668

6769
$returnType = $this->servicesReturnTypeHelper->check($nameType, $scope);
6870

69-
if ($returnType->isNull()->no()) {
71+
if ($returnType->isObject()->yes()) {
7072
return [];
7173
}
7274

7375
$name = $nameType->describe(VerbosityLevel::precise());
7476

75-
if (in_array(strtolower(trim($name, "'")), ServicesReturnTypeHelper::IMPOSSIBLE_SERVICE_METHOD_NAMES, true)) {
77+
$trimmedName = trim($name, "'");
78+
79+
if (in_array(strtolower($trimmedName), ServicesReturnTypeHelper::IMPOSSIBLE_SERVICE_METHOD_NAMES, true)) {
80+
return [
81+
RuleErrorBuilder::message(sprintf('The method %s is reserved for service location internals and cannot be used as a service method.', $name))
82+
->identifier('codeigniter.reservedServiceName')
83+
->build(),
84+
];
85+
}
86+
87+
if ($returnType->isNull()->maybe() && $returnType instanceof MixedType) {
88+
return [
89+
RuleErrorBuilder::message(sprintf('Service method %s returns mixed.', $name))
90+
->tip('Perhaps you forgot to add a return type?')
91+
->identifier('codeigniter.serviceMixedReturn')
92+
->build(),
93+
];
94+
}
95+
96+
$hasMethod = array_reduce(
97+
$this->servicesReturnTypeHelper->getServicesReflection(),
98+
static fn (bool $carry, ClassReflection $service): bool => $carry || $service->hasMethod($trimmedName),
99+
false
100+
);
101+
102+
if (! $returnType->isNull()->yes() || $hasMethod) {
76103
return [RuleErrorBuilder::message(sprintf(
77-
'The name %s is reserved for service location internals and cannot be used as a service name.',
78-
$name
79-
))->identifier('codeigniter.reservedServiceName')->build()];
104+
'Service method %s expected to return a service instance, got %s instead.',
105+
$name,
106+
$returnType->describe(VerbosityLevel::precise())
107+
))->identifier('codeigniter.serviceNonObjectReturn')->build()];
80108
}
81109

82110
$addTip = static function (RuleErrorBuilder $ruleErrorBuilder) use ($nameType): RuleErrorBuilder {
83111
foreach ($nameType->getConstantStrings() as $constantStringType) {
84112
$ruleErrorBuilder->addTip(sprintf(
85-
'If %s is a valid service name, you can add its possible service class(es) in <fg=cyan>codeigniter.additionalServices</> in your <fg=yellow>%%configurationFile%%</>.',
113+
'If %s is a valid service method, you can add its possible services factory class(es) ' .
114+
'in <fg=cyan>codeigniter.additionalServices</> in your <fg=yellow>%%configurationFile%%</>.',
86115
$constantStringType->describe(VerbosityLevel::precise()),
87116
));
88117
}
@@ -91,8 +120,8 @@ public function processNode(Node $node, Scope $scope): array
91120
};
92121

93122
return [$addTip(RuleErrorBuilder::message(sprintf(
94-
'Call to unknown service name %s.',
123+
'Call to unknown service method %s.',
95124
$nameType->describe(VerbosityLevel::precise())
96-
)))->identifier('codeigniter.unknownServiceName')->build()];
125+
)))->identifier('codeigniter.unknownServiceMethod')->build()];
97126
}
98127
}

‎src/Type/ServicesReturnTypeHelper.php‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
namespace CodeIgniter\PHPStan\Type;
1515

16+
use CodeIgniter\Config\BaseService;
1617
use CodeIgniter\Config\Services as FrameworkServices;
1718
use Config\Services as AppServices;
1819
use PHPStan\Analyser\Scope;
@@ -69,15 +70,7 @@ public function __construct(
6970

7071
public function check(Type $type, Scope $scope): Type
7172
{
72-
if (self::$servicesReflection === []) {
73-
self::$servicesReflection = array_map(function (string $service): ClassReflection {
74-
if (! $this->reflectionProvider->hasClass($service)) {
75-
throw new ShouldNotHappenException(sprintf('Service class "%s" is not found.', $service));
76-
}
77-
78-
return $this->reflectionProvider->getClass($service);
79-
}, $this->services);
80-
}
73+
$this->buildServicesCache();
8174

8275
return TypeTraverser::map($type, static function (Type $type, callable $traverse) use ($scope): Type {
8376
if ($type instanceof UnionType || $type instanceof IntersectionType) {
@@ -115,4 +108,33 @@ public function check(Type $type, Scope $scope): Type
115108
return new NullType();
116109
});
117110
}
111+
112+
/**
113+
* @return array<int, ClassReflection>
114+
*/
115+
public function getServicesReflection(): array
116+
{
117+
$this->buildServicesCache();
118+
119+
return self::$servicesReflection;
120+
}
121+
122+
private function buildServicesCache(): void
123+
{
124+
if (self::$servicesReflection === []) {
125+
self::$servicesReflection = array_map(function (string $service): ClassReflection {
126+
if (! $this->reflectionProvider->hasClass($service)) {
127+
throw new ShouldNotHappenException(sprintf('Services factory class "%s" not found.', $service));
128+
}
129+
130+
$serviceReflection = $this->reflectionProvider->getClass($service);
131+
132+
if ($serviceReflection->getParentClass()?->getName() !== BaseService::class) {
133+
throw new ShouldNotHappenException(sprintf('Services factory class "%s" does not extend %s.', $service, BaseService::class));
134+
}
135+
136+
return $serviceReflection;
137+
}, $this->services);
138+
}
139+
}
118140
}

‎tests/Fixtures/Type/OtherServices.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,4 +31,22 @@ public static function invoker(string $callable): Closure
3131
{
3232
return Closure::fromCallable($callable);
3333
}
34+
35+
public static function toBool(string $string): bool
36+
{
37+
return (bool) $string;
38+
}
39+
40+
public static function noReturn()
41+
{
42+
return self::class;
43+
}
44+
45+
/**
46+
* @return null
47+
*/
48+
public static function returnNull()
49+
{
50+
return null;
51+
}
3452
}

‎tests/Fixtures/Type/services.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,9 @@
6767
// gibberish
6868
assertType('null', single_service('bar'));
6969
assertType('null', single_service('timers'));
70+
71+
return [
72+
single_service('toBool'),
73+
service('noReturn'),
74+
service('returnNull'),
75+
];

‎tests/Rules/Functions/ServicesFunctionArgumentTypeRuleTest.php‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,38 +41,51 @@ public function testRule(): void
4141
{
4242
$this->analyse([__DIR__ . '/../../Fixtures/Type/services.php'], [
4343
[
44-
'The name \'createRequest\' is reserved for service location internals and cannot be used as a service name.',
44+
'The method \'createRequest\' is reserved for service location internals and cannot be used as a service method.',
4545
51,
4646
],
4747
[
48-
'The name \'__callStatic\' is reserved for service location internals and cannot be used as a service name.',
48+
'The method \'__callStatic\' is reserved for service location internals and cannot be used as a service method.',
4949
56,
5050
],
5151
[
52-
'The name \'serviceExists\' is reserved for service location internals and cannot be used as a service name.',
52+
'The method \'serviceExists\' is reserved for service location internals and cannot be used as a service method.',
5353
57,
5454
],
5555
[
56-
'The name \'reset\' is reserved for service location internals and cannot be used as a service name.',
56+
'The method \'reset\' is reserved for service location internals and cannot be used as a service method.',
5757
58,
5858
],
5959
[
60-
'The name \'resetSingle\' is reserved for service location internals and cannot be used as a service name.',
60+
'The method \'resetSingle\' is reserved for service location internals and cannot be used as a service method.',
6161
59,
6262
],
6363
[
64-
'The name \'injectMock\' is reserved for service location internals and cannot be used as a service name.',
64+
'The method \'injectMock\' is reserved for service location internals and cannot be used as a service method.',
6565
60,
6666
],
6767
[
68-
'Call to unknown service name \'bar\'.',
68+
'Call to unknown service method \'bar\'.',
6969
68,
70-
'If \'bar\' is a valid service name, you can add its possible service class(es) in <fg=cyan>codeigniter.additionalServices</> in your <fg=yellow>%configurationFile%</>.',
70+
'If \'bar\' is a valid service method, you can add its possible services factory class(es) in <fg=cyan>codeigniter.additionalServices</> in your <fg=yellow>%configurationFile%</>.',
7171
],
7272
[
73-
'Call to unknown service name \'timers\'.',
73+
'Call to unknown service method \'timers\'.',
7474
69,
75-
'If \'timers\' is a valid service name, you can add its possible service class(es) in <fg=cyan>codeigniter.additionalServices</> in your <fg=yellow>%configurationFile%</>.',
75+
'If \'timers\' is a valid service method, you can add its possible services factory class(es) in <fg=cyan>codeigniter.additionalServices</> in your <fg=yellow>%configurationFile%</>.',
76+
],
77+
[
78+
'Service method \'toBool\' expected to return a service instance, got bool instead.',
79+
72,
80+
],
81+
[
82+
'Service method \'noReturn\' returns mixed.',
83+
73,
84+
'Perhaps you forgot to add a return type?',
85+
],
86+
[
87+
'Service method \'returnNull\' expected to return a service instance, got null instead.',
88+
74,
7689
],
7790
]);
7891
}

0 commit comments

Comments
 (0)