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

Cast BIGINT values to int if possible #6177

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Oct 9, 2023

Q A
Type improvement
Fixed issues Replaces #6143, closes #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 #6143.

tests/Functional/Types/BigIntTypeTest.php Outdated Show resolved Hide resolved
tests/Functional/Types/BigIntTypeTest.php Outdated Show resolved Hide resolved
tests/Functional/Types/BigIntTypeTest.php Outdated Show resolved Hide resolved
src/Types/BigIntType.php Outdated Show resolved Hide resolved
@derrabus derrabus force-pushed the improvement/bigint-cast branch 2 times, most recently from 65896da to 62cecdb Compare October 9, 2023 14:08
@derrabus derrabus force-pushed the improvement/bigint-cast branch from 62cecdb to c04c987 Compare October 9, 2023 14:11
@derrabus derrabus linked an issue Oct 9, 2023 that may be closed by this pull request
@derrabus derrabus requested a review from greg0ire October 9, 2023 14:20
@derrabus derrabus merged commit 16c850f into doctrine:4.0.x Oct 9, 2023
66 checks passed
@derrabus derrabus deleted the improvement/bigint-cast branch October 9, 2023 14:37
@cizordj
Copy link
Contributor

cizordj commented Oct 10, 2023

Nice to see this feature implemented 😄.
Thanks you all

@ondrejmirtes
Copy link
Contributor

I'm not a big fan of this change. This now means that bigint properties now need to be declared as int|string and it brings additional complexity to the code.

@derrabus
Copy link
Member Author

derrabus commented Nov 8, 2023

This now means that bigint properties now need to be declared as int|string

No, you can declare them as int in most cases. The string edge-case is rare enough that you can decide to ignore it in your project.

@ondrejmirtes
Copy link
Contributor

Errr, what? Once we put a large enough integer value to the database, we need to declare string too. Of course from the point of static analysis, only code that declares int|string is safe.

@derrabus
Copy link
Member Author

derrabus commented Nov 8, 2023

Errr, what? Once we put a large enough integer value to the database, we need to declare string too.

Which is only possible if:

  • Your PHP is on 32bit
  • You use BIGINT UNSIGNED on MySQL

In both cases, you deliberately decided to deal with integers that are larger than what your PHP setup can handle, so the "added complexity" you're talking about is justified.

And if that int|string union is too much complexity for your taste, you can still declare your property as string and the ORM will cast the integer back to string. You basically opt-out of the new behavior that you don't like. Everybody wins. 🙂

@ondrejmirtes
Copy link
Contributor

Alright, thank you, hopefully this can be accommodated for in phpstan-doctrine correctly :)

@cizordj
Copy link
Contributor

cizordj commented Nov 8, 2023

@ondrejmirtes by default, BigInts are signed 8-byte numbers that work well with PHP's integer type. However, if you're hosting your project on a 32-bit system, the PHP integer range is smaller, and larger numbers need to be converted to strings to function properly. The same goes for unsigned integers, as their range exceeds what PHP can handle, leading to the need to cast them as strings due to memory limitations. If your database uses signed integers, sticking to the integer type in PHP is just fine.

@derrabus
Copy link
Member Author

derrabus commented Nov 8, 2023

Alright, thank you, hopefully this can be accommodated for in phpstan-doctrine correctly :)

💯 I have full confidence in the abilities of the authors of said package. 🚀

@ondrejmirtes
Copy link
Contributor

Just one more question about this - if the mapped property contains a string it's still okay and Doctrine will write it correctly o the BIGINT column in the database, right?

@derrabus
Copy link
Member Author

derrabus commented Feb 9, 2024

Sure. It should be the string representation of an integer, obviously.

@ondrejmirtes
Copy link
Contributor

Thanks :) So int|string is a valid type declaration for property mapped as bigint :)

derrabus pushed a commit that referenced this pull request Oct 25, 2024
<!-- Fill in the relevant information below to help triage your pull
request. -->

|      Q       |   A
|------------- | -----------
| Type         | bug
| Fixed issues | n/a

#### Summary

Resolve
#6177 (comment)
discussion and related original #6177.

Whole native php `int` range is guaranteed to be supported per
https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/types.html#bigint
docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DBAL36 - BIGINT documentation misunderstanding
5 participants