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

make PDOStatement generic #890

Closed
wants to merge 9 commits into from
Closed

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Dec 27, 2021

as discussed in phpstan/phpstan#6242 I want to build a dynamic return type extension for PDOStatement and therefore need a generic signature.

since I want to add a array-shape RowType, I need a single template for the whole type.
using the same generic template as psalm uses, would not allow me to define the exact type.

I am wondering that psalm defines the Traversable key as int but phpstan as int|string.

I opened another a Issue on psalm to discuss why the stub in psalm looks like that

@staabm
Copy link
Contributor Author

staabm commented Dec 27, 2021

after we have a clear plan on how the signature should look like, I will adjust the tests as needed and I am able to

Comment on lines 5 to 6
* @implements Traversable<int, RowType>
* @implements IteratorAggregate<int, RowType>
Copy link
Contributor Author

@staabm staabm Dec 27, 2021

Choose a reason for hiding this comment

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

as in psalm, I changed the key-type to int as we are effectively working with a list

stubs/PDO.stub Outdated Show resolved Hide resolved
@staabm
Copy link
Contributor Author

staabm commented Dec 28, 2021

it seems I need to define the generic for the static-reflection-case somewhere else.. but can't figure out where ..? no longer relevant


also I am wondering why templates need to be used in parameters .. -> #893 (comment)

grafik

@staabm
Copy link
Contributor Author

staabm commented Dec 28, 2021

just discovered the code for TemplateType validation is pretty new. adjusted the logic to also take return-types into account.. I think we should separate this fixes into a new PR...

@ondrejmirtes
Copy link
Member

Hi, can you please really make your local environment work? It's not very effective to run everything through CI and push 21 commits when you can find the same thing locally running make in a few seconds :)

@staabm
Copy link
Contributor Author

staabm commented Dec 28, 2021

Hi, can you please really make your local environment work? It's not very effective to run everything through CI and push 21 commits when you can find the same thing locally running make in a few seconds :)

getting it properly running on windows is a bit of a problem. the dev env works great on mac but requires some not-out-of-the-box tooling. I will see what I can do.

@ondrejmirtes
Copy link
Member

I know people that have it working on Windows without problems. Even without make you can simply run the commands manually from inside the Makefile.

@clxmstaab clxmstaab force-pushed the pdo-generic branch 2 times, most recently from d9827ea to 3a63c3c Compare December 28, 2021 15:49
@staabm staabm marked this pull request as ready for review December 28, 2021 16:16
@staabm
Copy link
Contributor Author

staabm commented Dec 29, 2021

I got make running in git-bash on windows.

this PR should be good to go.

stubs/PDOStatement.stub Outdated Show resolved Hide resolved
@ondrejmirtes
Copy link
Member

Also - PDOStatement needs to be added here:

skipCheckGenericClasses: []

@clxmstaab clxmstaab force-pushed the pdo-generic branch 3 times, most recently from a4adabb to 77372be Compare December 29, 2021 10:00
@staabm
Copy link
Contributor Author

staabm commented Dec 29, 2021

Also - PDOStatement needs to be added here:

oh great..

this change made some of the test-changes unnecessary. I reverted those.

@@ -4,7 +4,6 @@

final class PDO extends \PDO
{
#[\ReturnTypeWillChange]
Copy link
Contributor Author

@staabm staabm Dec 29, 2021

Choose a reason for hiding this comment

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

deleted this line because it lead to different error lineno beeing reported depending on php version/github action job

clxmstaab added a commit to staabm/phpstan-dba that referenced this pull request Dec 30, 2021
as long as the stubs are not merged in PHPStan upstream, we provide them with the project
- phpstan/phpstan-src#896
- phpstan/phpstan-src#890
* @param array<mixed> $ctorArgs
* @return false|T
*/
public function fetchObject($class = \stdclass::class, array $ctorArgs = array()) {}
Copy link
Member

Choose a reason for hiding this comment

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

Please test that the fetchObject method behaves as expected. I suspect you need to add T of object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test. IMO it works as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect you need to add T of object.

good point. also submitted to psalm with vimeo/psalm#7274

@clxmstaab clxmstaab force-pushed the pdo-generic branch 2 times, most recently from 2e93e67 to bf17420 Compare January 8, 2022 21:31
@staabm
Copy link
Contributor Author

staabm commented Jan 8, 2022

fwiw, this generic is kind of battle tested as I use it while building https://github.com/staabm/phpstan-dba

@staabm
Copy link
Contributor Author

staabm commented Jan 21, 2022

since I have put these stubs in phpstan-dba, I will close this PR.

having them inside the database related repo gives me more flexibility and I think is the best thing long term, as phpstan-src needs to stay more generic and cannot per default be as opinionated as phpstan-dba might needs to be

@staabm staabm closed this Jan 21, 2022
@staabm staabm deleted the pdo-generic branch January 21, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants