Skip to content
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

PDO: support PDOStatement::setFetchMode() #258

Merged
merged 24 commits into from Mar 2, 2022
Merged

PDO: support PDOStatement::setFetchMode() #258

merged 24 commits into from Mar 2, 2022

Conversation

staabm
Copy link
Owner

@staabm staabm commented Feb 6, 2022

closes #255

requires phpstan-fix phpstan/phpstan#6620

@staabm
Copy link
Owner Author

staabm commented Feb 6, 2022

hmm maybe those errors are related to the multiple-signautres of PDOStatement::setFetchMode.

I could not reproduce the problems on phpstan.org

@xPaw
Copy link
Contributor

xPaw commented Feb 7, 2022

You'll want to add a test for

$stmt->setFetchMode(PDO::FETCH_ASSOC);
$stmt->fetch(PDO::FETCH_NUM);

as I ran into the same issue in my PR.

@staabm
Copy link
Owner Author

staabm commented Feb 7, 2022

good point @xPaw - thx.

@ondrejmirtes do you have an idea/a hint, how a type specifying extension could trigger the signature error, which don't occur without the specifying extension?

grafik

@ondrejmirtes
Copy link

This message says that the type of $stmt will be *NEVER* after this call. It means that you're narrowing the type to something that's not possible. You can try $overwrite=true in the call to TypeSpecifier::create() if it makes sense... https://github.com/phpstan/phpstan-src/blob/35606f9ce1e933d4a226e8277176760ac9bd9162/src/Analyser/TypeSpecifier.php#L945-L952

@staabm
Copy link
Owner Author

staabm commented Feb 9, 2022

This message says that the type of $stmt will be *NEVER* after this call

@ondrejmirtes thx for the hint. I did some debugging locally and I can see that I never specify the type of $methodCall->var to a *NEVER* type.

grafik

I somehow have the feeling that the underlying problem is related to the fact, that setFetchMode is defined with multiple signatures in the resourceMap.

I have no further idea. do you have another tip, or maybe could even have a closer look at it?


the problem can be reproduced when checking out this PR and

composer install
vendor/bin/phpstan analyse -c tests/default/config/phpstan.neon.dist --debug

@ondrejmirtes
Copy link

The type you're specifying it to intersected with the type it already has leads to NeverType.

@staabm
Copy link
Owner Author

staabm commented Feb 9, 2022

The type you're specifying it to intersected with the type it already has leads to NeverType.

even if I use "overwrite=true", as I do?

said differently: the phpunit tests show me that phpstan is picking up my type as I want it to.

only phpstan itself is erroring, which kind of sounds like a false positive then?

@ondrejmirtes
Copy link

Not with overwrite, that would be a different problem. Don't be afraid to fire up Xdebug and see what's going on.

@staabm
Copy link
Owner Author

staabm commented Feb 9, 2022

I guess I tracked it down to:

https://github.com/phpstan/phpstan-src/blob/c5a9c2c50b9b639715ca85541fcfd7f877f5df06/src/Rules/Comparison/ImpossibleCheckTypeHelper.php#L195-L200

where $isSuperType->no() is true and therefore we run into the return false case.
this return false triggers the following case

https://github.com/phpstan/phpstan-src/blob/c5a9c2c50b9b639715ca85541fcfd7f877f5df06/src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php#L57-L66


I think phpstan is wrong as it concludes these 2 GenericObjectTypes of the same class with the same generic types are not an supertype of each other

grafik

@ondrejmirtes
Copy link

Please write a failing test case for that in GenericObjectTypeTest::dataIsSuperTypeOf() and we'll see how much sense it makes.

@staabm
Copy link
Owner Author

staabm commented Feb 11, 2022

just verified the new test works with phpstan@master

@staabm staabm marked this pull request as ready for review February 13, 2022 08:34
@staabm staabm merged commit 6fb6b07 into main Mar 2, 2022
@staabm staabm deleted the fetch-mode branch March 2, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change GenericObjectType generics via TypeSpecifyingExtension Pdo: support setFetchMode()
4 participants