[Validator] Made sure that context changes don't leak out of (Contextual)ValidatorInterface #11412

webmozart · 2014-07-17T15:55:56Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11046
License	MIT
Doc PR	-

Tobion · 2014-07-17T18:13:26Z

src/Symfony/Component/Validator/Context/ExecutionContext.php

@@ -139,7 +139,7 @@ public function __construct(ValidatorInterface $validator, $root, TranslatorInte
    /**
     * {@inheritdoc}
     */
-    public function setNode($value, $object, MetadataInterface $metadata, $propertyPath)
+    public function setNode($value, $object, MetadataInterface $metadata = null, $propertyPath)


The Interface needs to be changed as well

What about the $propertyPath variable that doesn't have a default value.

@Tobion Not necessarily, as we're weakening the API here (which is allowed for subclasses). I don't want to change the interface in a patch release, if possible.

@hhamon The default value is only there to allow passing null as $metadata. Both arguments should be passed explicitly.

@webmozart Then you would need to check $this->context instanceof ExecutionContext in RecursiveContextualValidator whenever you call setNode. This is pretty ugly. So I think changing the interface is cleaner.

And what will you pass to a different implementation when metadata is null? You would need to create a metadata instance, to be able to pass the other parameters.

@Tobion After thinking about this some more, I figured that you are right. Passing null to the method of an interface that technically does not accept null values is bogus, and introduces a dependency to the implementation. I changed the interface now.

redstar504 · 2014-07-17T19:35:34Z

Hey guys, if you see my issue #11413 I was thinking that I could use this ExecutionContextInterface::setNode method in order to set the correct property_path for my constraint, but I didn't know what to specify for the $metadata argument. If this PR gets merged, would this be a valid workaround?

webmozart · 2014-07-18T07:56:46Z

@redstar504 See my answer to your ticket. Using setNode() directly is never recommended because it is marked as internal and may be changed anytime (breaking your code).

tompedals · 2014-07-18T10:24:18Z

We're still having issues if we use the 2.4 validation API. Errors are being set on the form rather than fields. Works fine with 2.5-bc and 2.5.

webmozart · 2014-07-18T11:27:57Z

@tompedals Could you provide a fork of symfony-standard which reproduces your issue?

tompedals · 2014-07-18T15:29:43Z

@webmozart I'll give it a try. It may well have just been cached. The main issue I was having is now resolved anyway as we can use the 2.5 API rather than 2.4. Thanks for looking into it.

…ual)ValidatorInterface

Tobion · 2014-07-24T13:50:52Z

src/Symfony/Component/Validator/Context/ExecutionContextInterface.php

@@ -124,7 +124,7 @@ public function getObject();
     * @internal Used by the validator engine. Should not be called by user
     *           code.
     */
-    public function setNode($value, $object, MetadataInterface $metadata, $propertyPath);
+    public function setNode($value, $object, MetadataInterface $metadata = null, $propertyPath);


please update the phpdoc @param MetadataInterface|null

Tobion · 2014-07-24T13:57:22Z

Btw, there is another Interface problem. ExecutionContext::markObjectAsInitialized is not part of the interface, but it is called on the interface by https://github.com/webmozart/symfony/blob/issue11046/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php#L845

Tobion · 2014-07-24T14:43:12Z

And ValidatorInterface::startContext() does not have a parameter, but the implementation RecursiveValidator::startContext($root = null) has.

fabpot · 2014-07-25T14:43:53Z

This one is quite critical as without the fix, people cannot upgrade from 2.4 to 2.5 (2.4 being obsolete now.)

@webmozart @Tobion Can you find time to finish this one so that we can merge it soon?

Tobion · 2014-07-25T15:21:21Z

Looks good 👍 except the phpdoc I mentioned above. The other problems I figured out are unrelated and can be separate tickets.

webmozart · 2014-07-26T12:45:18Z

@Tobion Thanks, I addressed your comments now. startContext($root = null) shouldn't be a problem, as the parameter is only used internally in RecursiveValidator in validate() and validateProperty(). We could of course introduce a separate, private method, but I'd rather not introduce another method call just for cosmetic purposes.

If I get another go, I can merge the PR. /cc @stof

… to ExecutionContextInterface

Tobion · 2014-07-26T13:00:56Z

But startContext is a public method. So nothing hinders developers to actually use the parameter. So it's not only cosmetic. And changing the behavior of the parameter at some point could be considered BC break even though it's only intended for internal purposes.

webmozart · 2014-07-26T13:03:53Z

Sure, developers can use the parameter, but then their code is bound to the implementation, not the interface - which is their choice. If we want to change anything about that parameter, we need to do so in a BC fashion, but I don't think this is very likely.

Tobion · 2014-07-26T13:16:40Z

