[Validator] Pass validation groups on Collection validate. #23480
Conversation
|
For some reason, Travis did not start. Please add tests also. The "BC break" flag means you should explain what's the precise behavior change, why you think we should still do it, explain why this cannot be done as a new feature on 3.4, with proper BC layer, etc. |
Because I created a pr into master, and then changed to 2.8. Can you restart them manually?
Ok, I will try to find out where they should be.
All description in linked issue. |
|
Does this not happen on Symfony 2.7? |
|
@xabbuh I'm using LTS version 2.8, but I can check 2.7 a bit later. |
|
So, I check all tests, after my fix I have to fix Have no ideas why appveyor build fails, but seems there are some global problems, not because of my code. While fixing the tests, found another place with the same bug: AllValidator. Fix it too. |
I have checked it, the answer is yes, this bug is also actual in Symfony 2.7. I think it will reproduce on 2.5+ (see linked pr in issue) |
|
Can you rebase on 2.7 please? |
9687609 to
7a8cc0d
Compare
|
@nicolas-grekas done! So, does it BC break or not? I was not sure, so I mark pr as BC break. But may be it not 🤔 |
src/Symfony/Component/Validator/Tests/Constraints/AbstractConstraintValidatorTest.php
Outdated
Show resolved
Hide resolved
|
@xabbuh WDYT? |
|
Just noticed that this looks a lot like #17696, maybe the discussion there would help move forward? |
89f25c2 to
343c8ce
Compare
|
I can't understand, why it pasts about 3 weeks since I've report a bug and make a PR that solves it, but it is still not merged. What should I do to speed up merging it? |
|
Hi! I have the same problem with validation groups. Let's merge this pull request. |
|
@nicolas-grekas @xabbuh what do you think? |
|
The changes look valid to me. But can you add some test that would fail without them? |
185b55c to
5c3e710
Compare
|
@xabbuh I added some test (was taken from related pr), they fails in 2.7 without my fixes: |
|
@fabpot @nicolas-grekas @webmozart please look at my bugfix again and merge it please. |
|
You first need to rebase to get rid of the merge commit.
… On 24 Sep 2017, at 20:39, Lesnykh Ilia ***@***.***> wrote:
@fabpot @nicolas-grekas @webmozart please look at my bugfix again and merge it please.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@fabpot I've already rebased, mb you see the cached commits? |
|
@fabpot @xabbuh @nicolas-grekas is there something wrong with this pr, why you don't reply to me? :) |
|
@Aliance I've removed the |
|
May be someone can take a look at this bugfix now? |
| @@ -44,12 +44,12 @@ public function validate($value, Constraint $constraint) | |||
| $validator = $context->getValidator()->inContext($context); | |||
|
|
|||
| foreach ($value as $key => $element) { | |||
| $validator->atPath('['.$key.']')->validate($element, $constraint->constraints); | |||
| $validator->atPath('['.$key.']')->validate($element, $constraint->constraints, $constraint->groups); | |||
There was a problem hiding this comment.
I asked the same in #17696 too: Don't we have to use $context->getGroup() here to not accidentally widen the validation scope?
There was a problem hiding this comment.
I think getters is better than properties, but there is $constraint->constraints not $constraint->getConstraints(). So I wrote the same way.
Should I change only groups, change both or no one?
There was a problem hiding this comment.
No, what I mean is fetching the groups from the context but not from the constraint? There may be more groups configured on the constraint than what groups are actually used to validate.
There was a problem hiding this comment.
Nope, It's not the same. Please see my original issue: #23479
I have attached xdebug screenshots there.
Also I have post a link to my symfony standart fork with easy code, which is broken with current version of Symfony, but it works with my fixes. You can clone it and make sure that $context->getGroup will have only a Default string, while $constraint->constraints will have an array of needed groups: ["info", "Default", "Category] for the Category entity as it was set in validation.yml
There was a problem hiding this comment.
You are right that there is still a problem. But just passing the constraints here does not work either. For example, if you take your example project and modify the controller to only run validation in the Default and modify group, the Length constraint will still be evaluated which is only part of the info group.
|
@Aliance how could we move forward here? |
|
@nicolas-grekas I have no idea, but 1 year bug in Symfony makes me sad 😢 |
|
see also #25888 which tries to fix the issue too (currently with the same approach) |
|
Need any help with finishing this PR? Seems easy enough. Is changing |
|
Thank you all for your input and working on this. This is finally going to be fixed in #29499. |
This PR was merged into the 3.4 branch. Discussion ---------- [Validator] Fixed grouped composite constraints | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17675, #25888, #23480 | License | MIT | Doc PR | ~ From Lisbon :). Thanks @stof, @xabbuh for your help to finally fix this old issue. Commits ------- b53d911 [Validator] Fixed grouped composite constraints
Complete description can be found in related issue.