mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
PHP 8.1, SQLite: Decimal fields are hydrated as float instead of string #6812
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @derrabus on GitHub (Aug 28, 2021).
Bug Report
Summary
Currently, Doctrine hydrates fields with the
decimaltype to a string. After upgrading to PHP 8.1, I receive a float instead. This change of behavior might break applications, for instance if astringreturn type declaration is used for a getter method of that field.How to reproduce
Execute the test case
Doctrine\Tests\ORM\Functional\TypeTest::testDecimalon PHP 8.1.@derrabus commented on GitHub (Aug 28, 2021):
btw: In order to run the tests with PHP 8.1, I had to switch some dependencies to dev versions:
@derrabus commented on GitHub (Aug 28, 2021):
I've debugged the failing test and it appears that PDO has changed its behavior here.
@derrabus commented on GitHub (Aug 28, 2021):
Possibly realyted: php/php-src@438b025a28
@greg0ire commented on GitHub (Aug 28, 2021):
🤔 should we transfer this issue to the DBAL? That's where
$this->_stmtlives, right?@derrabus commented on GitHub (Aug 28, 2021):
Maybe. But honestly, I'm not sure PDO should behave that way. Casting a
DECIMALto a float might cause a loss of precision and rounding errors. Shall we report this to PHP instead? Maybe @morozov can advise here?@morozov commented on GitHub (Aug 28, 2021):
Reporting upstream sounds right. I only noticed the change in behavior regarding integers when working on https://github.com/doctrine/dbal/pull/4682. But even then, using a large enough unsigned integer (if SQLite supports them) could lead to a loss of precision.
Since SQLite is an embedded engine, in theory, a precision loss may already happen during the write to the "database", so using proper types while fetching the data may not cause any new issues. A test scenario where precision is actually lost might help facilitate the discussion.
@derrabus commented on GitHub (Aug 29, 2021):
Reported as https://bugs.php.net/81397
@Pictor13 commented on GitHub (Aug 30, 2021):
From the bug report:
@derrabus commented on GitHub (Aug 30, 2021):
Yeah. Sounds like we need to deal with it. What would be the proper place to address this?
floatvalues forDECIMALvalues, we could add ais_float()check toDoctrine\DBAL\Types\DecimalType::convertToPHPValue()and runnumber_format()on the value.DECIMALfields.@morozov commented on GitHub (Aug 30, 2021):
Before addressing the issue, let's reproduce it first. If the fact that SQLite stores decimal numbers internally as double causes precision issues, converting them from float to string via
number_format()won't solve the problem.FWIW, there's no consistency in handling numbers between database drivers for PHP, so unless there's a real problem, I'd just let it go.
@derrabus commented on GitHub (Aug 30, 2021):
No, but it solves the problem that users of the ORM expect decimals to be hydrated as strings as it has always been.
@morozov commented on GitHub (Aug 30, 2021):
I'd rather unset this expectation. The ORM cannot be in charge of type conversions like this. For instance, mysqli will return integers as
intorstringdepending on the value (whether it fits intoPHP_INT_MAXor not). It was our deliberate decision in the DBAL to not perform any conversion on top of that because behavior is caused by a limitation of PHP as a language.The ORM users will not start receiving floats instead of decimals all of a sudden, it will only happen after an upgrade to PHP 8.1 which is a deliberate step they will be making, and handling the change should be up to them.
Note, the same will apply to the
INTcolumns. Previously, SQLite users would expect to receive them asstringbut now it's anint. Will the ORM convert them back to string as well?@derrabus commented on GitHub (Aug 30, 2021):
No, DBAL converted those strings to integers for us:
96990667d5/src/Types/IntegerType.php (L32-L35)@morozov commented on GitHub (Aug 30, 2021):
That's a really questionable "feature". See how it will behave handling an
INT UNSIGNED(0…4294967295) on 32-bit PHP:I'd consider this behavior a design flaw on the DBAL side. It also (indirectly) leads to more practical issues than this: https://github.com/doctrine/dbal/issues/4488.
@derrabus commented on GitHub (Aug 31, 2021):
I understand your concern and I know the edge cases such a cast has. When using the ORM however, I would not optimize for the edge case but for usability and type safety. My average ORM app runs in a 64bit environment and does not use
BIGINT UNSIGNED. In that case, an integer cast is what I want. I explicitly use an ORM because I don't want to cast database results myself.@beberlei commented on GitHub (Aug 31, 2021):
Yes type safety over precision is important here since the types values will most likely go into typed properties. However adding check for type is_float to the dbal type is also problematic, these type conversions easily run thousands of times even with just a handful of entities. Decimal isnt a wide spread type, so perf could be less relevant
@morozov commented on GitHub (Aug 31, 2021):
I'd agree for the DBAL/ORM to provide type safety if they rejected knowingly unsupported values.
The APIs of the database extensions are designed with the limitations of PHP as a language in mind. The DBAL/ORM, try to improve this design by just ignoring the limitations at the cost of data integrity. For a database layer, corruption of the data it deals with is unacceptable even in the edge cases.
@derrabus commented on GitHub (Sep 27, 2021):
I've made an attempt to fix the issue on DBAL's side: doctrine/dbal#4818
Performing a string cast is the best solution I could come up with. Alternatively, we could set the attribute
PDO::ATTR_STRINGIFY_FETCHEStotrueon the PDO connection. This would restore the pre-8.1 behavior on SQLite and our tests would pass without doctrine/dbal#4818 agin. However, I'm a bit reluctant to fiddle with global settings on a connection that the app might use for other purposes as well.@derrabus commented on GitHub (Sep 28, 2021):
Fixed via doctrine/dbal#4818