Skip to content

Support for Messenger HandleTrait return types #404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jan 4, 2025
Prev Previous commit
Next Next commit
code review fixes
  • Loading branch information
Bartłomiej Nowak committed Dec 23, 2024
commit 89c7534a1377a5fa2f5fa034d2f2c848a886fe7f
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/tests/tmp
/build-cs
/vendor
/.idea
/composer.lock
.phpunit.result.cache
5 changes: 2 additions & 3 deletions src/Symfony/MessageMap.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@
final class MessageMap
{

/** @var array<class-string, Type> */
/** @var array<string, Type> */
private $messageMap;

/** @param array<class-string, Type> $messageMap */
/** @param array<string, Type> $messageMap */
public function __construct(array $messageMap)
{
$this->messageMap = $messageMap;
}

/** @param class-string $class */
public function getTypeForClass(string $class): ?Type
{
return $this->messageMap[$class] ?? null;
Expand Down
49 changes: 32 additions & 17 deletions src/Symfony/MessageMapFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
namespace PHPStan\Symfony;

use PHPStan\Reflection\ClassReflection;
use PHPStan\Reflection\MissingMethodFromReflectionException;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\ShouldNotHappenException;
use Symfony\Component\Messenger\Handler\MessageSubscriberInterface;
Expand Down Expand Up @@ -47,9 +46,14 @@ public function create(): MessageMap
continue;
}

if (!$this->reflectionProvider->hasClass($serviceClass)) {
continue;
}

$reflectionClass = $this->reflectionProvider->getClass($serviceClass);

/** @var array{handles?: class-string, method?: string} $tagAttributes */
$tagAttributes = $tag->getAttributes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a @return stub for this method instead of @var?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shape is dynamic and rely on what tag we're using from SF config. In this case I'm ensuring above that we're handling only messenger.message_handler, so we know what shape it should have.

As far I know stubs are static only, so unfortunately we cannot use them here.

Do you have other idea or it could stay as it is?

$reflectionClass = $this->reflectionProvider->getClass($serviceClass);

if (isset($tagAttributes['handles'])) {
$handles = [$tagAttributes['handles'] => ['method' => $tagAttributes['method'] ?? self::DEFAULT_HANDLER_METHOD]];
Expand All @@ -58,7 +62,13 @@ public function create(): MessageMap
}

foreach ($handles as $messageClassName => $options) {
$methodReflection = $reflectionClass->getNativeMethod($options['method'] ?? self::DEFAULT_HANDLER_METHOD);
$methodName = $options['method'] ?? self::DEFAULT_HANDLER_METHOD;

if (!$reflectionClass->hasNativeMethod($methodName)) {
continue;
}

$methodReflection = $reflectionClass->getNativeMethod($methodName);

foreach ($methodReflection->getVariants() as $variant) {
$returnTypesMap[$messageClassName][] = $variant->getReturnType();
Expand All @@ -79,27 +89,33 @@ public function create(): MessageMap
return new MessageMap($messageMap);
}

/** @return array<class-string, array<string, string>> */
/** @return iterable<string, array<string, string>> */
private function guessHandledMessages(ClassReflection $reflectionClass): iterable
{
if ($reflectionClass->implementsInterface(MessageSubscriberInterface::class)) {
foreach ($reflectionClass->getName()::getHandledMessages() as $index => $value) {
if (self::containOptions($index, $value)) {
yield $index => $value;
} else {
yield $value => ['method' => self::DEFAULT_HANDLER_METHOD];
$className = $reflectionClass->getName();

foreach ($className::getHandledMessages() as $index => $value) {
try {
if (self::containOptions($index, $value)) {
yield $index => $value;
} else {
yield $value => ['method' => self::DEFAULT_HANDLER_METHOD];
}
} catch (ShouldNotHappenException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never catch ShouldNotHappenException.

continue;
}
}

return;
}

try {
$methodReflection = $reflectionClass->getNativeMethod(self::DEFAULT_HANDLER_METHOD);
} catch (MissingMethodFromReflectionException $e) {
if (!$reflectionClass->hasNativeMethod(self::DEFAULT_HANDLER_METHOD)) {
return;
}

$methodReflection = $reflectionClass->getNativeMethod(self::DEFAULT_HANDLER_METHOD);

$variants = $methodReflection->getVariants();
if (count($variants) !== 1) {
return;
Expand All @@ -111,7 +127,6 @@ private function guessHandledMessages(ClassReflection $reflectionClass): iterabl
return;
}

/** @var class-string[] $classNames */
$classNames = $parameters[0]->getType()->getObjectClassNames();

if (count($classNames) !== 1) {
Expand All @@ -124,10 +139,10 @@ private function guessHandledMessages(ClassReflection $reflectionClass): iterabl
/**
* @param mixed $index
* @param mixed $value
* @phpstan-assert-if-true class-string $index
* @phpstan-assert-if-true array<string, mixed> $value
* @phpstan-assert-if-false int $index
* @phpstan-assert-if-false class-string $value
* @phpstan-assert-if-true =class-string $index
* @phpstan-assert-if-true =array<string, mixed> $value
* @phpstan-assert-if-false =int $index
* @phpstan-assert-if-false =class-string $value
*/
private static function containOptions($index, $value): bool
{
Expand Down
16 changes: 8 additions & 8 deletions src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use PHPStan\Symfony\MessageMapFactory;
use PHPStan\Type\ExpressionTypeResolverExtension;
use PHPStan\Type\Type;
use ReflectionException;
use function count;
use function is_null;

Expand All @@ -35,7 +34,6 @@ public function getType(Expr $expr, Scope $scope): ?Type
{
if ($this->isSupported($expr, $scope)) {
$arg = $expr->getArgs()[0]->value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$expr->getArgs()[0] might not exist. Check the existence first.

/** @var class-string[] $argClassNames */
$argClassNames = $scope->getType($arg)->getObjectClassNames();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add this @var.


if (count($argClassNames) === 1) {
Expand All @@ -61,7 +59,7 @@ private function getMessageMap(): MessageMap
}

/**
* @phpstan-assert-if-true MethodCall $expr
* @phpstan-assert-if-true =MethodCall $expr
*/
private function isSupported(Expr $expr, Scope $scope): bool
{
Expand All @@ -73,14 +71,16 @@ private function isSupported(Expr $expr, Scope $scope): bool
return false;
}

try {
$methodReflection = $scope->getClassReflection()->getNativeReflection()->getMethod(self::TRAIT_METHOD_NAME);
$declaringClassReflection = $methodReflection->getBetterReflection()->getDeclaringClass();
$reflectionClass = $scope->getClassReflection()->getNativeReflection();

return $declaringClassReflection->getName() === self::TRAIT_NAME;
} catch (ReflectionException $e) {
if (!$reflectionClass->hasMethod(self::TRAIT_METHOD_NAME)) {
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of catching exception, check if the method exists first.

}

$methodReflection = $reflectionClass->getMethod(self::TRAIT_METHOD_NAME);
$declaringClassReflection = $methodReflection->getBetterReflection()->getDeclaringClass();

return $declaringClassReflection->getName() === self::TRAIT_NAME;
}

}
Loading