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

Return type for integers should be numeric-string #222

Closed
Seldaek opened this issue Feb 1, 2022 · 12 comments · Fixed by #227
Closed

Return type for integers should be numeric-string #222

Seldaek opened this issue Feb 1, 2022 · 12 comments · Fixed by #227
Labels
enhancement New feature or request

Comments

@Seldaek
Copy link
Contributor

Seldaek commented Feb 1, 2022

This code here:

        $result = $this->getEntityManager()->getConnection()->fetchOne('SELECT verified FROM vendor WHERE name = :vendor', ['vendor' => $vendor]);

        return $result === '1';

Triggered:

Strict comparison using === between int<-128, 127> and '1' will always evaluate to false.

At least with doctrine connection like this it appears to really return '1'. I am not sure if there are cases where PDO will return the proper integers, or perhaps with Doctrine when using query builder it may have smarter return values. Would need to all be verified.

@staabm staabm added bug Something isn't working enhancement New feature or request labels Feb 1, 2022
@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 1, 2022

Now the sad thing with numeric-string is we lose the integer ranges :)

@staabm
Copy link
Owner

staabm commented Feb 1, 2022

yeah, I guess this would only work as a big union of all possible values (and this is limited to at max 256 possible unique values by phpstan)

@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 1, 2022

Yeah that or you'd need numeric-string<x,y> syntax support in phpstan. That would be better but not sure it's worth it.

@staabm
Copy link
Owner

staabm commented Feb 1, 2022

per https://stackoverflow.com/a/20123337 it should be possible using regular int etc

@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 1, 2022

Hmm ok.. need to check the real runtime output I get, maybe my code is broken :D

@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 2, 2022

OK confirming I get '1' (string) back from a tinyint(1) column with pdo_mysql (mysqlnd 8.0.13).

Now if I do $this->getEntityManager()->getConnection()->getNativeConnection()->setAttribute(\PDO::ATTR_EMULATE_PREPARES, false) before it does indeed return 1, but I am not sure what other impact there is of doing this. doctrine/dbal#1270 scares me a bit :)

It does seem like PHP 8.1 however returns correct types by default as per php/php-src@c18b1ae - so that's some good news. I'm not sure if the code here should only assume int returns for 8.1, or if it should be an option in the phpstan config (as emulated prepare statements is the default AFAIK, it should probably default to numeric-string on php<8.1 and ints on php8.1+).

@staabm
Copy link
Owner

staabm commented Feb 2, 2022

Thanks for investigating. Thats great news.

We will have a new RuntimeConfiguration switch then, with which you can tell phpstan-dba how types should be handled

https://github.com/staabm/phpstan-dba/blob/main/src/QueryReflection/RuntimeConfiguration.php

@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 2, 2022

Also note that I just noticed this only says PDO MySQL in the PHP changelog. It may not apply to other DBs.

@staabm
Copy link
Owner

staabm commented Feb 2, 2022

php/php-src@c18b1ae also mentions a ATTR_STRINGIFY_FETCHES option.. did you try that? maybe that works without the problems of ATTR_EMULATE_PREPARES

@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 2, 2022

I did try, it said it is not available for mysqlnd.. Maybe in PHP 8.1 it becomes available to revert to the old mode, that's my understanding from changelog anyway.

@staabm
Copy link
Owner

staabm commented Feb 3, 2022

just landed a new stringifyTypes RuntimeConfiguration option:

https://github.com/staabm/phpstan-dba#runtime-configuration

@Seldaek
Copy link
Contributor Author

Seldaek commented Feb 3, 2022

Great thanks, seems to work fine :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants