Fix for #6300 - I18n float validator rewrite #6373

moderndeveloperllc · 2014-06-13T19:20:46Z

Fix for #6300. This is a complete rewrite of the validator to account for non-latin number systems, scientific notation, and "look-alike" characters (space for NO-BREAK SPACE, comma for ARABIC DECIMAL SEPARATOR, and single quote for ARABIC THOUSANDS SEPARATOR). Though the test class only tests against 13 locales, the logic was tested against all 709 locales in CLDR 25.

This also includes a small cleanup of the formatting on NumberFormat filter and removal of the extraneous NO-BREAK SPACE removal.

Rewrite of the test class to use programmatic values for true tests using both decimal and scientific notation.

…bers, exponential notation, and a few "look-alike" characters (space for NO_BREAK SPACE, comma for ARABIC DECIMAL SEPARATOR, and single quote for ARABIC THOUSANDS SEPARATOR).

…rmatters; Move to prefix/suffix logic due to locales like mr; Remove LRM from string and symbols

…missing grouping sizes for some locales)

DASPRiD · 2014-06-16T09:29:11Z

tests/ZendTest/I18n/Validator/FloatTest.php

    {
-        $valid = new FloatValidator(array('locale' => 'en_US'));
-        $this->assertEquals($expected, $valid->isValid($float));
+        Locale::setDefault('de');


Make sure to reset the locale to what it was before.

The teardown() for the class resets the locale to whatever it was coming in.

Ah, nevermind then.

DASPRiD · 2014-06-16T14:40:06Z

library/Zend/I18n/Validator/Float.php

+                   '(' . $suffix . '(' . $lnum . $prefix . $exp . '(' . $dnum . '|' . $lnum . '))' . $prefix . '))';
+
+        //LEFT-TO-RIGHT MARK (U+200E) is messing up everything for the handful of locales that have it
+        $lnumSearch     = str_replace("\xE2\x80\x8E", '', '/^' .$prefix . $lnum . $suffix . '$/'.$useUnicode);


Either use ( and ) as regex delimiters instead of /, or add the / as second parameter to all preg_quote() calls.

@DASPRiD umm... this is a str_replace, not preg_quote...

@weierophinney @DASPRiD was referring to lines 169, 178, and 186. I added the slash as suggested. Not sure why his comment still shows. Github quirk.

DASPRiD · 2014-06-16T14:41:39Z

Sadly we have disabled coveralls for pull requests because of the noise. Would you mind exporting a coverage report for the validator? I just want to make sure that this is properly covered.

…e anyways.

moderndeveloperllc · 2014-07-07T02:14:08Z

@DASPRiD gotta admit my ignorance here. What software were you looking for me to use for the coverage report? I can say I tested this with all the locales from ICU 53 and it passed. I know that's not what you meant, but just FYI for the reviewers.

DASPRiD · 2014-07-07T09:08:46Z

@moderndeveloperllc Just run phpunit --coverage-html=/some/path

rahuldroy · 2014-07-08T13:32:43Z

tests/ZendTest/I18n/Validator/FloatTest.php

+    {
+        $trueArray       = array();
+        $testingLocales  = array('ar', 'bn', 'de', 'dz', 'en', 'fr-CH', 'ja', 'ks', 'ml-IN', 'mr', 'my', 'ps', 'ru');
+        $testingExamples = array(1000, -2000, +398.00, 0.04, -0.5, .6, -.70, 8E10, -9.3456E-2, 10.23E6);


I have a few of suggestions that can be added to this test -

High Precision float values such as 123.1234567890987654321

Testing strings e.g. "123.1234567890987654321" (had a problem with this recently not passing the validator)

@rahuldroy Eh, why not. Note: This will pass as decimal because we now only check that the input is in the form of a float and don't try and do any value equivalence checking like the old validator. The old validator was doing some processing of the number and then checking that the value did not change. This ran into the issue you saw - PHP and it's "lax" handling of floats that have a large number of significant digits.

@moderndeveloperllc cool thanks 👍 . You are right about PHPs handling of floats and I believe that is something that needs to be addressed upstream urgently. I was rewriting that part myself (only to find out that you were already working on it)

…idator, but this validator correctly passes it for all locales.

moderndeveloperllc · 2014-07-08T14:37:07Z

@DASPRiD I put up the HTML at https://gist.github.com/moderndeveloperllc/3b63cd47dfd7c576e2bd#file-floatcoverage

It seems to be rather well covered with the exception of a few of the standard exceptions .

ezimuel · 2014-08-06T16:21:19Z

@DASPRiD is this PR ready to be merged?

- Concatenators should be on following line - `||`/`&&` should be on following line - Spaces around `.` concatenation - Trailing whitespace

Forward port #6373

weierophinney · 2014-08-07T16:13:16Z

Cherry-picked to master for release with 2.3.2; merged to both master and develop.

Ocramius · 2014-10-15T20:08:53Z

library/Zend/I18n/Validator/Float.php

        }

-        if (strval($parsedFloat) !== $valueFiltered) {
-            $this->error(self::NOT_FLOAT);


According to @NickBelhomme, this change is causing trouble, as this error message is not being displayed anymore when upgrading to 2.3.3 from any previous version.

@Ocramius Did you mean to put this message on this PR? I know there are a few others out there talking about NOT_FLOAT. Personally, I never saw the utility of the error message as this is a validator - if the method returns false the input is, by definition, not a float. I kept in the error messages that were caused my bad inputs or instantiations. But if y'all want to include errors like this, others are welcome to include them.

@moderndeveloperllc it can be changed, but only as a BC break in a point release

- Concatenators should be on following line - `||`/`&&` should be on following line - Spaces around `.` concatenation - Trailing whitespace

Close zendframework/zendframework#6373 Fixes zendframework/zendframework#6300

Forward port zendframework/zendframework#6373

moderndeveloperllc added 5 commits May 29, 2014 16:53

WIP

18259f5

Fix for #6300 - New validator takes into account non-Latin script num…

dd110e3

…bers, exponential notation, and a few "look-alike" characters (space for NO_BREAK SPACE, comma for ARABIC DECIMAL SEPARATOR, and single quote for ARABIC THOUSANDS SEPARATOR).

Revert NumberFormat filter logic; Deal with decimal vs. scientific fo…

5f20013

…rmatters; Move to prefix/suffix logic due to locales like mr; Remove LRM from string and symbols

Get more data on failed tests

0e25a56

Taking ICU4.x oddities into account (very large exponential symbols, …

fb666d9

…missing grouping sizes for some locales)

DASPRiD reviewed Jun 16, 2014
View reviewed changes

Change to proper preg_quote instead of quotemeta

60bb7d4

DASPRiD reviewed Jun 16, 2014
View reviewed changes

Add second parameter to preg_quote. Not needed, but good code practic…

98e4230

…e anyways.

DASPRiD mentioned this pull request Jul 8, 2014

Possible bug in Float/Double Validation #6404

Closed

rahuldroy reviewed Jul 8, 2014
View reviewed changes

Check a float with lots of significant digits. This broke the old val…

ff31b3e

…idator, but this validator correctly passes it for all locales.

ezimuel added bug labels Aug 6, 2014

ezimuel added this to the 2.3.2 milestone Aug 6, 2014

Ocramius assigned DASPRiD Aug 6, 2014

weierophinney assigned weierophinney and unassigned DASPRiD Aug 7, 2014

weierophinney added a commit that referenced this pull request Aug 7, 2014

[#6373] Reviewed for CS

75e31c5

- Concatenators should be on following line - `||`/`&&` should be on following line - Spaces around `.` concatenation - Trailing whitespace

weierophinney added a commit that referenced this pull request Aug 7, 2014

Merge branch 'hotfix/6373' into develop

f0ed1d2

Forward port #6373

weierophinney closed this in 1aedf8f Aug 7, 2014

moderndeveloperllc deleted the hotfix/6300-i18n-flot-validator branch August 7, 2014 16:25

ThaDafinser mentioned this pull request Sep 25, 2014

add try/catch around statements that execute intl_is_failure() #6598

Closed

Ocramius reviewed Oct 15, 2014
View reviewed changes

weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

Merge branch 'hotfix/6373'

e240ab3

Close zendframework/zendframework#6373 Fixes zendframework/zendframework#6300

weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

Merge branch 'hotfix/6373' into develop

bc6b82e

Forward port zendframework/zendframework#6373

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #6300 - I18n float validator rewrite #6373

Fix for #6300 - I18n float validator rewrite #6373

moderndeveloperllc commented Jun 13, 2014

DASPRiD Jun 16, 2014

moderndeveloperllc Jun 16, 2014

DASPRiD Jun 16, 2014

DASPRiD Jun 16, 2014

weierophinney Aug 7, 2014

moderndeveloperllc Aug 7, 2014

DASPRiD commented Jun 16, 2014

moderndeveloperllc commented Jul 7, 2014

DASPRiD commented Jul 7, 2014

rahuldroy Jul 8, 2014

moderndeveloperllc Jul 8, 2014

rahuldroy Jul 8, 2014

moderndeveloperllc commented Jul 8, 2014

ezimuel commented Aug 6, 2014

weierophinney commented Aug 7, 2014

Ocramius Oct 15, 2014

Ocramius Oct 15, 2014

moderndeveloperllc Oct 15, 2014

Ocramius Oct 15, 2014

Fix for #6300 - I18n float validator rewrite #6373

Fix for #6300 - I18n float validator rewrite #6373

Conversation

moderndeveloperllc commented Jun 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DASPRiD commented Jun 16, 2014

moderndeveloperllc commented Jul 7, 2014

DASPRiD commented Jul 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moderndeveloperllc commented Jul 8, 2014

ezimuel commented Aug 6, 2014

weierophinney commented Aug 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment