search

staabm / phpstan-dba

PHPStan based SQL static analysis and type inference for the database access layer
https://staabm.github.io/archive.html#phpstan-dba
MIT License
255 stars 17 forks source link

Failing AST test with non-named column #570

Closed jakubvojacek closed 1 year ago

jakubvojacek commented 1 year ago

Hello @staabm

this is related to https://github.com/staabm/phpstan-dba/issues/566#issuecomment-1468512616, this is the reproducing case. When count(*) (or other functions) is used without alias, it fails to narrow down the type for integer indexed key in the resulting PDOStatement.

I tried playing around with ParserInference but couldn't find a easy way to fix this yet

staabm commented 1 year ago

to implement this I think we need https://github.com/SQLFTW/sqlftw/commit/da4e0354e6d48dffb94a04a316b1b08b6a2ca38e which will be part of the next sqlftw release, since it looks pretty similar to what I had in mind when reporting https://github.com/SQLFTW/sqlftw/issues/15

jakubvojacek commented 1 year ago

perfect, cant wait for this

staabm commented 1 year ago

Today a new sqlftw was released. You can give it a try and update the lob with this PR and add a fix for you problem if you like

jakubvojacek commented 1 year ago

I will give it a go over the weekend! Thanks

jakubvojacek commented 1 year ago

The fix for the correct type inference was easy, but tests are failing @staabm

We got some problems with postgres tests, since AST does not work there, it fails (https://github.com/staabm/phpstan-dba/actions/runs/4454918527/jobs/7824413966?pr=570)

For the same reason, it also fails for php 7.3 and lower

Any idea how to overcome this, please?

staabm commented 1 year ago

I think we should move ast tests, e.g.

https://github.com/staabm/phpstan-dba/blob/a1bb0c6e77f514528471e87d785084c21868b923/tests/default/DbaInferenceTest.php#L66-L68

into a separate test-class and enable the runtime-config for SQL AST only for this new test-class

make sure to re-base this PR as in parallel another PR was merged

jakubvojacek commented 1 year ago

Is this what you had in mind please?