Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Validator::decimal() to be more flexible with decimal separators and PHP magic. Lots of testing added. #800

Merged
merged 1 commit into from Aug 28, 2012

Conversation

bar
Copy link
Contributor

@bar bar commented Aug 28, 2012

} else {
$regex = '/^[-+]?[0-9]*\\.{1}[0-9]{' . $places . '}$/';
$lnum = '[0-9]+';
$dnum = "(?:[0-9]*[\.]{$lnum}|{$lnum}[\.][0-9]+)";
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this just be (?:[0-9]*[\.]{$lnum}) I don't understand why the alternation is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I forgot to remove the grouping, it was needed to make '10.' valid (but with * instead of + at the end).
$dnum = "(?:[0-9]*[\.]{$lnum}|{$lnum}[\.][0-9]+)";
should have been:
$dnum = "[0-9]*[\.]{$lnum}";
I'll amend it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep thinking it should be $dnum = "(?:[0-9]*[\.]{$lnum}|{$lnum}[\.][0-9]*)";
so that this is true

$this->assertTrue(Validation::decimal('1234.', null));
$this->assertTrue(Validation::decimal('1234.', true));

because this is true

$this->assertTrue(Validation::decimal(1234., null));
$this->assertTrue(Validation::decimal(1234., true));

separators and PHP magic.
Lots of testing added.
$this->assertTrue(Validation::decimal(.01));
$this->assertTrue(Validation::decimal('.01'));
public function testDecimalWithPlacesNull() {
$this->assertTrue(Validation::decimal('+1234.54321', null));
Copy link
Member

Choose a reason for hiding this comment

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

you dont have to change all the test cases and manually add "null", since the default value is null, you dont have to provide this second argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but considering the function heavily depends on that parameter, that's a way of making it known. Also, if the default value changes in the (near) future, the test won't break.

Copy link
Member

Choose a reason for hiding this comment

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

just saying :)

but besides that. why would you want to use decimal (with null) to validate float? there is an own validation rule "numeric" for that. you would only want to use "decimal" if you actually required the digits after the point - hence this validation rule. isnt your changed decimal pretty equal the numeric one now for null? the same thing does not have to be provided twice I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, thanks for the hint!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think exactly the same :P

I invite you to read 28bd688 which got itself to master, and later b75a6b4 which fully explains the rationale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, take a look at this comment
b75a6b4#commitcomment-1773097

and the outdated diff from this very same PR

Copy link
Member

Choose a reason for hiding this comment

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

quite confusing ;) thx anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mess! sorry, b75a6b4 is an extract

markstory added a commit that referenced this pull request Aug 28, 2012
Refactor Validator::decimal() to be more flexible with decimal separators and PHP magic. Lots of testing added.
@markstory markstory merged commit f97d8f9 into cakephp:master Aug 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants