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

Invalid field error reported for bigint DBAL type #11073

Closed
emodric opened this issue Nov 20, 2023 · 28 comments
Closed

Invalid field error reported for bigint DBAL type #11073

emodric opened this issue Nov 20, 2023 · 28 comments

Comments

@emodric
Copy link

emodric commented Nov 20, 2023

Bug Report

The field 'App\Entity\Message#id' has the property type 'int' that differs
from the metadata field type 'string' returned by the 'bigint' DBAL type.
Q A
BC Break no
Version 2.17.1

Summary

I have an entity with a bigint primary key, wich has an int typehint in the entity class. I realize from the error message that bigint uses string internally, but so far, there was no issues with using int typehint, especially, since we do not need values larger than 64bit integers.

Current behavior

After upgrading to Doctrine ORM 2.17.1, the above error is shown in logs.

How to reproduce

<entity name="App\Entity\Message">
    <id name="id" type="bigint">
        <generator />
    </id>
</entity>
class Message
{
    private int $id;
}

Expected behavior

No error message is reported.

@greg0ire
Copy link
Member

Expected behavior

No error message is reported.

Why? As you mentioned yourself, bigint uses string internally. Either you should pick another type, or you should write a custom type that wraps the bigint type and returns an int. Note that you might not be aware of performance issues this can cause, see #10944

@emodric
Copy link
Author

emodric commented Nov 20, 2023

What I really wanted to say is that it would be nice to allow usage of bigint with int PHP typehint, allowing up to 64bit size integer values, without having the warning in the logs.

@dmamchyts
Copy link

The same issue after upgrade from

        "doctrine/doctrine-bundle": "^2.11.0",
        "doctrine/orm": "^2.16.2",

to

        "doctrine/doctrine-bundle": "^2.11.1",
        "doctrine/orm": "^2.17.1",

Entity:

