[Validator] Ensure nested validations for Collections happen in a separate context #14786
Conversation
evanpurkhiser
commented
May 30, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #14639 |
| License | MIT |
| Doc PR | n/a |
|
Ping @fabpot |
|
ping @symfony/deciders |
|
I would like a validation of @webmozart to be sure it is the right fix. I'm not sure the ExecutionContext is meant to be clonable (and different implementations might not share the same ViolationList when cloning btw, which would break this fix) |
|
Hmm, I think this may have some relation to #11412. Looking at the changes in that fix, indeed maybe cloning the context might not be the right solution. Hopefully @webmozart can comment :-) |
|
@webmozart Can you tell us your point of view on this PR? |
|
ping @webmozart |
1 similar comment
|
ping @webmozart |
|
Ping @webmozart on this again :-) |
|
@webmozart The change made in this PR is "simple" enough but we really need your help to validate that this is indeed the right way. |
| @@ -60,7 +60,7 @@ public function validate($value, Constraint $constraint) | |||
| if (count($fieldConstraint->constraints) > 0) { | |||
| if ($context instanceof ExecutionContextInterface) { | |||
| $context->getValidator() | |||
| ->inContext($context) | |||
| ->inContext(clone $context) | |||
There was a problem hiding this comment.
This solution doesn't work. Now all constraints thrown by the validators in the Collection constraint will be added to the cloned context and not appear in the current context (which is the point of calling inContext()).
|
@fabpot Thanks for the ping. Unfortunately this solution would bringe more problems than it would solve. The proper solution is to reset the constraint in the ExecutionContext to the previous one after the nested validation call. We already do the same for the other context properties. We need to add a new method |
|
Status: Needs Work |
|
Closing as @webmozart mentioned that this is not something we can merge anyway. As there is a corresponding issue, this will still be linked there in case someone want to work on @webmozart suggestion. |
|
see #20664 for an alternative approach |