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] fix parsing invalid floating point numbers#24460
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
| } | ||
| return$value; | ||
| if (function_exists('mb_detect_encoding') &&false !==$encoding =mb_detect_encoding($value,null,true)) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 usingmb_strlen
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.
Nope, I just adapted the code from theNumberToLocalizedStringTransformer. Seems this was done on purpose in#7902.
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 looked at the tests for theNumberToLocalizedStringTransformer and copied over some of them. They prove that we indeed need to use the mbstring extension (for example, the exception message intestReverseTransformDisallowsCenteredExtraCharactersMultibyte() would not match otherwise).
| $remainder =trim($remainder,"\t\n\r\0\x0b\xc2\xa0"); | ||
| if ('' !==$remainder) { | ||
| thrownewTransformationFailedException( |
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.
We need tests reaching this new exception
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.
PercentToLocalizedStringTransformerTest ::testDecimalSeparatorMayNotBeDotIfGroupingSeparatorIsDot() is failing without this change on the3.4 branch on AppVeyor:https://ci.appveyor.com/project/fabpot/symfony/build/1.0.28085#L1537
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.
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.
See also comment#19854 (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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@aaa2000 Indeed, changing the locale tode_DE fixes the initial issue.
7c42b2f toefb8db8Comparexabbuh commentedOct 7, 2017
Status: Needs Review |
| if ('' !==$remainder) { | ||
| thrownewTransformationFailedException( | ||
| sprintf('The number contains unrecognized characters: "%s"',$remainder) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be\strlen(PHP-CS-Fixer/PHP-CS-Fixer#3048)
fabpot commentedOct 10, 2017
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
Uh oh!
There was an error while loading.Please reload this page.
Should make AppVeyor builds pass again. Code borrowed from
NumberToLocalizedStringTransformer.