8000 [Validator] deal with fields for which no constraints have been configured by xabbuh · Pull Request #53443 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[Validator] deal with fields for which no constraints have been configured #53443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to 8000 file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion src/Symfony/Component/Validator/Constraints/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class Collection extends Composite
*/
public function __construct($fields = null, array $groups = null, $payload = null, bool $allowExtraFields = null, bool $allowMissingFields = null, string $extraFieldsMessage = null, string $missingFieldsMessage = null)
{
if (\is_array($fields) && ([] === $fields || ($firstField = reset($fields)) instanceof Constraint || ($firstField[0] ?? null) instanceof Constraint)) {
if (self::isFieldsOption($fields)) {
$fields = ['fields' => $fields];
}

Expand Down Expand Up @@ -87,4 +87,31 @@ protected function getCompositeOption()
{
return 'fields';
}

private static function isFieldsOption($options): bool
{
if (!\is_array($options)) {
return false;
}

if ([] === $options) {
return true;
}

foreach ($options as $optionOrField) {
if ($optionOrField instanceof Constraint) {
return true;
}

if (!\is_array($optionOrField)) {
return false;
}

if ([] !== $optionOrField && !($optionOrField[0] ?? null) instanceof Constraint) {
Copy link
Contributor
@tscni tscni Jan 6, 2024 •
edited
Loading

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 = 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'),
            ],
        ],
    ],
);

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor
@tscni tscni Jan 6, 2024 •
edited
Loading

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

new Collection(
    fields: [
        'foo' => new Required([
            new Type('string'),
        ]),
    ],
);
// more explicit
new Collection(
    fields: [
        'foo' => new Required(
            options:  [
                'constraints' => [
                    new Type('string')
                ],
            ],
            groups: ['bar'],
        ),
    ],
);

Copy link
Member

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 = 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

// 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 (!$field instanceof Optional && !$field instanceof Required) {
$this->fields[$fieldName] = new Required($field);
}

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

if (!\is_array($nestedConstraints)) {
$nestedConstraints = [$nestedConstraints];
}

So my conclusion is that this used to work by accident, and was really not intended to work.

return false;
}
}

return true;
}
}
35 changes: 35 additions & 0 deletions src/Symfony/Component/Validator/Tests/Constraints/CollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,41 @@ public function testEmptyFieldsInOptions()
'extraFieldsMessage' => 'foo bar baz',
]);

$this->assertSame([], $constraint->fields);
$this->assertTrue($constraint->allowExtraFields);
$this->assertSame('foo bar baz', $constraint->extraFieldsMessage);
}

public function testEmptyConstraintListFor()
{
$constraint = new Collection([
'foo' => [],
],
null,
null,
true,
null,
'foo bar baz'
);

$this->assertArrayHasKey('foo', $constraint->fields);
$this->assertInstanceOf(Required::class, $constraint->fields['foo']);
$this->assertTrue($constraint->allowExtraFields);
$this->assertSame('foo bar baz', $constraint->extraFieldsMessage);
}

public function testEmptyConstraintListForFieldInOptions()
{
$constraint = new Collection([
'fields' => [
'foo' => [],
],
'allowExtraFields' => true,
'extraFieldsMessage' => 'foo bar baz',
]);

$this->assertArrayHasKey('foo', $constraint->fields);
$this->assertInstanceOf(Required::class, $constraint->fields['foo']);
$this->assertTrue($constraint->allowExtraFields);
$this->assertSame('foo bar baz', $constraint->extraFieldsMessage);
}
Expand Down
0