Skip to content

[Validator] Insufficient documentation or bugs #10992

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crtl opened this issue Feb 13, 2019 · 6 comments · Fixed by #13492
Closed

[Validator] Insufficient documentation or bugs #10992

crtl opened this issue Feb 13, 2019 · 6 comments · Fixed by #13492
Assignees

Comments

@crtl
Copy link

crtl commented Feb 13, 2019

I wanted to use the validator component but I am having some problems and many of them are related to insufficient docs.
Either im to dumb, the docs are incomplete, theres a bug or all of the three.

Given the following example:

$constraints = new Assert\Collection([
    "type" => new Assert\Choice([
        "choices" => ["credentials", "google"],
        "groups" => ["type"]
    ]),
    "token" => new Assert\NotBlank([
        "groups" => ["google"]
    ])
]);

$data = ["type" => "credentials"]
$validator = Validation::createValidator();

$result = $validator->validate($data, $constraints, "type");

The Resulting messages are:

type = 'The value you selected is not a valid choice.'
token = 'The field is missing'

According to the documentation:

Only the group type should be validated.
If I pass a sequence of ["type", "google"] it validates the type group first and then validates google group, but shows all fields from type group as unexpected fields.

Aside of that, all examples are given for object validation.

Usually you will be validating entire objects. But sometimes, you just want to validate a simple value[...]

Its the total opposite in my opinion, in most cases you will get input as an scalar, array or object and not a Class.

@crtl crtl changed the title Insufficient documentation for validator [Validator] Insufficient documentation Feb 13, 2019
@crtl crtl changed the title [Validator] Insufficient documentation [Validator] Insufficient documentation or bugs Feb 13, 2019
@HeahDude
Copy link
Contributor

HeahDude commented Apr 4, 2020

Hello @crtl, sorry for the late response.

In fact your example in not exactly the same but quite similar to the one of your first link.
What is missing is what's documented in the Collection constraint which considers all keys as required by default in all group, so your example is configured as if it was the following:

$constraints = new Assert\Collection([
    "fields" => [
        "type" => new Assert\Required([
            "constraints" => [
                new Assert\Choice([
                    "choices" => ["credentials", "google"],
                    "groups" => ["type"]
                ]),
            ],
            "groups" => ["type", "google"],
        ]),
        "token" => new Assert\Required([
            "constraints" => [
                new Assert\NotBlank([
                    "groups" => ["google"],
                ],
            ],
            "groups" => ["type", "google"],
        ]),
    ],
    // the collection constraint must be traversed for each nested group
    "groups" => ["type", "google"],
    "allowMissingFields" => false,
]);

So each field is required in all groups by default, you need to change this one line in your example to get the intended behavior:

-"token" => new Assert\NotBlank([
+"token" => new Assert\Optional(new Assert\NotBlank([
    "groups" => ["google"]
-])
+]))

@symfony/team-symfony-docs do you think we need a PR for this? Thanks!

@weaverryan
Copy link
Member

I want really aware of this Required and Optional stuff. I DO think we should document this better :)

@HeahDude
Copy link
Contributor

HeahDude commented Apr 5, 2020

Thanks for the input @weaverryan, then I suggest to update this article https://symfony.com/doc/current/validation/raw_values.html to add a new section with groups, to tell about what I've explained in my comment above. WDYT?

@weaverryan
Copy link
Member

That looks right. But we should also link to that (if we don’t already) from the CollectionType

@HeahDude
Copy link
Contributor

HeahDude commented Apr 5, 2020

I guess you're thinking about the Collection constraint, as I was thinking about it too :).

@HeahDude
Copy link
Contributor

HeahDude commented Apr 5, 2020

In fact it fitted more smoothly in the Collection constraint doc itself with a proper note from the article (the opposite of my first thought), I've opened #13492.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants