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

DBAL36 - BIGINT documentation misunderstanding #6126

Closed
cizordj opened this issue Aug 10, 2023 · 17 comments · Fixed by #6177
Closed

DBAL36 - BIGINT documentation misunderstanding #6126

cizordj opened this issue Aug 10, 2023 · 17 comments · Fixed by #6177

Comments

@cizordj
Copy link
Contributor

cizordj commented Aug 10, 2023

Bug Report

Q A
Version 3.6.5

Summary

Please rephrase the explanation in the docs about the BIGINT datatype.

Current behaviour

The current statements lead to misinterpretation:

<!-- dbal/docs/en/reference/types.rst:90 -->
Values retrieved from the database are always converted to PHP's ``string`` type
or ``null`` if no data is present.
<!-- dbal/docs/en/reference/types.rst:95 -->
For compatibility reasons this type is not converted to an integer

Which is not true.

We have many entities mapped with BIGINT and BIGSERIAL in a PostgreSQL database
and doctrine always casts their value to a regular PHP integer. We use the 64 bit architecture
and the primary keys are mapped this way.

<?php
class User {
    #[ORM\Id]
    #[ORM\GeneratedValue(strategy: 'SEQUENCE')]
    #[ORM\SequenceGenerator(
        sequenceName: 'user.user_id_user_seq',
        allocationSize: 1,
        initialValue: 1
    )]
    #[ORM\Column(name: 'id_user', type: Types::BIGINT)]
    private ?int $id = null;
}

According to the PostgreSQL's documentation the maximum number for a BIGINT field is 9223372036854775807 which is the same as the PHP_INT_MAX.

How to reproduce

  • Make an entity whose primary key represents a BIGINT type in the database.
  • Run the respective migration
  • Add a row to the table via a SQL script
  • Fetch that entity using that entity's repository via Symfony Commands or Regular Controllers
  • Debug the datatype via dd() or let it serialize it through a JsonResponse.

You'll notice that the value comes as a PHP integer.

You can also add another row to the table with the maximum possible value, see the value of PHP_INT_MAX and make an insertion to the database.
Fetch all the entities and you can see that the value still comes as a regular integer type.

Due to the limitations of JSON, things start to get really weird for numbers like this, so you can only trust the dd() output in this case.

Expected behaviour

The docs should say that the values retrieved from the database are always converted to PHP's int type or null if no data is present and display a notice about it being cast to string only in legacy 32-bit platforms.

Btw why have you guys changed your minds? #2976

@derrabus
Copy link
Member

In your example, the ID is cast to an integer because your class uses int as a native property type. The documentation is correct, see:

return $value === null ? null : (string) $value;

But given that 32bit systems are rather rare, we could change this behavior in 4.0. Up for a PR?

@derrabus
Copy link
Member

According to the PostgreSQL's documentation the maximum number for a BIGINT field is 9223372036854775807 which is the same as the PHP_INT_MAX.

Note that MySQL's BIGINT UNSIGNED can still exceed PHP_INT_MAX.

@cizordj
Copy link
Contributor Author

cizordj commented Aug 14, 2023

But given that 32bit systems are rather rare, we could change this behavior in 4.0. Up for a PR?

Yes, but what about the UNSIGNED integers?

@derrabus
Copy link
Member

derrabus commented Aug 15, 2023

But given that 32bit systems are rather rare, we could change this behavior in 4.0. Up for a PR?

Yes, but what about the UNSIGNED integers?

I would simply remove the string cast and return the value as we receive it from the driver. This is how we've always done it for 32bit integers.

edit: I was wrong. We can compare the value returned from the database with PHP_INT_MAX and perform the int cast only when it's safe to do so. Otherwise, we return the value as is.

@cizordj
Copy link
Contributor Author

cizordj commented Aug 16, 2023

So we'll be basically returning two types depending on the database, right? integer when it's bigint signed and float when it's bigint unsigned. Doesn't that make the behavior unpredictable? How about we create another dedicated type for unsigned big integers?
I guess we shouldn't support non-standard SQL types, right? EDIT: I mean the unsigned big integers

@derrabus
Copy link
Member

float when it's bigint unsigned

No, we never cast integers to float.

@cizordj
Copy link
Contributor Author

cizordj commented Sep 3, 2023

Not you but PHP does.

php > var_dump((int) PHP_INT_MAX + 1);
float(9.223372036854776E+18)

@cizordj
Copy link
Contributor Author

cizordj commented Sep 3, 2023

In my opinion it got a little confused.

pull-6143

The same class is returning two distinct types but it's saying that it returns Integer in the getBindingType.

Is this is a case for a new class?

@derrabus
Copy link
Member

derrabus commented Sep 3, 2023

Not you but PHP does.

php > var_dump((int) PHP_INT_MAX + 1);
float(9.223372036854776E+18)

This is not a type-cast. It's a sum operation with a float result. I don't see how this relates to my statement or the issue we're discussing here.

@cizordj
Copy link
Contributor Author

cizordj commented Sep 3, 2023

Alright, no problem.

@stof
Copy link
Member

stof commented Sep 12, 2023

@cizordj the binding type is not about the type of the PHP representation. It is about the way the value should be interpreted in query parameters (so that the database knows how to cast it when executing a prepared statement)

@derrabus derrabus linked a pull request Oct 9, 2023 that will close this issue
derrabus added a commit to derrabus/dbal that referenced this issue Oct 9, 2023
|      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]>
@ondrejmirtes

This comment was marked as duplicate.

@derrabus
Copy link
Member

derrabus commented Nov 8, 2023

I think, we can close this issue. @ondrejmirtes, I've answered your comment on #6177 already. Let's keep the discussion in one place.

@derrabus derrabus closed this as completed Nov 8, 2023
@cizordj
Copy link
Contributor Author

cizordj commented Nov 8, 2023

@ondrejmirtes if you know for sure your integer is signed then you don't need to declare as string|int

@ondrejmirtes
Copy link
Contributor

@cizordj Can you tell me the conditions for which Orm\Column with bigint type can only ever be int, and when it can also be string?

@cizordj
Copy link
Contributor Author

cizordj commented Nov 8, 2023

I'll answer you there #6177

Copy link

github-actions bot commented Dec 9, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants