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

Add rule "Age" to validation. #150

Closed
wants to merge 5 commits into from
Closed

Conversation

malukenho
Copy link

Add "Age" rule in favor of "MinimumAge".

Age rule works pretty much the same way “minimumAge”
worked but it optionally accepts a maximum age as second
argument.

See #103.

Blockers for merge

  • Add age rule on README.
  • Add age rule on Validator docblocs.
  • Remove minimumAge rule from Validator docblocs.
  • Modify README to reflect removal of minimumAge (point deprecation in favor of age).
  • Clean up unrelated commits and merges.
  • Make age compatible with minimumAge.

@augustohp augustohp added this to the 0.6.0 milestone Feb 20, 2014
@augustohp
Copy link
Member

Missing README.md changes.

@augustohp
Copy link
Member

Hmn, there is a merge commit on the pull request. Could you do a git fetch upstream && git rebase upstream/master on your branch and then a forced push to update the pull request and get rid of the unnecessary merge commit?

@augustohp
Copy link
Member

Ping

@augustohp augustohp modified the milestones: 0.7.0, 0.6.0 Jun 26, 2014
@malukenho malukenho closed this Jun 27, 2014
@augustohp
Copy link
Member

Any reason why you closed this? I see some confusion on the commits, If I were you I would:

  1. Rebase this current branch against the 0.6.0 tag.
  2. Create a different branch for issue 190.
  3. Checkout the age branch to d05f7b4 which seems to have the right delta and push --force that to your remote branch.

PS: You may need to create another pull request for issue 190, since you are -apparently- using the same branch for both issues.

@augustohp augustohp reopened this Jul 11, 2014
@malukenho
Copy link
Author

@augustohp what's missing here yet?

@henriquemoody
Copy link
Member

I'm sorry by our late.

@malukenho, can you please rebase with our current master an update documentation (README.md and Validator docblocks)?


} elseif (!is_string($input) || (is_null($this->format) && false === strtotime($input))) {
return false;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Avoid to use else.

@henriquemoody henriquemoody modified the milestone: 0.7.0 Jan 7, 2015
malukenho and others added 4 commits January 19, 2015 18:28
Instead of adding a MaximumAge rule, this creates the Age rule which
incorporates both rules: minimum and maximum age. Maximum age is optional.
Fixup this commit.
This needs a fix, the "format" parameters accepted by "minimumAge"
rule is not supported by the new validator. It was supposed to be
compatible and do the exactly same thing as "minimumAge".

It would be cool to keep the same API as the "minimumAge", example:

	<?php
		// As it was before
		v::minimumAge(18, 'Y-m-d')->validate('1990-01-01'); // true

		// Needs to work that way
		v::age(18, 'Y-m-d')->validate('1990-01-01'); // true
		v::age(18, 65, 'Y-m-d')->validate('1990-01-01'); //true
	?>

It could work that way as the `format` will always be something
acceptable by `strtotime()` function. And an `age` (an small integer)
can also be a string (Ex: "18") but is not a valid format. So, if the
second parameter is given, and is not an `is_numeric()`, it is a format.

use DateTime;

class AgeTest extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Wrong file name, also wrong path, it must be tests/Rules/AgeTest.php

Copy link
Author

Choose a reason for hiding this comment

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

My fail, that need to be removed

@henriquemoody
Copy link
Member

@malukenho, I'm sorry to make you change it again, but we decided to keep BC between minor versions, so just let maximumAge() as it is unless age() is totally compatible with maximumAge() rule, if that's the case you can just use inheritance on MaximumAge.

@henriquemoody henriquemoody added this to the 0.8 milestone Jan 20, 2015
@henriquemoody
Copy link
Member

@malukenho, is there any progress on that issue?

@AndyWendt
Copy link
Contributor

I could look into this if you like and get it fixed up. Unless there is something else you would rather have me work on.

@henriquemoody
Copy link
Member

If @malukenho wants to pass this to another one I'm ok with that, let's just wait for his answer.

@henriquemoody
Copy link
Member

@AndyWendt, since @malukenho didn't answer our question and there are 21 days since the last interaction, I'd be glad if you finish it.

@henriquemoody henriquemoody modified the milestones: Backlog, 0.8 Feb 11, 2015
@henriquemoody henriquemoody mentioned this pull request Feb 19, 2015
@henriquemoody henriquemoody removed this from the Backlog milestone Sep 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants