[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 🙂 |
nicolas-grekas
left a comment
There was a problem hiding this comment.
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.
| $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.
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.
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.
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.
Got it. What do you think of this new naming I just pushed, $testedConstraints?
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: