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] Fixed PercentToLocalizedStringTransformer to accept both comma and dot as decimal separator, if possible#22586
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
| } | ||
| $formatter =$this->getNumberFormatter(); | ||
| $groupSep =$formatter->getSymbol(\NumberFormatter::GROUPING_SEPARATOR_SYMBOL); |
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 duplicate code ofNumberToLocalizedStringTransformer.php maybe it is better to use it directly ?
- if (!is_string($value)) {- throw new TransformationFailedException('Expected a string.');- }- if ('' === $value) { return; } $formatter = $this->getNumberFormatter();- $groupSep = $formatter->getSymbol(\NumberFormatter::GROUPING_SEPARATOR_SYMBOL);- $decSep = $formatter->getSymbol(\NumberFormatter::DECIMAL_SEPARATOR_SYMBOL); $grouping = $formatter->getAttribute(\NumberFormatter::GROUPING_USED);-- if ('.' !== $decSep && (!$grouping || '.' !== $groupSep)) {- $value = str_replace('.', $decSep, $value);- }-- if (',' !== $decSep && (!$grouping || ',' !== $groupSep)) {- $value = str_replace(',', $decSep, $value);- }-- // replace normal spaces so that the formatter can read them- $value = $formatter->parse(str_replace(' ', ' ', $value));-- if (intl_is_failure($formatter->getErrorCode())) {- throw new TransformationFailedException($formatter->getErrorMessage());- }+ $transformer = new NumberToLocalizedStringTransformer($this->scale, (bool) $grouping);+ $value = $transformer->reverseTransform($value);| $decSep =$formatter->getSymbol(\NumberFormatter::DECIMAL_SEPARATOR_SYMBOL); | ||
| $grouping =$formatter->getAttribute(\NumberFormatter::GROUPING_USED); | ||
| if ('.' !==$decSep && (!$grouping ||'.' !==$groupSep)) { |
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.
Wouldn't be easier to first remove the grouping character and then replace$decSep with a dot?
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 prefer to use the same logic asNumberToLocalizedStringTransformer but if you want, I can try
nicolas-grekas left a comment
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.
LGTM
@xabbuh would you be +1 even if we ignore your comment?
| * "choice_value" | ||
| * "choice_attr" | ||
| * "group_by" | ||
| * added PercentToLocalizedStringTransformer to accept both comma and dot as decimal separator, if possible |
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 think we don't need this change, we don't put entries there for bug fixes
| publicfunctiontestDecimalSeparatorMayBeDotIfGroupingSeparatorIsNotDot() | ||
| { | ||
| \Locale::setDefault('fr'); |
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.
tests are failing because this method is not always available, can you fix it@aaa2000 ?
… and dot as decimal separator, if possible
aaa2000 commentedJul 29, 2017
Tests fixed and revert the change in changelog. |
fabpot commentedOct 1, 2017
@xabbuh@nicolas-grekas Any decision on this one? |
fabpot commentedOct 1, 2017
Thank you@aaa2000. |
… both comma and dot as decimal separator, if possible (aaa2000)This PR was squashed before being merged into the 2.7 branch (closes#22586).Discussion----------[Form] Fixed PercentToLocalizedStringTransformer to accept both comma and dot as decimal separator, if possible| Q | A| ------------- | ---| Branch? | 2.7 <!-- see comment below -->| Bug fix? | yes-ish| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | <!-- #-prefixed issue number(s), if any -->| License | MIT<!--- Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->Implements the same behaviour that `NumberToLocalizedStringTransformer` in order to accept both comma and dot implemented in#5941Commits-------f96a7f8 [Form] Fixed PercentToLocalizedStringTransformer to accept both comma and dot as decimal separator, if possible
This PR was merged into the 2.7 branch.Discussion----------[Form] fix parsing invalid floating point numbers| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19854,#22586| License | MIT| Doc PR |Should make AppVeyor builds pass again. Code borrowed from `NumberToLocalizedStringTransformer`.Commits-------042eac4 [Form] fix parsing invalid floating point numbers
Uh oh!
There was an error while loading.Please reload this page.
Implements the same behaviour that
NumberToLocalizedStringTransformerin order to accept both comma and dot implemented in#5941