-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Add CompoundConstraintTestCase
to ease testing Compound Constraints
#49547
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
[Validator] Add CompoundConstraintTestCase
to ease testing Compound Constraints
#49547
Conversation
2aeaf5b
to
cd080f1
Compare
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
87365e3
to
dac2687
Compare
Hello @alexandre-daubois If not (I totally understand), @stof @nicolas-grekas @xabbuh I suggest making
I lost about two hours yesterday because of that (not blaming anyone) 😬 |
Hi, thanks for the interest in this PR! I'd still love to see this happen in the framework. I'll rebase it on 6.4 🙂 |
1ddaebc
to
3f7261c
Compare
878105b
to
cf20e03
Compare
I just rebased in 7.2, if anyone wants to have a look 🙂 |
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.
LGTM with minor CS changes. Please rebase also.
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Test/CompoundConstraintTestCase.php
Outdated
Show resolved
Hide resolved
cf20e03
to
7d18268
Compare
CompoundConstraintTestCase
to ease testing Compound ConstraintCompoundConstraintTestCase
to ease testing Compound Constraints
7d18268
to
73de44d
Compare
532f838
to
d196c22
Compare
$validator->validate($this->validatedValue, new class($constraints) extends Compound { | ||
public function __construct(array $constraints) | ||
{ | ||
parent::__construct(['constraints' => $constraints]); | ||
} | ||
|
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.
$validator->validate($this->validatedValue, new class($constraints) extends Compound { | |
public function __construct(array $constraints) | |
{ | |
parent::__construct(['constraints' => $constraints]); | |
} | |
$validator->validate($this->validatedValue, new class(['constraints' => $constraints]) extends Compound { |
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.
Actually, this solution (and the other one also unfortunately) doesn't work, it raises an exception:
Symfony\Component\Validator\Exception\ConstraintDefinitionException : You can't redefine the "constraints" option. Use the "Symfony\Component\Validator\Constraints\Compound::getConstraints()" method instead.
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.
ah!
d196c22
to
682d866
Compare
$validator->initialize($context); | ||
|
||
$validator->validate($this->validatedValue, new class($constraints) extends Compound { | ||
public function __construct(public array $constraints) |
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.
You redeclare and assign the $constraints
property to use it in the getConstraints()
implementation which is called by the parent class to populate the $constraints
property. This is very confusing.
I suggest we find a different name for a local $constraints
property and make that property private.
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.
Got it. What do you think of this new naming I just pushed, $testedConstraints
?
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.
👌🏻
682d866
to
14f3252
Compare
Thank you @alexandre-daubois. |
…ructor (derrabus) This PR was merged into the 7.2 branch. Discussion ---------- [Validator] Add $groups and $payload to Compound constructor | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | N/A | License | MIT While reviewing #49547 I noticed that the `$groups` and `$payload` parameters introduced in #41994 cannot be leveraged in compound constraints. I'd like to change that. Commits ------- bf4207c [Validator] Add $groups and $payload to Compound constructor
This PR aims to ease compound constraints to be tested.
Real world use case: we're using the Validator component to test password strength and if they match our password policy. We created a compound constraint for this. This PR allows to write easy and understandable specific tests of compound constraints, like this:
Testing this constraint with this new TestCase helper class would result on a clear test classe: