[Form] fix parsing invalid floating point numbers #24460
Conversation
| } | ||
|
|
||
| return $value; | ||
| if (function_exists('mb_detect_encoding') && false !== $encoding = mb_detect_encoding($value, null, true)) { |
There was a problem hiding this comment.
function_exists should be fully qualified to benefit from PHP 7 compile-time evaluation
| } | ||
|
|
||
| return $value; | ||
| if (function_exists('mb_detect_encoding') && false !== $encoding = mb_detect_encoding($value, null, true)) { | ||
| $length = mb_strlen($value, $encoding); |
There was a problem hiding this comment.
Are you sure about it ? NumberFormatter::parse is documented as setting the offset in the string on php.net. This offset will be based on bytes, not on characters, so you should not be using mb_strlen
There was a problem hiding this comment.
Nope, I just adapted the code from the NumberToLocalizedStringTransformer. Seems this was done on purpose in #7902.
There was a problem hiding this comment.
I looked at the tests for the NumberToLocalizedStringTransformer and copied over some of them. They prove that we indeed need to use the mbstring extension (for example, the exception message in testReverseTransformDisallowsCenteredExtraCharactersMultibyte() would not match otherwise).
| $remainder = trim($remainder, " \t\n\r\0\x0b\xc2\xa0"); | ||
|
|
||
| if ('' !== $remainder) { | ||
| throw new TransformationFailedException( |
There was a problem hiding this comment.
We need tests reaching this new exception
There was a problem hiding this comment.
PercentToLocalizedStringTransformerTest ::testDecimalSeparatorMayNotBeDotIfGroupingSeparatorIsDot() is failing without this change on the 3.4 branch on AppVeyor: https://ci.appveyor.com/project/fabpot/symfony/build/1.0.28085#L1537
There was a problem hiding this comment.
It may be necessary to replace \Locale::setDefault('de_AT'); by \Locale::setDefault('de_DE'); at
See also aa3c66e
There was a problem hiding this comment.
See also comment #19854 (comment)
There was a problem hiding this comment.
Did you see the comment #19854 (comment) ? You have maybe not see the previous link
ICU data was indeed changed for de_AT. It is taken from cldr though, which also has this bit
changed. So it looks like it's a bug in CLDR (unless Austria really got rid of the period as a >grouping separator).
There was a problem hiding this comment.
@aaa2000 Indeed, changing the locale to de_DE fixes the initial issue.
7c42b2f to
efb8db8
Compare
|
Status: Needs Review |
|
|
||
| if ('' !== $remainder) { | ||
| throw new TransformationFailedException( | ||
| sprintf('The number contains unrecognized characters: "%s"', $remainder) |
There was a problem hiding this comment.
exception should be on one line
| $length = mb_strlen($value, $encoding); | ||
| $remainder = mb_substr($value, $position, $length, $encoding); | ||
| } else { | ||
| $length = strlen($value); |
There was a problem hiding this comment.
should be \strlen(PHP-CS-Fixer/PHP-CS-Fixer#3048)
|
Thank you @xabbuh. |
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
Should make AppVeyor builds pass again. Code borrowed from
NumberToLocalizedStringTransformer.