Skip to content

[Auto-Validator] Decimal fields cannot be type-hinted as float #32260

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

Closed
spackmat opened this issue Jun 28, 2019 · 5 comments
Closed

[Auto-Validator] Decimal fields cannot be type-hinted as float #32260

spackmat opened this issue Jun 28, 2019 · 5 comments

Comments

@spackmat
Copy link
Contributor

This is a follow-up-problem related to #32124

Symfony version(s) affected: 4.3.2

Description
After the changes made for #32124 (and updating to 4.3.2), my properties type-hinted as floats throw validation errors about their type, when the automatic validations constraints feature is enabled.

The PropertyInfoLoader automatically adds type constraints based on the internal doctrine type, which is string for decimal fields. So when my getters and setters only accept and return type-hinted float-values, the NumberType-fields for those decimal-properties are always invalid.

I could go through all my entities and make them work with string type-hints (like before they were strict typed), but as they are handling in fact float values from the interface-perspective, I have a strong feeling that the automatic type validation constraint is the wrong part here. That Doctrine handles decimal-fields as strings feels just wrong in a strict-mode type-hinted world, so I type-hinted all my decimal properties with float and casted the values accordingly. Now the form and the auto-validator hunt me.

How to reproduce
Have an entity with a decimal-type property and have getters and setters for it using float-type-hints (with a cast to float in the getter). Then try to save a form with a NumberType for that property: you'll get a "must be string" validation constrain for perfectly valid numeric values.

Possible Solution
I see two ways out:

  1. The PropertyInfoLoader could handle decimal fields, as an exception, in another way. Leaving out the type constraint on those fields at all (as there is no "string or float") or loose it to scalar. Or maybe do some magic and find out the return type of the getter of a property and set the type validator to that type, as it works with the values, the getter returns.
  2. Document a strong warning about typecasting the decimal-properties of Doctrine-entities to float, telling devs that and why this is a bad idea and why this might feel wrong, but is actually the only correct way.
@xabbuh
Copy link
Member

xabbuh commented Jun 28, 2019

#32107 would probably help here

@spackmat
Copy link
Contributor Author

@xabbuh Could be a workaround, when it's released. But I like the Auto-Constraints a lot and want to get the case with the decimals and the type-constraint cleared in one or another way. So far I didn't get why type-hinting/-casting decimals to float is considered a bad practice/idea, maybe I'm simply wrong.

@xabbuh
Copy link
Member

xabbuh commented Jun 28, 2019

Casting the value to float means that you may lose some precision due to the way floating point numbers are handled.

Edit: Casting values also means that Doctrine always considers managed objects to have changed (see the explanation at #30893).

@spackmat
Copy link
Contributor Author

spackmat commented Jul 1, 2019

OK, I see the problem. I refactored all my type hints for decimal properties to string.

That doesn't solve the underlying problem that there is an automatic type constraint conflicting with typecasted values. What if someone casts the decimal properties to a Decimal object (like one from the php-decimal extension)? I won't do that since my application should be portable, but this would be a valid choice and would trigger the same errors.

I think such automatic constraints shouldn't have any possibly unwanted side effects. The Length and the NotNull constraint are mostly safe and it would be a pity to lose them by disabling all automatic constraints on a decimal property. But that belongs into #32107 ;)

@spackmat
Copy link
Contributor Author

spackmat commented Jul 1, 2019

For others stumbling upon this: Another workaround for that particular problem would be to add an explicit Type constraint for the float type, so that the AutoMapper skips adding its own Type constraint.

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

No branches or pull requests

2 participants