Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] deal with fields for which no constraints have been configured#53443
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returnfalse; | ||
| } | ||
| if ([] !==$optionOrField && !($optionOrField[0] ??null)instanceof Constraint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In theory this would still break constructions like these
$c =newCollection( ['foo' => [// implicit "new Required(...)" [newType('string'), ], ], ],);$c =newCollection( ['foo' => [// implicit "new Required(...)"'groups' => ['bar'],// no constraint in first value'constraints' => [newType('string'), ], ], ],);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not sure I understand what you mean. Both examples where already invalid before, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I meant that they originally worked before#53133
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure? How were both forms supposed to end up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, both work on e.g. 6.4.0. That 0 index access seemed suspicous to me, so I tried to break it a bit.
(Note that I don't care about this working, but thought it worthwhile to point out)
The examples above would turn into
newCollection( fields: ['foo' =>newRequired([newType('string'), ]), ],);// more explicitnewCollection( fields: ['foo' =>newRequired( options: ['constraints' => [newType('string') ], ], groups: ['bar'], ), ],);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After doing some testing, I don't think it makes sense to support this case, here's why:
$c1 =newCollection( ['foo' => [ [newType('string'), ], ], ],);$c2 =newCollection( ['foo' => [newType('string'), ], ],);$c3 =newCollection( ['foo' =>newType('string'), ],);
All three cases result in:
array ('foo' => \Symfony\Component\Validator\Constraints\Required::__set_state(array('payload' =>NULL,'groups' =>array (0 =>'Default', ),'constraints' =>array (0 => \Symfony\Component\Validator\Constraints\Type::__set_state(array('payload' =>NULL,'groups' =>array (0 =>'Default', ),'message' =>'This value should be of type {{ type }}.','type' =>'string', )), ), )),)
The reason why the first example works is because of the following code:
| // the XmlFileLoader and YamlFileLoader pass the field Optional | |
| // and Required constraint as an array with exactly one element | |
| if (\is_array($field) &&1 ==\count($field)) { | |
| $this->fields[$fieldName] =$field =$field[0]; | |
| } | |
| if (!$fieldinstanceof Optional && !$fieldinstanceof Required) { | |
| $this->fields[$fieldName] =newRequired($field); | |
| } |
So when it gets to line 76, it's eithernew Required([new Type('string')]) ornew Required(new Type('string')), and sinceRequired casts non arrays to arrays, the result is the same:
symfony/src/Symfony/Component/Validator/Constraints/Composite.php
Lines 64 to 66 inff5dc26
| if (!\is_array($nestedConstraints)) { | |
| $nestedConstraints = [$nestedConstraints]; | |
| } |
So my conclusion is that this used to work by accident, and was really not intended to work.
xabbuh commentedJan 8, 2024
Status: Needs work |
HypeMC commentedJan 30, 2024
@xabbuh Do you need any help finishing this one? I feel responsible cause it was my PR that caused the issues 😄 |
xabbuh commentedJan 31, 2024
@HypeMC Please do if you have the time for it. :) |
HypeMC commentedFeb 4, 2024
xabbuh commentedFeb 4, 2024
closing in favor of#53755 |
… (xabbuh, HypeMC)This PR was merged into the 5.4 branch.Discussion----------[Validator] Fix fields without constraints in `Collection`| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#53133 (comment),Fix#53455| License | MITContinuation of#53443.Commits-------f6217d8 [Validator] Fix fields without constraints in `Collection`b341535 deal with fields for which no constraints have been configured
Uh oh!
There was an error while loading.Please reload this page.