#[ORM\Entity(repositoryClass: UserRepository::class)]
#[ORM\HasLifecycleCallbacks]
#[ORM\Table(name: 'users')]
class User extends AbstractEntity implements UserInterface
{
...
    #[ORM\Column(type: Types::BIGINT, nullable: true, unique: true)]
    protected ?int $facebookId = null;
...

error:

	
The field 'App\Common\Entity\User#facebookId' has the
property type 'int' that differs from the metadata field type 'string' returned by the 'bigint' DBAL type.

@greg0ire
Copy link
Member

@emodric @dmamchyts it's unclear to me whether you have read and understood #10944

@emodric
Copy link
Author

emodric commented Nov 21, 2023

Yes, and so far I haven't had issues with performance.

@greg0ire
Copy link
Member

greg0ire commented Nov 21, 2023

Ok, so… if you were starting from scratch, you would not be using bigint then, since you don't plan on using values larger than 64 bit integers, right? This must have to do with a wrong, maybe legacy choice that is hard to change now, correct?

@emodric
Copy link
Author

emodric commented Nov 22, 2023

Not really. Isn't bigint in MySQL used for 64bit integers?

https://dev.mysql.com/doc/refman/8.0/en/integer-types.html

@greg0ire
Copy link
Member

I wrongly thought it would allow integers larger than 64 bits because you stated:

especially, since we do not need values larger than 64bit integers.

Now I fail to understand the logic in your first message. Did you mean to write "since we do not need values larger than PHP_INT_MAX"? On my system, PHP_INT_MAX is smaller than 2**64

@emodric
Copy link
Author

emodric commented Nov 22, 2023

Well, yes, that's true, I meant PHP_INT_MAX, which is on my 64bit system equal to 2**63 -1, but I said 64bit colloquially, without taking into account signness of the integer value.

So, just to be clear, yes, we're using bigint to allow values up to 2**63 - 1, which corresponds to PHP's max integer value on 64bit systems.

@dmamchyts
Copy link

If I understand correctly https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#bigint - in 4.0.0 current issue will be fixed?

Values retrieved from the database are always converted to PHP's integer type if they are within PHP's integer range or string if they aren't. Otherwise, returns null if no data is present.

@greg0ire
Copy link
Member

greg0ire commented Nov 22, 2023

Indeed, I forgot about that. The related PR is doctrine/dbal#6177

Not sure what to do now… I don't think there is an upgrade path for this particular breaking change. I think the right course of action for you is to use string as a type hint for now, and to add a method that casts it to int. And then, to never access the property directly, always go through the method.

Then before migrating to 4.0.0, you change the type to string|int, ignore the warning, and change to int after the migration.

@dmamchyts
Copy link

Looks like I can ignore minor/patch releases (if no any performance issues).
Wait for the release of 4.0.0 and only then update libraries
No any changes in code-base.

Do you have any ETA about the release 4.0.0?

@greg0ire
Copy link
Member

I'd say months if not weeks.

@derrabus
Copy link
Member

derrabus commented Nov 22, 2023

Do you have any ETA about the release 4.0.0?

Soon'ish, but it will require you to upgrade to ORM 3 as well.


I have the same issue on one of my projects as well and my fear is that this error incentivizes people to change all their BIGINT properties to string just to run into the same issue once they upgrade to DBAL 4 one fine day. With a smooth upgrade path in mind, typing BIGINT properties as int should be correct and we should try to make it work in ORM 2 already.

Also, a lot of projects have the schema validation in their CI (which is good!) and now this new check (which is absolutely reasonable!) blocks the ORM upgrade unless the schema validation is either disabled completely or all errors have been fixed.

In the project where I have this problem, we have twenty-something properties that have the infamous DECIMAL + float type mismatch that motivated this new check. Each of them needs an individual solution, so it might take us a while. It would be good, if we could baseline or silence this issue somehow for the time being.

@bobvandevijver
Copy link
Contributor

I have the same issue with float php type combined with decimal in the database: for us it is actually a great reminder that we need to update this anyways to something else, but it would indeed be nice to have some method to silence the issue for the time being.

@greg0ire
Copy link
Member

greg0ire commented Nov 23, 2023

Might be cool to have some kind of baseline concept but for that we would first need to come up with identifiers for each and every issue. Or at least for this one, as a start. Then I guess that system could be developed so that it's possible to baseline issues per entity/property/whatever. Might even be call to use PHP attributes to that end.

class MyEntity
{
    #[SuppressValidationError('invalid_type')]
    private float $myDecimal;
}

Not saying I will build it, but still, might be cool.

@vitxd

This comment was marked as off-topic.

@greg0ire

This comment was marked as off-topic.

@yceruto

This comment was marked as off-topic.

@yceruto

This comment was marked as off-topic.

@vitxd

This comment was marked as off-topic.

@greg0ire

This comment was marked as off-topic.

@vitxd
Copy link

vitxd commented Dec 15, 2023

Is this issue being fixed by the way? I have this problem too as well as the other you've already fixed (thanks!)

@mindaugasvcs
Copy link

Why Doctrine devs did not check PHP_INT_SIZE in Doctrine\DBAL\Types\BigIntType and cast to string for value 4 and cast to int for value 8? :/

@greg0ire
Copy link
Member

greg0ire commented Jan 11, 2024

@vitxd there is #11130 that might be of help

@mindaugasvcs I don't think I 100% understand your question, but having an application behave differently based on PHP_INT_SIZE sounds like a terrible idea.

@mindaugasvcs
Copy link

@vitxd there is #11130 that might be of help @mindaugasvcs I think I 100% understand your question, but having an application behave differently based on PHP_INT_SIZE sounds like a terrible idea.

Of course always casting to a 'string' is much nicer when in fact it should be int as 32-bit systems are dead by now.

@greg0ire
Copy link
Member

greg0ire commented Jan 11, 2024

Ah sorry, I did misunderstand. What you're suggesting is actually more or less what's going to happen with DBAL 4: doctrine/dbal#6177

@greg0ire
Copy link
Member

Anyway, closing as:

  • the error is valid
  • there is a workaround to ignore it until DBAL 4 is out.

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

No branches or pull requests

8 participants