Change BigIntType Casting #6143

cizordj · 2023-09-03T13:42:18Z

Q	A
Type	improvement
Fixed issues	#6126

Summary

Cast to int within safe range, else to string.

derrabus

Thank you. Please add tests.

src/Types/BigIntType.php

derrabus · 2023-09-03T13:55:06Z

Please also document this change in UPGRADE.md because this is a breaking change.

cizordj · 2023-09-03T15:50:34Z

I'll fix the issues with the phpcs

derrabus

I think this calls for a functional test on MySQL with a BIGINT UNSIGNED field.

derrabus · 2023-09-03T15:50:22Z

src/Types/BigIntType.php

+            ]
+        ];
+
+        $safeInt = filter_var($value, FILTER_VALIDATE_INT, $options);


Let's not use ext-filter here. Your previous code that simply compared the value to PHP_INT_MIN and PHP_INT_MAX was just fine.

When I was creating the unit tests I was getting a strange behavior.

If the value is a string and exceeds the PHP_INT_MAX then it passes the comparison.

See this PHP shell example:

php > var_dump(PHP_INT_MAX); int(9223372036854775807) php > var_dump(('9223372036854775808' <= PHP_INT_MAX)); bool(true)

The problem is the <= operator. If you have a string with an integer lager than PHP_INT_MAX, an integer cast gives you PHP_INT_MAX in PHP.

I'm afraid the ext-filter methods are to slow to put them on a hot execution path. Let's stick to < and >. I realize that this creates an edge case for PHP_INT_MAX itself because we cannot tell apart PHP_INT_MAX from an integer overflow, but I think we can live with that.

derrabus · 2023-09-03T15:54:21Z

tests/Types/BigIntTest.php

