Mark Connection::fetch methods as impure

[VincentLanglet]

Q	A
Type	improvement
Fixed issues

Summary

Related to phpstan/phpstan-phpunit#232 @greg0ire
and the doc https://phpstan.org/blog/remembering-and-forgetting-returned-values

By default phpstan consider method with a non void return type as pure. So it's better to declare them as explicitly impure.
This way phpstan doesn't remember the return value.
I think this is important for method which interact with an external database.

There is certainly tons in this lib (and other doctrine ones), I just wanted:

• To validate you're ok with this annotation
• To start somewhere (I'm looking at the more used methods, which might create already issues)
• To share the tips in order to share the annotation work ^^'

[staabm]

@VincentLanglet maybe wait for phpstan/phpstan-src#4422 and utilize @phpstan-all-methods-impure ?

[VincentLanglet]

@VincentLanglet maybe wait for phpstan/phpstan-src#4422 and utilize @phpstan-all-methods-impure ?

I hesitate about it... I'm not sure that all the method are impure in this class for instance quote.
I feel safer to manually add one-by-one phpstan-impure on method

• We are sure they are impure
• People have issue with (like the one gregoire reported)

Also i cannot say @phpstan-all-methods-impure requires

• to be merged (I cannot say when it will happen)
• people to use the latest phpstan version.

[greg0ire]

I think I'm OK with this annotation.

People have issue with (like the one gregoire reported)

It's going to be weird if we have 1000 methods and people have to file 1000 bug reports. Maybe we should add the annotation of all methods of the public API that are impure?

[VincentLanglet]

Maybe we should add the annotation of all methods of the public API that are impure?

Sure, it's just that I'm not that familiar with all the doctrine code base/implementation and it was not obvious to me to know all the impure method (so I took the easy ones). But if someone is motivated to do so, it's better

[morozov]

By default phpstan consider method with a non void return type as pure.

This is very questionable default. I don't think the DBAL (and by extension, any other libraries) should work around that.

I don't think we should be making this change. Especially because there's no way for us to validate the correctness and consistency of these annotations.

[VincentLanglet]

By default phpstan consider method with a non void return type as pure.

This is very questionable default. I don't think the DBAL (and by extension, any other libraries) should work around that.

Because you're working at lot on/with doctrine, I understand you do not agree this default. But it's a useful one to

• remember return values of method
• consider void method as impure

And it gives such behavior https://phpstan.org/r/3e4b3776-2dad-4511-a6d5-4f9eaeb3ea24

It still work well for multiple methods of doctrine. As a random example, if I call $query->getHydrationMode() two times in a row it will gives the same result.

I don't think we should be making this change. Especially because there's no way for us to validate the correctness and consistency of these annotations.

If you consider the default behavior as wrong, you're basically asking to add @phpstan-all-methods-impure on every class of doctrine. This won't be more correct, but your call if your prefer it this way.

Doing nothing seems the worst to me. PHPStan already have to maintain libs to improve the behavior and the PHPDoc of doctrine (https://github.com/phpstan/phpstan-doctrine), I can do the change there but I feel like it would be more benefit on the original repo.

Such change will directly benefit you because you'll be able to stop ignoring error like this one in doctrine codebase (and you're not using phpstan-doctrine):
https://github.com/doctrine/migrations/blob/04681f365819a82df5d718dbc2b68a453fcbffe2/tests/Metadata/Storage/TableMetadataStorageTest.php#L361

[morozov]

Thank you @VincentLanglet, but I don't think this change will provide any value at this point.