Fix build failures in develop #59
Conversation
| } | ||
|
|
||
| return null; | ||
| $type = $this->reflection->getType(); | ||
| assert($type !== null); |
There was a problem hiding this comment.
it has no sense
if we are checking first hasType() then getType() can't be null
There was a problem hiding this comment.
Guess i was too slow. PHPstan does not infer it correctly. Not sure if an update will change this. Might try this as well-
| $this->assertInstanceOf( | ||
| StdClassFactory::class, | ||
| self::readAttribute($subject, 'factoryInstances')['SomeClass'] ?? null | ||
| ); |
There was a problem hiding this comment.
what is the reason of removing these assertions?
I know that probably these are not the best, as these checks attributes and we really shouldn't do that.
There was a problem hiding this comment.
See my Review comment. The test should not check the internal state and it will stop working with PHPUnit 9
There was a problem hiding this comment.
Yes, I agree it should not. The problem is that there was no any other way to check it, that's why it was done that way (and we have it in many components....).
There was a problem hiding this comment.
I know, and it was me who added them. We could use Reflection directly, but it still feels kinda hacky and bad.
The question is: do we really need this kind of assertion, or could we do better with infection and benchmark testing here (two tools I'd like to be added anyways).
What do we want to test here? A factory class name should be accepted or more? If more: does it really add value?
That's some topic where I'd like to hear @Ocramius opinion, if he could spend a minute
There was a problem hiding this comment.
@michalbundyra I re-added the assertions with reflection, but I still don't think that they add any real value to the test.
| } | ||
|
|
||
| return null; | ||
| $type = $this->reflection->getType(); | ||
| assert($type !== null); |
There was a problem hiding this comment.
PHPstan does not infer that the returned type is not null when hasType() is true. I guess asserting not-null is safe here.
There was a problem hiding this comment.
In my opinion this should be added then to the exceptions in phpstan configuration.
There was a problem hiding this comment.
I'll try a more recent version of phpstan first. Adding ignores is also something I'd like to avoid when possible.
Maybe switching to psalm would also be an option. It feels a bit more reliable in comparison. At least it's type assertions and inference is quite powerful.
There was a problem hiding this comment.
Upgrading PHPStan does not fix it and it will result in a bigger PR.
I reverted the code change and added an exclude to phpstan.neon, but this is awful. I'd like to rewrite this method like this:
public function getType() : ?string
{
$type = $this->reflection->getType();
return $type !== null ? $type->getName() : null;
}What do you think?
| $this->assertSame( | ||
| StdClassFactory::class, | ||
| self::readAttribute($subject, 'factories')['SomeClass'] ?? null | ||
| ); |
There was a problem hiding this comment.
This and the other assertion check the internal state of the class, which is considered bad practice and readAttribute() has been removed from phpunit (see sebastianbergmann/phpunit#3338).
Testing the result of create() should be sufficient.
The Hotfix #56 introduced build failures on develop. This PR targets to fix them.