I dislike this because it does not respect the interface and is only possible in loosly checked languages like PHP. In stricter languages like Java this is not possible. To me this is a hack without any usefulness. And static code analysis will probably not like this either. I'm +0

webmozart · 2014-07-26T13:18:59Z

I think the solution is pragmatic. But if others agree with you, I'm not opposed to adding another private method either.

Tobion · 2014-07-26T13:22:33Z

Another reason against it is that the parameter is not documented. And documentation is not only for users but also for understanding code, i.e. contributors.

webmozart · 2014-07-26T14:16:31Z

Feel free to open a PR to the branch. :)

fabpot · 2014-07-28T10:05:27Z

👍

…of (Contextual)ValidatorInterface (webmozart) This PR was merged into the 2.5 branch. Discussion ---------- [Validator] Made sure that context changes don't leak out of (Contextual)ValidatorInterface | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11046 | License | MIT | Doc PR | - Commits ------- ff48939 [Validator] Added markObjectAsInitialized() and isObjectInitialized() to ExecutionContextInterface 14b60c8 [Validator] Fixed doc block 91bf277 [Validator] Made sure that context changes don't leak out of (Contextual)ValidatorInterface

peterrehm · 2014-07-28T10:53:15Z

I still see errors attached to a wrong filed with dev-master #7af2563 that errors are attached to the wrong field with 2.5 and 2.5-bc api. 2.4 api works fine.

@webmozart Are you aware of further issues regarding this or shall I try to reproduce it in a SE?

xabbuh · 2014-07-28T13:09:29Z

@peterrehm 2.5 hasn't been merged into master since then.

peterrehm · 2014-07-28T13:21:38Z

@xabbuh Good catch, my bad. Just checked it against 2.5.x-master and could not reproduce my test cases. Seems to be fixed. Thanks!

fabpot · 2014-07-28T13:22:30Z

@peterrehm I've just merged 2.5 into master.

peterrehm · 2014-07-28T13:32:06Z

@fabpot Thanks.

Tobion reviewed Jul 17, 2014
View reviewed changes

romainneutron added the Validator label Jul 17, 2014

[Validator] Made sure that context changes don't leak out of (Context…

91bf277

…ual)ValidatorInterface

Tobion reviewed Jul 24, 2014
View reviewed changes

[Validator] Fixed doc block

14b60c8

[Validator] Added markObjectAsInitialized() and isObjectInitialized()…

ff48939

… to ExecutionContextInterface

webmozart merged commit ff48939 into symfony:2.5 Jul 28, 2014

webmozart deleted the issue11046 branch August 4, 2014 14:40

evanpurkhiser mentioned this pull request Jun 10, 2015

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

Closed

Uh oh!

[Validator] Made sure that context changes don't leak out of (Contextual)ValidatorInterface #11412

[Validator] Made sure that context changes don't leak out of (Contextual)ValidatorInterface #11412

Uh oh!

Conversation

webmozart commented Jul 17, 2014

Uh oh!

Tobion Jul 17, 2014

Choose a reason for hiding this comment

Uh oh!

hhamon Jul 17, 2014

Choose a reason for hiding this comment

Uh oh!

webmozart Jul 18, 2014

Choose a reason for hiding this comment

Uh oh!

Tobion Jul 18, 2014

Choose a reason for hiding this comment

Uh oh!

Tobion Jul 18, 2014

Choose a reason for hiding this comment

Uh oh!

webmozart Jul 24, 2014

Choose a reason for hiding this comment

Uh oh!

redstar504 commented Jul 17, 2014

Uh oh!

webmozart commented Jul 18, 2014

Uh oh!

tompedals commented Jul 18, 2014

Uh oh!

webmozart commented Jul 18, 2014

Uh oh!

tompedals commented Jul 18, 2014

Uh oh!

Tobion Jul 24, 2014

Choose a reason for hiding this comment

Uh oh!

Tobion commented Jul 24, 2014

Uh oh!

Tobion commented Jul 24, 2014

Uh oh!

fabpot commented Jul 25, 2014

Uh oh!

Tobion commented Jul 25, 2014

Uh oh!

webmozart commented Jul 26, 2014

Uh oh!

Tobion commented Jul 26, 2014

Uh oh!

webmozart commented Jul 26, 2014

Uh oh!

Tobion commented Jul 26, 2014

Uh oh!

webmozart commented Jul 26, 2014

Uh oh!

Tobion commented Jul 26, 2014

Uh oh!

webmozart commented Jul 26, 2014

Uh oh!

fabpot commented Jul 28, 2014

Uh oh!

peterrehm commented Jul 28, 2014

Uh oh!

xabbuh commented Jul 28, 2014

Uh oh!

peterrehm commented Jul 28, 2014

Uh oh!

fabpot commented Jul 28, 2014

Uh oh!

peterrehm commented Jul 28, 2014

Uh oh!

Uh oh!