[2.3] [Validator] added comparison constraints and validators #790

danielholmes · 2011-05-05T17:04:52Z

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
/**
 * @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
/** @assert:GreaterThan(80) */
private $iq;
/** @assert:LessThan('dateDied') */
private $bornDate;
/** @assert:LessThanOrEqual('C') */
private $grade;
/** @assert:GreaterThanOrEqual({50, 'ageStartedSchool'}) */
private $age;

stof · 2011-09-04T12:50:14Z

src/Symfony/Component/Validator/Constraints/AbstractComparisonValidator.php

+                $values[] = $object->$getMethod();
+            } else {
+                $error = sprintf('No method "%s" on object of type "%s"', $getMethod, 
+                            get_class($object));


this should stay on the previous line

stof · 2011-09-04T12:52:13Z

all the constraint classes should now be tagged with @Annotation to make them usable as annotations.

@danielholmes could you update the PR ?

stof · 2011-09-04T12:55:55Z

src/Symfony/Component/Validator/Constraints/AbstractComparisonValidator.php

+    {
+        $valid = false;
+
+        if ($object === null) {


it should be null === $object according to the CS

stof · 2011-11-11T14:59:54Z

@danielholmes ping. could you update the PR according to the comments ?

danielholmes · 2011-11-12T15:09:07Z

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.

stof · 2011-11-12T17:57:47Z

src/Symfony/Component/Validator/Constraints/AbstractComparisonValidator.php

+     * 
+     * @return string The human readable symbol that represents the comparison
+     */
+    abstract public function getSymbol();


does it really need to be public ?

No that was just laziness - it was just making the tests simpler. I've made it public now

raziel057 · 2012-04-03T18:33:48Z

It would be cool to integrate this PR.

stof · 2012-04-03T18:35:30Z

@danielholmes could you rebase your PR as tests have been moved to a different location ?

@bschussek can you review it ?

danielholmes · 2012-04-04T01:51:11Z

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 · 2012-04-04T02:20:16Z

@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 backup
git branch tmp

# reset your branch before the wrong merge commit
git reset --hard 677559d

# cherry-pick the commit done after the merge to move the tests
git 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 repo
git push origin comparison_validators --force

# eventually delete the backup branch once you checked on github that it went fine
git branch -D tmp

danielholmes · 2012-04-04T03:50:23Z

Amazing! Thanks so much for the tip @stof , that did the trick

vicb · 2012-07-03T08:15:57Z

src/Symfony/Component/Validator/Constraints/AbstractComparisonValidator.php

+                $values[] = $reflProperty->getValue($object);
+                $valueFound = true;
+            } else {
+                foreach ($getterMethods as $getterMethod) {


Shouldn't getters be checked first as they should be the preferred way to access the propery ?

it could be:

<?php if (getter) { $values[] = ...; continue; } if (property) { $values[] = ...; continue; } throw new;

Just made a commit to fix this, i.e. favour getters over properties

vicb · 2012-07-03T08:32:16Z

@danielholmes do you have time to update the PR according to the comments ?

danielholmes · 2012-07-03T08:40:49Z

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 · 2012-07-03T09:00:04Z

@danielholmes that will be ok. Thanks for your help.

stof · 2012-07-14T20:49:23Z

src/Symfony/Component/Validator/Constraints/AbstractComparisonValidator.php

+            }
+        }
+
+        return $valid;


the return value should be removed once you switch to validate

Added a commit to update this

danielholmes · 2013-04-28T13:04:27Z

@bschussek @stof looks like 2.3 is stabilising, will this be pushed back to 2.4+ then?

fabpot · 2013-04-29T07:10:10Z

I would love to have this in 2.3 as this is now the oldest PR for Symfony.

stloyd · 2013-04-29T07:14:31Z

src/Symfony/Component/PropertyAccess/PropertyPathBuilder.php

     */
-    public function __construct(PropertyPathInterface $path = null)


This is BC (between 2.2 & 2.3) and should be noted in CHANGELOG file.

I can't see where this breaks BC.

webmozart · 2013-04-29T08:01:22Z

@danielholmes Since no conclusion has been made on the reference handling yet, we have two ways to go now:

Remove reference handling from this PR and merge it without
Wait for 2.4

What do you prefer? On a side note, feature freeze is tomorrow.

danielholmes · 2013-04-29T11:09:38Z

@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 · 2013-04-29T11:27:07Z

@bschussek removed property references so it only deals with values now

marcospassos · 2013-04-29T11:47:22Z

@danielholmes Will you work on Reference PR? I really need this, so if you need some help, please, let me know.

danielholmes · 2013-04-29T11:57:13Z

@marcospassos I don't really understand the Reference PR (actually I don't think there's a PR atm, just an issue to decide on design decisions: #7726 ). I'm more than happy to implement it, but there doesn't seem to be a consensus about what should be implemented

webmozart · 2013-04-29T12:01:15Z

src/Symfony/Component/Validator/Constraints/AbstractComparisonValidator.php

+     *
+     * @return Boolean true if the relationship is valid, false otherwise
+     */
+    abstract protected function compareValues($value1, $value2, Constraint $constraint);


Can we remove the $constraint parameter?

danielholmes · 2013-04-29T12:12:31Z

@bschussek Feedback addressed

webmozart · 2013-04-29T12:28:22Z

👍

webmozart · 2013-04-29T12:29:05Z

Could you please squash the commits? There's a lot of changes in here that clutter up the history.

danielholmes · 2013-04-29T12:54:35Z

@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 · 2013-04-29T14:08:06Z

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 · 2013-04-30T16:46:12Z

@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 | MIT Commits ------- 90a7e86 [Validator] Add missing translation for new validators from #790

This PR was merged into the master branch. Discussion ---------- [Validation] Add dutch translation for new validators This PR already uses the dots added in #7911 Commits ------- 84a0618 Add translation for new validators from #790

This PR was merged into the master branch. Discussion ---------- [Validator] Basque translations for new validators from #790 Commits ------- 971f551 Basque translations for new validators from #790

raziel057 · 2013-11-29T07:52:46Z

Any chance to discuss about the Reference validator to include it in Symfony 2.5 ? It's a important and interesting feature.

ghost assigned webmozart May 6, 2011

stof reviewed Sep 4, 2011
View reviewed changes

stof reviewed Nov 12, 2011
View reviewed changes

vicb reviewed Jul 3, 2012
View reviewed changes

stof reviewed Jul 14, 2012
View reviewed changes

stloyd reviewed Apr 29, 2013
View reviewed changes

webmozart reviewed Apr 29, 2013
View reviewed changes

[Validator] Added comparison validators.

0bffdff

fabpot merged commit 0bffdff into symfony:master Apr 30, 2013

stloyd added a commit to stloyd/symfony that referenced this pull request May 2, 2013

[Validator] Add missing translation for new validators from symfony#790

90a7e86

wouterj added a commit to wouterj/symfony that referenced this pull request May 2, 2013

Add translation for new validators from symfony#790

84a0618

This was referenced May 3, 2013

Document new Comparison Validators symfony/symfony-docs#2590

Closed

Documented the Comparison validators symfony/symfony-docs#2599

Merged

danielholmes mentioned this pull request May 5, 2013

Added documentation for comparison validators symfony/symfony-docs#2603

Closed

eduardosoliv mentioned this pull request Jan 12, 2014

[Validator] Fixed constraint violation to string not handling array #10007

Closed

		*/
		public function __construct(PropertyPathInterface $path = null)

[2.3] [Validator] added comparison constraints and validators #790

[2.3] [Validator] added comparison constraints and validators #790

Conversation

danielholmes commented May 5, 2011

Choose a reason for hiding this comment

stof commented Sep 4, 2011

Choose a reason for hiding this comment

stof commented Nov 11, 2011

danielholmes commented Nov 12, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raziel057 commented Apr 3, 2012

stof commented Apr 3, 2012

danielholmes commented Apr 4, 2012

stof commented Apr 4, 2012

danielholmes commented Apr 4, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Jul 3, 2012

danielholmes commented Jul 3, 2012

vicb commented Jul 3, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielholmes commented Apr 28, 2013

fabpot commented Apr 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmozart commented Apr 29, 2013

danielholmes commented Apr 29, 2013

danielholmes commented Apr 29, 2013

marcospassos commented Apr 29, 2013

danielholmes commented Apr 29, 2013

Choose a reason for hiding this comment

danielholmes commented Apr 29, 2013

webmozart commented Apr 29, 2013

webmozart commented Apr 29, 2013

danielholmes commented Apr 29, 2013

webmozart commented Apr 29, 2013

fabpot commented Apr 30, 2013

raziel057 commented Nov 29, 2013