Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] forward valid numeric values to transform()#30126
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedFeb 9, 2019
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #30114 |
| License | MIT |
| Doc PR |
xabbuh commentedFeb 9, 2019
@Rubinum@dbrumann@syastrebov I could need your feedback here as you all have been involved in#24036 and#26781. |
| thrownewTransformationFailedException('Expected a numeric.'); | ||
| } | ||
| $value= (string) ($value /$this->divisor); | ||
| $value/=$this->divisor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have tried forcing a rounding issue with and without casting to string and also by reverse transforming either value and I can't find any differences in the behavior. I can't remember why we put the string cast there, but I assume it was just for safety. This change looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh checked the referenced issue and this fix, looks good to me, yes, doesn't seem casting to string was added on specific purpose here, so i think it should be fine.
be61bff toc46cee5Compare...Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...omponent/Form/Tests/Extension/Core/DataTransformer/MoneyToLocalizedStringTransformerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedFeb 13, 2019
Thank you@xabbuh. |
…bbuh)This PR was merged into the 3.4 branch.Discussion----------[Form] forward valid numeric values to transform()| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#30114| License | MIT| Doc PR |Commits-------3be0d35 forward valid numeric values to transform()