NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Validator] Ensure nested validations for Collections happen in a separate context#14786

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

evanpurkhiser wants to merge1 commit intosymfony:2.6fromevanpurkhiser:fixup/collection-validation-context

Closed

[Validator] Ensure nested validations for Collections happen in a separate context#14786

evanpurkhiser wants to merge1 commit intosymfony:2.6fromevanpurkhiser:fixup/collection-validation-context

Conversation

Copy link

evanpurkhiser commentedMay 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

Ensure nested validations for Collections happen in a separate context

5a89485

Copy link

Author

evanpurkhiser commentedJun 9, 2015

Ping@fabpot

jakzal added the Validator label

Jun 9, 2015

Copy link

Member

fabpot commentedJun 9, 2015

ping @symfony/deciders

nicolas-grekas added the Ready label

Jun 9, 2015

Copy link

Member

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)

Copy link

Author

evanpurkhiser commentedJun 10, 2015

Hmm, I think thismay 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 :-)

nicolas-grekas added Ready and removed Ready labels

Jun 20, 2015

Copy link

Member

fabpot commentedJun 30, 2015

@webmozart Can you tell us your point of view on this PR?

Copy link

Member

xabbuh commentedJul 2, 2015

ping@webmozart

1 similar comment

Copy link

reket commentedNov 13, 2015

ping@webmozart

Copy link

Author

evanpurkhiser commentedJan 25, 2016

Ping@webmozart on this again :-)

javiereguiluz added Bug and removed Ready labels

Mar 3, 2016

Copy link

Member

fabpot commentedMar 4, 2016

@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.

webmozart reviewed

Mar 4, 2016

View reviewed changes

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

		if ($contextinstanceof ExecutionContextInterface) {
		$context->getValidator()
		->inContext($context)
		->inContext(clone$context)

Copy link

Contributor

webmozartMar 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This solution doesn't work. Now all constraints thrown by the validators in theCollection constraint will be added to the cloned context and not appear in the current context (which is the point of callinginContext()).

Copy link

Contributor

webmozart commentedMar 4, 2016

@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 alreadydo the same for the other context properties.

We need to add a new methodgetConstraint() to ExecutionContext in order to do this. When we switch to 4.0, the method should be added to ExecutionContextInterface as well.

Copy link

Contributor

webmozart commentedMar 4, 2016

Status: Needs Work

carsonbot added the Status: Needs Work label

Mar 4, 2016

Copy link

Member

fabpot commentedJun 9, 2016

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.