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

Fix BIGINT validation #11414

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Apr 15, 2024

Fixes #11377.

Our schema validation currently fails if we map a BIGINT column to a property with an int PHP type. While this is technically correct because DBAL 3 hydrates BIGINT values as strings, the error is also unnecessarily strict:

The ORM will cast strings returned by DBAL back to int, if that's the property type. The value range of BIGINT and PHP's int (on 64bit machines) is the same. Storing BIGINT values as string is only necessary if we run on 32bit PHP or we deal with BIGINT UNSIGNED on MySQL/MariaDB. Both are edge cases a project may validly choose to ignore.

Emitting this error encourages downstream projects to change their property types of BIGINT columns from int to string which is unfortunate given that DBAL 4 moved towards returning integers if possible.

@derrabus derrabus added this to the 2.19.4 milestone Apr 15, 2024
@derrabus derrabus requested a review from greg0ire April 15, 2024 10:38
@derrabus
Copy link
Member Author

This will probably make us revert most of #11399 by @ThomasLandauer when merging up.

@derrabus derrabus merged commit b274893 into doctrine:2.19.x Apr 15, 2024
56 checks passed
@derrabus derrabus deleted the bugfix/validate-bigint branch April 15, 2024 13:11
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2024
* 2.19.x:
  Fix BIGINT validation (doctrine#11414)
  Fix templated phpdoc return type (doctrine#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2024
* 2.19.x:
  Fix BIGINT validation (doctrine#11414)
  Fix templated phpdoc return type (doctrine#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2024
* 2.19.x:
  Fix BIGINT validation (doctrine#11414)
  Fix templated phpdoc return type (doctrine#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2024
* 2.19.x:
  Fix BIGINT validation (doctrine#11414)
  Fix templated phpdoc return type (doctrine#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
derrabus added a commit that referenced this pull request Apr 15, 2024
* 2.19.x:
  Fix BIGINT validation (#11414)
  Fix templated phpdoc return type (#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
derrabus added a commit that referenced this pull request Apr 15, 2024
* 3.1.x:
  Revert "Merge pull request #11399 from ThomasLandauer/issue-11377" (#11415)
  Fix BIGINT validation (#11414)
  docs: update PHP version in doc
  Fix fromMappingArray definition
  Fix templated phpdoc return type (#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  SchemaValidator: Changing mapping of BIGINT to string|int
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
derrabus added a commit that referenced this pull request Apr 15, 2024
* 3.2.x:
  Revert "Merge pull request #11399 from ThomasLandauer/issue-11377" (#11415)
  Fix BIGINT validation (#11414)
  docs: update PHP version in doc
  Fix fromMappingArray definition
  Fix templated phpdoc return type (#11407)
  [Documentation] Merging "Query Result Formats" with "Hydration Modes"
  SchemaValidator: Changing mapping of BIGINT to string|int
  Fix psalm errors: remove override of template type
  Update dql-doctrine-query-language.rst
  Adding `NonUniqueResultException`
  [Documentation] Query Result Formats
@acoulton
Copy link
Contributor

@derrabus @greg0ire I don't think this is valid - at least not for orm 2.x and dbal 3.x.

Relying on PHP to cast the value to an int when assigned to the entity appears to work on the surface. But under the hood, it means that with default change tracking, $entity_manager->flush() will always treat every instance of that entity as changed and issue an extra UPDATE for each one.

This is because

orm/src/UnitOfWork.php

Lines 787 to 790 in b725908

// skip if value haven't changed
if ($orgValue === $actualValue) {
continue;
}
compares the current property on the entity with the original value that came back from dbal using ===. And that value will still be a string, so '1' === 1 fails and the property will be added to the changeset even if the entity has not actually changed.

We were bitten by this in production a while ago and realised that in fact you can only safely use bigint as a php int by registering a custom DBAL type returning an int, so that UnitOfWork is comparing against the correct original value. This is the case for any situation where the entity property typecasts the value returned from the underlying DBAL type.

@greg0ire
Copy link
Member

And that value will still be a string

Are you sure?

DBAL 4 moved towards returning integers if possible.

@acoulton
Copy link
Contributor

@greg0ire with DBAL 3.x it will be, yes - I tested locally.

With DBAL 4.x I suppose it will vary - in theory if you had code running on different platforms then sometimes the data in the UnitOfWork->originalEntityData will be a string and sometimes an int (depending on actual value, it won't be consistent to the platform). So you might sometimes get a false-dirty entity if you have typed the entity to always take int. Although that's probably quite a rare edge case.

@greg0ire
Copy link
Member

I think you're correct. On doctrine/orm 2 we should recommend string and nothing else, I should have caught this. @derrabus should we revert?

@derrabus
Copy link
Member Author

I'll look into it.

@derrabus
Copy link
Member Author

My main problem is this:

Emitting this error encourages downstream projects to change their property types of BIGINT columns from int to string which is unfortunate given that DBAL 4 moved towards returning integers if possible.

I don't want to projects to do big migrations to string properties just to revert back to integers once DBAL 4 is installed. That's why I'd rather not emit an error in this special case. But you are right, with BIGINT mapped to integers we run into the very problem that motivated introducing this validation.

@acoulton
Copy link
Contributor

The safest thing might be to update UnitOfWork to cope with this case and e.g. cast both original & current value back to string internally when it checks to see if a bigint column has changed.

I think that would still detect genuine changes, but avoid any risk of accidental changesets across both DBAL 3&4 and all platforms / possible values?

Similar to how UnitOfWork already has a workaround to cast enums in the entity properties back to strings before comparing to the DBAL value

Off the top of my head I don't think that would break any existing behaviour if it was possible to apply it only to bigint columns?

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

Successfully merging this pull request may close these issues.

3 participants