Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[2.3] [Validator] added comparison constraints and validators#790
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should stay on the previous line
stof commentedSep 4, 2011
all the constraint classes should now be tagged with @danielholmes could you update the PR ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it should benull === $object according to the CS
stof commentedNov 11, 2011
@danielholmes ping. could you update the PR according to the comments ? |
danielholmes commentedNov 12, 2011
Hi@stof, apologies for the late reply on this one. Thanks for all the feedback, I've addressed it all now. Let me know if there's anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does it really need to be public ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No that was just laziness - it was just making the tests simpler. I've made it public now
raziel057 commentedApr 3, 2012
It would be cool to integrate this PR. |
stof commentedApr 3, 2012
@danielholmes could you rebase your PR as tests have been moved to a different location ? @bschussek can you review it ? |
danielholmes commentedApr 4, 2012
I've rebased, moved the tests to the new locations and fixed them up to work with current code. Unfortunately the rebase looks a bit messy - not sure what I did wrong |
stof commentedApr 4, 2012
@danielholmes seems like you merged your older remote branch after rebasing instead of forcing the push. this means you duplicated the history instead of cleaning it. Here is the way to clean things: # create a temp branch as backupgit branch tmp# reset your branch before the wrong merge commitgit reset --hard 677559d# cherry-pick the commit done after the merge to move the testsgit cherry-pick 3da62d2# force the push to github to overwrite the old (messy) branch# /!\ take care to specify the branch name to be sure not to mess other branches in your github repogit push origin comparison_validators --force# eventually delete the backup branch once you checked on github that it went finegit branch -D tmp |
danielholmes commentedApr 4, 2012
Amazing! Thanks so much for the tip@stof , that did the trick |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't getters be checked first as they should be the preferred way to access the propery ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it could be:
<?phpif (getter) {$values[] = ...;continue;}if (property) {$values[] = ...;continue;}throw new;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just made a commit to fix this, i.e. favour getters over properties
vicb commentedJul 3, 2012
@danielholmes do you have time to update the PR according to the comments ? |
danielholmes commentedJul 3, 2012
Right now is pretty bad timing for me. I'm flying to the US in 4 days so have a pretty tight schedule until then tying things off. I'll have time to look at it in about 7 days - hopefully that fits in okay with the release schedule |
vicb commentedJul 3, 2012
@danielholmes that will be ok. Thanks for your help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the return value should be removed once you switch to validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added a commit to update this
danielholmes commentedApr 28, 2013
@bschussek@stof looks like 2.3 is stabilising, will this be pushed back to 2.4+ then? |
fabpot commentedApr 29, 2013
I would love to have this in 2.3 as this is now the oldest PR for Symfony. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is BC (between 2.2 & 2.3) and should be noted in CHANGELOG file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can't see where this breaks BC.
webmozart commentedApr 29, 2013
@danielholmes Since no conclusion has been made on the reference handling yet, we have two ways to go now:
What do you prefer? On a side note, feature freeze is tomorrow. |
danielholmes commentedApr 29, 2013
@bschussek my preference then would be to merge it in with values only. References are really useful (start and end dates, from and to prices, etc), but obviously better if it's done right. I'll work on removing references now and have an updated PR ready soon, unless there's any objections |
danielholmes commentedApr 29, 2013
@bschussek removed property references so it only deals with values now |
marcospassos commentedApr 29, 2013
@danielholmes Will you work on |
danielholmes commentedApr 29, 2013
@marcospassos I don't really understand the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we remove the $constraint parameter?
danielholmes commentedApr 29, 2013
@bschussek Feedback addressed |
webmozart commentedApr 29, 2013
👍 |
webmozart commentedApr 29, 2013
Could you please squash the commits? There's a lot of changes in here that clutter up the history. |
danielholmes commentedApr 29, 2013
@bschussek done. The property access changes we made along the way were now unrelated to the validators, so I moved them to their own PR if you still think they'd be useful:#7876 |
webmozart commentedApr 29, 2013
Splendid, thank you! :) |
This PR was merged into the master branch.Discussion----------[2.3] [Validator] added comparison constraints and validators| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |They work on a class level and you specify a list of properties. Included are the standard GreaterThan, GreaterThanOrEqual, LessThan, LessThanOrEqual and Equal. e.g.:```php<?php/** *@Assert:GreaterThan({"diedDate", "bornDate"}) *@Assert:LessThanOrEqual({"bornDate", "firstSchoolDayDate", "diedDate"}) */class Person{ private $bornDate; private $firstSchoolDayDate; private $diedDate;}```I think it would also be useful if they worked on a property level rather than just class. I'm not sure what the default option should be though. e.g. is there a reliable way to determine what's a raw value and what's a property name:```php<?php/**@Assert:GreaterThan(80) */private $iq;/**@Assert:LessThan('dateDied') */private $bornDate;/**@Assert:LessThanOrEqual('C') */private $grade;/**@Assert:GreaterThanOrEqual({50, 'ageStartedSchool'}) */private $age;```Commits-------0bffdff [Validator] Added comparison validators.
fabpot commentedApr 30, 2013
@danielholmes Merge now, thanks! Can you open a PR on the documentation repository? |
This PR was merged into the master branch.Discussion----------[Validator] Add missing translation for new validators from#790> Note: dots are added to be consistent with other validators.| Q | A| ------------- | ---| Bug fix? | yes| License | MITCommits-------90a7e86 [Validator] Add missing translation for new validators from#790
raziel057 commentedNov 29, 2013
Any chance to discuss about the |
They work on a class level and you specify a list of properties. Included are the standard GreaterThan, GreaterThanOrEqual, LessThan, LessThanOrEqual and Equal. e.g.:
I think it would also be useful if they worked on a property level rather than just class. I'm not sure what the default option should be though. e.g. is there a reliable way to determine what's a raw value and what's a property name: