Skip to content

Commit

Permalink
Treat float numbers with trailing zeroes removed by PHP, accordingly.
Browse files Browse the repository at this point in the history 
Revert #2800 (28bd688) regex changes. Also add more tests.

Signed-off-by: mark_story <mark@mark-story.com>
  • Loading branch information
@bar @markstory
bar authored and markstory committed Aug 25, 2012
1 parent f06ce13 commit b75a6b4
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
10 changes: 7 additions & 3 deletions lib/Cake/Test/Case/Utility/ValidationTest.php
View file
Expand Up @@ -1498,16 +1498,20 @@ public function testDecimal() {
$this->assertTrue(Validation::decimal('+0123.45e6'));
$this->assertTrue(Validation::decimal('-0123.45e6'));
$this->assertTrue(Validation::decimal('0123.45e6'));
$this->assertTrue(Validation::decimal('1234'));

This comment has been minimized.

Sign in to view

Copy link
@renan

renan Aug 25, 2012

Contributor

Its not a decimal anymore?

This comment has been minimized.

Sign in to view

Copy link
@ADmad

ADmad Aug 25, 2012

Member

This commit will break existing usages. Only the fix where empty string was incorrectly validated as decimal should have been done.

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 25, 2012

Member

These tests were added as part of #2800. I'm not entirely sure that change should have ever been done, I guess its reasonable to not require trailing decimal points, the discussion around 28bd688 indicated that people felt otherwise.

This comment has been minimized.

Sign in to view

Copy link
@ceeram

ceeram Aug 25, 2012

Contributor

How about using $places === true, to only validate when there is a decimal point, else validate round numbers as well as true?

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 25, 2012

Member

I'd rather avoid more parameters, also I think requiring a decimal place is not totally unreasonable, as we have numeric() for situations where you want more permissive validation.

This comment has been minimized.

Sign in to view

Copy link
@ceeram

ceeram Aug 25, 2012

Contributor

decimal() already has this param

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 26, 2012

Member

So it does, my mistake. So does the following sound ok for $places behavior:

  • null = Any number of decimal places including 0, the . is not required.
  • true = Any number of decimal places including 0, the . is required.
  • 1..N = Exactly that many number of decimal places, the . is required.

This comment has been minimized.

Sign in to view

Copy link
@ceeram

ceeram via email Aug 26, 2012

Contributor

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 27, 2012

Member

@bar @milesj Any thoughts on the proposed changes from @ceeram and myself?

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 27, 2012

Author Contributor

Sorry, the notifications never arrived until now. I like the whole idea, and the null => . not required switch, but I have two concerns:

  • I think people expects . to be checked as default when calling decimal() so maybe $places = true should be the default in that case.
  • Will this be used? I mean, a person trying to validate a decimal will think about validating a ., but a person trying to validate a number without a . will use numeric() or even naturalNumber(); will they think about decimal()?¿ cause the [-+] will be the only benefit in this case.

Again, I can do that if you are all OK :)

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 27, 2012

Member

I'm thinking of the use case where you're validating user input in amount fields say on an shopping cart or an invoice application. It annoying that 10 will fail validation while 10.0 and 10. will pass, this doesn't seem like the most useful default behavior to have.

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 27, 2012

Author Contributor

Yup, but I think users validating $ knowing they use rounded numbers won't validate with decimal(). And people validating $ with decimal() will do that because they are sure they need to test for a decimal separator (with or without $places), so they are already aware of that and should be able to, casting to float (server side), or even toying with JS (client side) and PHP will convert it to a float-like string later. Or with $places = null (and/or false) :)

Ohh, and 10. won't validate with the commited code, should it?

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 27, 2012

Member

It depends. Making/Leaving 10. as an invalid case means that the list of values for valid and their meanings are as follows:

  • null = Any number of decimal places including none the . is not required.
  • true = Any number of decimal places greater than 0, the . is required.
  • 1..N = Exactly that many number of decimal places, the . is required.

Which creates a stricter mode which more people seem to be in favour of.

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 27, 2012

Author Contributor

I like that, 10. does not seem to be a correctly formatted number to me, 10 and 10.0 does.

This comment has been minimized.

Sign in to view

Copy link
@ADmad

ADmad Aug 27, 2012

Member

Yup, number like 10. shouldn't be allowed. Decimal point should be followed by a digit.

This comment has been minimized.

Sign in to view

Copy link
@CauanCabral

CauanCabral Aug 27, 2012

Contributor

I continue disagreeing with "10" isn't a valid default decimal number.
By definition it is a decimal value. Isn't because we have another validation rule we can change that fact.

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 27, 2012

Author Contributor

@CauanCabral albeit being or not a decimal, the funcion name is a just a hint for the user, it does not have to be strictly what Wikipedia states, but a help to the people using it.

