Skip to content

Commit

Permalink
Make Validation::decimal accept integers
Browse files Browse the repository at this point in the history 
Fix #2800
Force locale of ValidationTests with en_US to ensure decimal dot separator
  • Loading branch information
@CauanCabral @markstory
CauanCabral authored and markstory committed Apr 18, 2012
1 parent afbd582 commit 28bd688
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 5 deletions.
12 changes: 9 additions & 3 deletions lib/Cake/Test/Case/Utility/ValidationTest.php
View file
Expand Up @@ -103,6 +103,8 @@ class ValidationTest extends CakeTestCase {
public function setUp() { public function setUp() {
parent::setUp(); parent::setUp();
$this->_appEncoding = Configure::read('App.encoding'); $this->_appEncoding = Configure::read('App.encoding');
$this->_appLocale = setlocale(LC_ALL, "0");
setlocale(LC_ALL, 'en_US');
} }


/** /**
Expand All @@ -113,6 +115,7 @@ public function setUp() {
public function tearDown() { public function tearDown() {
parent::tearDown(); parent::tearDown();
Configure::write('App.encoding', $this->_appEncoding); Configure::write('App.encoding', $this->_appEncoding);
setlocale(LC_ALL, $this->_appLocale);
} }


/** /**
Expand Down Expand Up @@ -1490,10 +1493,13 @@ 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('-0123.45e6'));
$this->assertTrue(Validation::decimal('0123.45e6')); $this->assertTrue(Validation::decimal('0123.45e6'));
$this->assertTrue(Validation::decimal('1234'));
$this->assertTrue(Validation::decimal('-1234'));
$this->assertTrue(Validation::decimal('+1234'));
$this->assertTrue(Validation::decimal(1234.56));
$this->assertTrue(Validation::decimal(1234.00));

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


/** /**
Expand Down
4 changes: 2 additions & 2 deletions lib/Cake/Utility/Validation.php
View file
Expand Up @@ -382,9 +382,9 @@ public static function boolean($check) {
public static function decimal($check, $places = null, $regex = null) { public static function decimal($check, $places = null, $regex = null) {
if (is_null($regex)) { if (is_null($regex)) {
if (is_null($places)) { if (is_null($places)) {
$regex = '/^[-+]?[0-9]*\\.{1}[0-9]+(?:[eE][-+]?[0-9]+)?$/'; $regex = '/^[-+]?[0-9]*(\\.{1}[0-9]+(?:[eE][-+]?[0-9]+)?)?$/';
} else { } else {
$regex = '/^[-+]?[0-9]*\\.{1}[0-9]{' . $places . '}$/'; $regex = '/^[-+]?[0-9]*(\\.{1}[0-9]{' . $places . '})?$/';
} }
} }
return self::_check($check, $regex); return self::_check($check, $regex);
Expand Down

11 comments on commit 28bd688

@bar
Copy link
Contributor

@bar bar commented on 28bd688 Aug 24, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Validator::decimal() nos validation decimal point anymore?¿
Even the docs says it should:

Checks that a value is a valid decimal. If $places is null, the $check is allowed to be a scientific float
If no decimal point is found a false will be returned. Both the sign and exponent are optional.

From:

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

to

$this->assertTrue(Validation::decimal('1234'));

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well 1234 is a valid decimal value, as it has implicit 00's

@bar
Copy link
Contributor

@bar bar commented on 28bd688 Aug 24, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but the whoie point of testing against decimal(), and not numeric() or naturalNumber() is to detect a decimal point (again, as the documentation says). It can't be right that (int)1234 passes the test...

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original issue that created this change was http://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/2800 The wikipedia article on decimal values says

Trailing zeros after the decimal point are not necessary

Which seems reasonable considering decimal values generally come from user input, and requiring trailing zero's seems a bit excessive. Both '10' and '10.000' have the same representation in both PHP and Javascript. Additionally databases like MySQL and SQLite treat these values as interchangeable as does Postgres from what I remember.

@bar
Copy link
Contributor

@bar bar commented on 28bd688 Aug 24, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because of PHP type juggling feature and being loosely typed, it removes trailing zeroes and automatically types the value to float, so when you type 13.00 ends being (float) 13, but when you type '13.00', it does not change but correctly passes the tests.

The solution to that can be undoing what PHP does when a float number is detected, a working proof of concept with old tests and new ones should be:

bar@70deff6

@bar
Copy link
Contributor

@bar bar commented on 28bd688 Aug 24, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating the commit with a clearly invalid test wich fails here because of the ()? regex grouping

$this->assertFalse(Validation::decimal(''));

bar@a59548a

@milesj
Copy link

@milesj milesj commented on 28bd688 Aug 24, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with bar. I would only use the decimal validation rule if I wanted to validate a number with a decimal place: 10.23, 45323.33 and even 4343.00. Numbers without a decimal shouldn't pass IMO.

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bar a59548a5e951f433dc24e3040c708941ca37de50 seems like a reasonable change as '' passing as a decimal is wrong like you said.

@bar
Copy link
Contributor

@bar bar commented on 28bd688 Aug 24, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory yes, it seemed odd...

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change @bar, merged in b75a6b4

@bar
Copy link
Contributor

@bar bar commented on 28bd688 Aug 25, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, being able to contribute to the core is always a pleasure!

Please sign in to comment.