ConstraintValidator allows extra fields if all children are optional

[jannes-io]

Symfony version(s) affected: 4.3.3

Description
When running a validator on a collection that only has optional fields it does not give violations on extra fields. It does work however if I make one of the fields required.

How to reproduce

        $toValidate = ['a' => 'b'];

        $constraints = new Assert\Collection([
            'firstName' => new Assert\Optional(),
            'lastName' => new Assert\Optional(),
            'username' => new Assert\Optional(),
        ]);

        $validator = Validation::createValidator();
        $violations = $validator->validate($toValidate, $constraints, new Assert\GroupSequence(['Default', 'custom']));
        // $violations is empty

If I change firstName to new Assert\Positive() as example, then it does work.

{"errors":["[firstName]: This field is missing.","[a]: This field was not expected."]}

[jannes-io]

Another workaround is to add an additional field to the constraint and make it check on NULL. Ex:

        $toValidate['testestest'] = null;
        $constraints = new Assert\Collection([
            'testestest' => new Assert\IsNull(),
            'firstName' => new Assert\Optional(),
            'lastName' => new Assert\Optional(),
            'username' => new Assert\Optional(),
        ]);

But this is a very hackish solution.

[stof]

this looks like an issue related to the handling of the implicit group for this case. Putting an explicit Default group on the collection constraint also makes it work.
Without it, the CollectionValidator is not called at all.

[stof]

It looks like the merging of groups for the Composite constraint does not work well when all nested constraints are composite ones (and Optional is a composite constraint which can be used without anything inside it as in this case).

[jannes-io]

@stof You are correct, if I explicitly state groups and fields on the Collection then it does work aswell.

        $constraints = new Assert\Collection([
            'groups' => 'Default',
            'fields' => [
                'firstName' => new Assert\Optional(),
                'lastName' => new Assert\Optional(),
                'username' => new Assert\Optional(),
            ]
        ]);

Is this intended behavior?

[stof]

and this also works:

$toValidate = ['a' => 'b'];

$constraints = new Assert\Collection([
    'firstName' => new Assert\Optional([new Assert\Type('string')]),
    'lastName' => new Assert\Optional(),
    'username' => new Assert\Optional(),
]);

$validator = \Symfony\Component\Validator\Validation::createValidator();
$violations = $validator->validate($toValidate, array($constraints));

dump($violations);

so this is indeed about composite constraint with no nested constraints (or with only such broken composite constraints as nested constraints)

[HeahDude]

I could not reproduce the issue. Has it been fixed in the meantime? Does it happen with the form integration?

@stof @jannes-io could you please confirm this is still an issue in latest releases?
I've tested 3.4, 4.4, 5.0 and master.
Thanks!

[jannes-io]

@HeahDude I just tested on 4.3 and 4.4 and if all fields are optional it does not return the "UnexpectedField" violation, we've fixed it by always supplying an Assert\Type inside of the optional like @stof suggested:

and this also works:

$toValidate = ['a' => 'b'];

$constraints = new Assert\Collection([
    'firstName' => new Assert\Optional([new Assert\Type('string')]),
    'lastName' => new Assert\Optional(),
    'username' => new Assert\Optional(),
]);

$validator = \Symfony\Component\Validator\Validation::createValidator();
$violations = $validator->validate($toValidate, array($constraints));

dump($violations);

so this is indeed about composite constraint with no nested constraints (or with only such broken composite constraints as nested constraints)

This is the exact symfony/validator we're using now:

            "name": "symfony/validator",
            "version": "4.4.x-dev",
            "source": {
                "type": "git",
                "url": "https://github.com/symfony/validator.git",
                "reference": "a8254c1c4070cd147f343bf43771f28e750e55bd"
            },
            "dist": {
                "type": "zip",
                "url": "https://api.github.com/repos/symfony/validator/zipball/a8254c1c4070cd147f343bf43771f28e750e55bd",
                "reference": "a8254c1c4070cd147f343bf43771f28e750e55bd",
                "shasum": ""
            },

[HeahDude]

@jannes-io thank you for the quick feedback! I've been able to add some tests this time at the root of the issue. In fact this also fails with required keys:

$constraints = new Assert\Collection([
    'fields' => [
        'firstName' => new Assert\Required(),
    ],
]);

as long as we don't define explicitly the Default group as you both confirmed... or don't define a nested "non composite" constraint. But Required and Optional are composite.

See #36365 for a fix. Could you confirm it works for you? Thanks!

[jannes-io]

Hi @HeahDude ,

Thanks for the quick fix! Unfortunately I don't have a "symfony playground" set up at the moment to juggle between versions without bricking the project but from reading the tests in the commit it does align with the behavior that we expect from Required and Optional.

Good for me 👍

Just wondering, when it is merged in 3.4, will that cascade to the next releases of 4.3/4.4 and 5.0 too? Additionally I assume that this will require a minor version bump, seeing that this could be a breaking change for some edge cases?

[HeahDude]

@jannes-io

Just wondering, when it is merged in 3.4, will that cascade to the next releases of 4.3/4.4 and 5.0 too? Additionally I assume that this will require a minor version bump, seeing that this could be a breaking change for some edge cases?

Yes, we merge fixes in the lowest maintained branch possible (currently 3.4, 4.4 or 5.0), then merge branches up to master.