This comment has been minimized.

Sign in to view

Copy link
@CauanCabral

CauanCabral Aug 27, 2012

Contributor

@bar It's not a Wikipedia definition, it's a mathematical truth. Wiki is just a reference link.
If not follow definitions, documentation should explicit which "decimal" in the rule name is just an alias, not the real name.

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 27, 2012

Author Contributor

Yes, I know, I just took the Wiki as an example :P
Documentation actually says:

If no decimal point is found a false will be returned.

Again, if you validate 12.0, or even (float) 12 it will work, (int) 12 should not validate, and the way PHP works, and considering user input (strings), it is hard to test all of this.
I think the way we found is pretty neat.

This comment has been minimized.

Sign in to view

Copy link
@ceeram

ceeram Aug 27, 2012

Contributor

@CauanCabral Lets forget about the exact definition and keep in mind what/how users want to validate mostly end-user input.

The proposed solution offers support for both 10 as valid or invalid depending on $places = null or $places = true
Imo it should default to null, thus allowing 10 as valid decimal, if you want to force the decimal dot, use true

This comment has been minimized.

Sign in to view

Copy link
@CauanCabral

CauanCabral Aug 27, 2012

Contributor

@bar some decimal can't be expressed as float number, so, sometimes typecasting isn't possible.

@ceeram maybe its just me, but that is exactly what I want validate. If places default is to null, thats fine for me. Otherwise, I will disagree, but ok too, consensus is most important.

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 27, 2012

Author Contributor

@CauanCabral for example?

This comment has been minimized.

Sign in to view

Copy link
@bar

bar Aug 28, 2012

Author Contributor

The working ALGO #800

  • It defaults to $places = null but IMO it should to $places = true (the stricter form it historically had).
  • '10.' does not validate but as PHP treats numbers 10.0 and 10. the same, they are valid and impossible to differentiate later in the parsing, so (after reading a lot, e.g. Formal patterns from http://www.php.net/manual/en/language.types.float.php), I came to the conclusion that maybe it should be valid and be consistent to @markstory 's first draft.
  • Added a minor modification to Validator::_check() so that only strings can be used as $pattern for preg_match(). And when null regexs are used false is returned and Validator::$error is updated.

This comment has been minimized.

Sign in to view

Copy link
@CauanCabral

CauanCabral Aug 28, 2012

Contributor

@bar representation isn't possible in binary notation, so, for validation objective isn't relevant. My fault.
PHP store values as char* keeping a non-representable float number 0.1 as '0.1'. But if after cast and validate you need use that number in math operations, you will lost accuracy.

Anyway, I did see your PR and totally agree with implementation.

This comment has been minimized.

Sign in to view

Copy link
@markstory

markstory Aug 28, 2012

Member

Ok if the issues around the PR#800 are good I'll get it merged in :)
$this->assertTrue(Validation::decimal('-1234'));
$this->assertTrue(Validation::decimal('+1234'));
$this->assertTrue(Validation::decimal(1234.56));
$this->assertTrue(Validation::decimal(1234.00));
$this->assertTrue(Validation::decimal('1234.00'));
$this->assertTrue(Validation::decimal(.0));
$this->assertTrue(Validation::decimal(.00));
$this->assertTrue(Validation::decimal('.00'));
$this->assertTrue(Validation::decimal(.01));
$this->assertTrue(Validation::decimal('.01'));

$this->assertFalse(Validation::decimal(''));
$this->assertFalse(Validation::decimal('string'));
$this->assertFalse(Validation::decimal('1234'));
$this->assertFalse(Validation::decimal('-1234'));
$this->assertFalse(Validation::decimal('+1234'));
}

/**
Expand Down
7 changes: 5 additions & 2 deletions lib/Cake/Utility/Validation.php
View file
Expand Up @@ -381,11 +381,14 @@ public static function boolean($check) {
* @return boolean Success
*/
public static function decimal($check, $places = null, $regex = null) {
if (is_float($check) && floor($check) === $check) {
$check = sprintf("%.1f", $check);
}
if (is_null($regex)) {
if (is_null($places)) {
$regex = '/^[-+]?[0-9]*(\\.{1}[0-9]+(?:[eE][-+]?[0-9]+)?)?$/';
$regex = '/^[-+]?[0-9]*\\.{1}[0-9]+(?:[eE][-+]?[0-9]+)?$/';
} else {
$regex = '/^[-+]?[0-9]*(\\.{1}[0-9]{' . $places . '})?$/';
$regex = '/^[-+]?[0-9]*\\.{1}[0-9]{' . $places . '}$/';
}
}
return self::_check($check, $regex);
Expand Down

0 comments on commit b75a6b4

Please sign in to comment.