-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: resolve phpstan errors method.nonObject in tests for Types #7253
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
base: 4.4.x
Are you sure you want to change the base?
feat: resolve phpstan errors method.nonObject in tests for Types #7253
Conversation
| /** @var DateTime $date */ | ||
| $date = $this->type->convertToPHPValue('1985-09-01', $this->platform); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that PHPStan does not understand the type of $this->type. Your annotation is a workaround, not a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which I see meanwhile I fix this, it is src/Types/Type.php is an abstract class.
And the problem using convertToPHPValue in that class, it is that return as mixed, but other clases like tests/Types/TimeTest.php are overriding the abstract return, for example with ?DateTime
In src/Types/VarDateTimeImmutableType.php is ?DateTimeImmutable.
So I think that the problem is that the method should be abtract in src/Types/Type.php and no contain body
That means which src/Types/TypeWithConstructor.php should have also the method to avoid:
Line TypeWithConstructor.php
:10 Non-abstract class Doctrine\DBAL\Tests\Types\TypeWithConstructor contains abstract method convertToPHPValue() from class
Doctrine\DBAL\Types\Type.
I made that changes at 1abb899
So phpstan is a static analyzer, it doesn't execute code or infer types based on which concrete subclass is instantiated at runtime.
In TimeTest.php, $this->type is declared as Type (the base class), even though it's initialized as new TimeType().
When you call $this->type->convertToPHPValue('01:23:34', $this->platform), phpstan sees the return as mixed because that's what the base class declares.
It doesn't "know" that $this->type is actually a TimeType instance that returns ?DateTime.
So I understan which this is a limitation of static analysis: it prioritizes declared types over inferred runtime types to avoid false positives.
And level 9 requires explicit types where ambiguity exists, even if the code is "obviously" correct at runtime.
Other options that I can try:
- Instead of $this->type being Type, declare it as the specific subclass (e.g., protected TimeType $type; in TimeTest). This tells PHPStan the exact type, so the return type is inferred correctly (but reduces polymorphism in tests and it will need separate properties for each type)
- Cast to the expected type: $time = (DateTime) $this->type->convertToPHPValue
- Use assert (already explored and you dont like it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think that the problem is that the method should be abtract in src/Types/Type.php and no contain body
No. A subclass may narrow the return type of a method. This has nothing to do with the parent class/method being abstract or not.
Other options that I can try:
- Make
BaseDateTypeTestCasegeneric. ✌🏻
| { | ||
| return $value; | ||
| } | ||
| abstract public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. We can't do this in a minor release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we use the phpstan annotations for the minor only? and the other solution for a major?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this method needs to be abstract, tbh. And no, I'm not planning major releases for the sake of satisfying a static analyser.
Summary
Related #7248
This splitted pull request makes minor improvements to the type safety and clarity of several unit tests by adding explicit type annotations for variables that hold
DateTimeobjects. These changes help clarify the expected types and can improve static analysis and IDE support.it avoids use of assert or assertInstanceOf
Test improvements (type annotations):
/** @var DateTime $date */annotation before assignments to$dateintestDateResetsNonDatePartsToZeroUnixTimeValuesandtestDateRestsSummerTimeAffectionintests/Types/DateTest.php./** @var DateTime $actual */annotation intestConvertsNonMatchingFormatToPhpValueWithParserintests/Types/DateTimeTest.php./** @var DateTime $time */annotation intestDateFieldResetInPHPValueintests/Types/TimeTest.php.Resolves the phpstan errors: