[Validator] deal with fields for which no constraints have been configured #53443
Conversation
xabbuh
commented
Jan 6, 2024
•
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | Fix #53133 (comment), Fix #53455 |
| License | MIT |
| return false; | ||
| } | ||
|
|
||
| if ([] !== $optionOrField && !($optionOrField[0] ?? null) instanceof Constraint) { |
There was a problem hiding this comment.
In theory this would still break constructions like these
$c = new Collection(
[
'foo' => [ // implicit "new Required(...)"
[
new Type('string'),
],
],
],
);
$c = new Collection(
[
'foo' => [ // implicit "new Required(...)"
'groups' => ['bar'], // no constraint in first value
'constraints' => [
new Type('string'),
],
],
],
);There was a problem hiding this comment.
I am not sure I understand what you mean. Both examples where already invalid before, right?
There was a problem hiding this comment.
I meant that they originally worked before #53133
There was a problem hiding this comment.
Are you sure? How were both forms supposed to end up?
There was a problem hiding this comment.
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
new Collection(
fields: [
'foo' => new Required([
new Type('string'),
]),
],
);
// more explicit
new Collection(
fields: [
'foo' => new Required(
options: [
'constraints' => [
new Type('string')
],
],
groups: ['bar'],
),
],
);There was a problem hiding this comment.
After doing some testing, I don't think it makes sense to support this case, here's why:
$c1 = new Collection(
[
'foo' => [
[
new Type('string'),
],
],
],
);
$c2 = new Collection(
[
'foo' => [
new Type('string'),
],
],
);
$c3 = new Collection(
[
'foo' => new Type('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:
symfony/src/Symfony/Component/Validator/Constraints/Collection.php
Lines 69 to 77 in ff5dc26
So when it gets to line 76, it's either new Required([new Type('string')]) or new Required(new Type('string')), and since Required casts non arrays to arrays, the result is the same:
symfony/src/Symfony/Component/Validator/Constraints/Composite.php
Lines 64 to 66 in ff5dc26
So my conclusion is that this used to work by accident, and was really not intended to work.
|
Status: Needs work |
|
@xabbuh Do you need any help finishing this one? I feel responsible cause it was my PR that caused the issues 😄 |
|
@HypeMC Please do if you have the time for it. :) |
|
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 | MIT Continuation of #53443. Commits ------- f6217d8 [Validator] Fix fields without constraints in `Collection` b341535 deal with fields for which no constraints have been configured