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

[Form] DoctrineOrmTypeGuesser causes error for DecimalType #31905

Closed
ricohumme opened this issue Jun 6, 2019 · 7 comments
Closed

[Form] DoctrineOrmTypeGuesser causes error for DecimalType #31905

ricohumme opened this issue Jun 6, 2019 · 7 comments

Comments

@ricohumme
Copy link

Symfony version(s) affected: 4.3.0

Description
The TypeGuesser (DoctrineOrmTypeGuesser) sets the option input to string which causes the StringToFloatTransformer to take effect. Passing a float from the model to the form causes the transformation to fail

Possible Solution
Remove the option input: string from the TypeGuesser or allow values which are already a float to pass the transformer.
Why this option is added for a DecimalType (as a default) in the first place is a riddle to me.

@xabbuh
Copy link
Member

xabbuh commented Jun 6, 2019

This is expected behaviour. The decimal type maps values to a PHP string (see https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/basic-mapping.html#doctrine-mapping-types).

@ricohumme
Copy link
Author

Expected as it may be regarding the doctrine mapping, the transformer introduces a BC break here. In that case I would expect the transformer to pass when the value enters the transformer as a float.
Wouldn’t you agree?

@xabbuh
Copy link
Member

xabbuh commented Jun 6, 2019

I would first investigate why your data is a float in the first place as it's probably even not guaranteed that it will work with Doctrine in all cases.

As a quick workaround you can probably also add the input option of your form type to number explicitly to not rely on the guessing of the option.

@ricohumme
Copy link
Author

Well, to the why it is a float is simple, the model says it is a float and the return type of the method as well as the declaration of the property is configured that way.

I have in fact implemented the field with the number type in the form explicitly now so the pressure is off at the moment.
However, this is the case for one of our projects. We use a generic form which relies on the guesser to pass correct information and in this case an assumption is made which is false imo.

But I’m interested to hear what others think.

@ricohumme
Copy link
Author

ricohumme commented Jun 7, 2019

After discussing this with my colleague I tried to alter the type of my model from Decimal to Float and the following came to light:
Previous:
Model:
@ORM\Column(type="decimal", precision=9, scale=2, nullable=true)
Database:
Datatype: DECIMAL(9,2)

After:
Model:
@ORM\Column(type="float", precision=9, scale=2, nullable=true)
Database:
Datatype: DOUBLE

As long as the precision and scale are defined, the form enforces the values to meet these demands, however on database level these constraints are no longer present.
I have yet to test what would happen if the value is updated through the model without a form. Updating the value directly in the database shows there is no restriction for decimals.

@xabbuh
Copy link
Member

xabbuh commented Jun 20, 2019

See #32125 where we try to relax the data transformer. Does that help here?

@ricohumme
Copy link
Author

I think this will do the trick. Let me test this tomorrow

@fabpot fabpot closed this as completed Jun 22, 2019
fabpot added a commit that referenced this issue Jun 22, 2019
…buh)

This PR was merged into the 4.3 branch.

Discussion
----------

[Form] accept floats for input="string" in NumberType

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31905, #32124
| License       | MIT
| Doc PR        |

Commits
-------

2abf855 accept floats for input="string" in NumberType
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

4 participants