[Form] forward valid numeric values to transform() #30126
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
xabbuh
commented
Feb 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 |
|
@Rubinum @dbrumann @syastrebov I could need your feedback here as you all have been involved in #24036 and #26781. |
dbrumann
approved these changes
Feb 9, 2019
| @@ -58,7 +64,7 @@ public function transform($value) | |||
| if (!is_numeric($value)) { | |||
| throw new TransformationFailedException('Expected a numeric.'); | |||
| } | |||
| $value = (string) ($value / $this->divisor); | |||
| $value /= $this->divisor; | |||
There was a problem hiding this comment.
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.
@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 to
c46cee5
Compare
OskarStark
reviewed
Feb 9, 2019
...Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php
Outdated
Show resolved
Hide resolved
...omponent/Form/Tests/Extension/Core/DataTransformer/MoneyToLocalizedStringTransformerTest.php
Outdated
Show resolved
Hide resolved
OskarStark
approved these changes
Feb 9, 2019
vudaltsov
approved these changes
Feb 12, 2019
fabpot
approved these changes
Feb 13, 2019
|
Thank you @xabbuh. |
fabpot
added a commit
that referenced
this pull request
Feb 13, 2019
…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()
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.