+
+    public function testShouldConvertPhpIntMinToInteger(): void
+    {
+        self::assertIsInt($this->type->convertToPHPValue(PHP_INT_MIN, $this->platform));


assertIsInt() is too weak. An implementation that always returns 42 would pass this test. Why not assertSame()? We pretty much know that value to expect.

Ok then, I'll change that.

derrabus · 2023-09-03T15:55:51Z

tests/Types/BigIntTest.php

+
+    public function testShouldConvertSafeNegativeValueToInteger(): void
+    {
+        self::assertIsInt($this->type->convertToPHPValue(PHP_INT_MIN + 1, $this->platform));


This test is bogus. PHP_INT_MIN + 1 is a float and a database would never return a float for a BIGINT field.

No according to PHP shell.

php > var_dump(PHP_INT_MIN + 1); int(-9223372036854775807)

The same goes for the other test

php > var_dump(PHP_INT_MAX - 1); int(9223372036854775806)

The idea is to test scenarios around the limits of PHP.

@derrabus PHP_INT_MIN + 1 is not a float. You confused with PHP_INT_MAX + 1

@stof You're right.

derrabus · 2023-09-03T15:57:02Z

tests/Types/BigIntTest.php

+
+    public function testShouldConvertSafeNegativeStringValueToInteger(): void
+    {
+        self::assertIsInt($this->type->convertToPHPValue((string) (PHP_INT_MIN + 1), $this->platform));


That's also not a valid scenario.

Alright, I'll remove these kinds of tests.

cizordj · 2023-09-03T18:36:51Z

Could you tell me how to run integration tests? When I do this most tests are skipped.

php vendor/bin/phpunit --testdox tests/Functional

greg0ire · 2023-09-05T14:20:53Z

@cizordj by default, tests are run with SQLite. You can add the -vvv flag to see how this impacts skipped tests. You can take a look at phpunit.xml for instructions on how to use another database.

cizordj · 2023-09-08T11:36:01Z

@derrabus Hi, can you please take a look at this test?

https://github.com/doctrine/dbal/blob/2264b78385c6961df9caecce6e9fb56f607317be/tests/Functional/BigIntUnsignedConversionTest.php#L89C5-L98C6

It's failing on my machine. Does that mean we should change our implementation?

stof · 2023-09-08T12:21:01Z

tests/Functional/BigIntUnsignedConversionTest.php

+    protected function setUp(): void
+    {
+        if (
+            ! $this->connection->getDatabasePlatform() instanceof MySQLPlatform


the sqlite platform also support unsigned integer fields

Thanks, I didn't know that 😁

stof · 2023-09-08T12:21:45Z

tests/Functional/BigIntUnsignedConversionTest.php

+
+        $this->connection->executeStatement(
+            <<<'SQL'
+CREATE TABLE IF NOT EXISTS doctrine_tests.mysql_unsigned_test (


This should use the schema API so that it does not hardcode support for mysql only

stof · 2023-09-08T12:23:59Z

docs/en/reference/types.rst

-
-    Not all of the database vendors support unsigned integers, so such an assumption
-    might not be propagated to the database.
+Values retrieved from the database are always converted to PHP's ``integer`` type


it might be worth mentioning that getting a string can only happen on PHP 32-bit architecture or for unsigned integers, as PHP 64-bit has an integer type that corresponds to the range of signed bigint

I agree, I had no idea on how to express this in the documentation.

tests/Functional/BigIntConversionTest.php

- Cast to int within safe range, else to string. - Update the documentation regarding the BigIntType - Add unit tests using assertSame - Add integration test with both signed and unsigned integers - Add the breaking change to the UPGRADE.md file

derrabus · 2023-10-09T09:20:47Z

src/Types/BigIntType.php

+        if ($value === null) {
+            return null;
+        }


Suggested change

if ($value === null) {

return null;

}

if ($value === null || is_int($value)) {

return $value;

}

Let's exit here already if we already hold an integer in our hand. This prevents accidental int-to-string casts. This should also help us getting the tests to pass.

morozov · 2023-10-09T13:33:49Z

Closing in favor of #6177.

@cizordj

| Q | A |------------- | ----------- | Type | improvement | Fixed issues | Replaces doctrine#6143, closes doctrine#6126 #### Summary `BigIntType` casts values retrieved from the database to int if they're inside the integer range of PHP. Previously, those values were always cast to string. This PR continues the work done by @cizordj in doctrine#6143. Co-authored-by: cizordj <[email protected]>

cizordj mentioned this pull request Sep 3, 2023

DBAL36 - BIGINT documentation misunderstanding #6126

Closed

derrabus requested changes Sep 3, 2023

View reviewed changes

src/Types/BigIntType.php Outdated Show resolved Hide resolved

src/Types/BigIntType.php Outdated Show resolved Hide resolved

derrabus added Improvement Types BC Break Type Conversion labels Sep 3, 2023

derrabus requested changes Sep 3, 2023

View reviewed changes

stof reviewed Sep 8, 2023

View reviewed changes

stof suggested changes Sep 11, 2023

View reviewed changes

tests/Functional/BigIntConversionTest.php Outdated Show resolved Hide resolved

cizordj added 2 commits September 11, 2023 21:52

Change BigIntType Casting

21de508

- Cast to int within safe range, else to string. - Update the documentation regarding the BigIntType - Add unit tests using assertSame - Add integration test with both signed and unsigned integers - Add the breaking change to the UPGRADE.md file

Fix broken tests on MySql

abcd922

derrabus reviewed Oct 9, 2023

View reviewed changes

morozov added this to the 4.0.0-RC1 milestone Oct 9, 2023

derrabus mentioned this pull request Oct 9, 2023

Cast BIGINT values to int if possible #6177

Merged

morozov closed this Oct 9, 2023

morozov removed this from the 4.0.0-RC1 milestone Oct 9, 2023

cizordj deleted the cizordj-patch-1 branch October 10, 2023 20:03

github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2024

Uh oh!

Change BigIntType Casting #6143

Change BigIntType Casting #6143

Uh oh!

Conversation

cizordj commented Sep 3, 2023

Summary

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

derrabus commented Sep 3, 2023

Uh oh!

cizordj commented Sep 3, 2023

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cizordj commented Sep 3, 2023

Uh oh!

greg0ire commented Sep 5, 2023

Uh oh!

cizordj commented Sep 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Oct 9, 2023

Uh oh!

Uh oh!