Skip to content
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

[Validator] Breaking changes in Collection #53455

Closed
axzx opened this issue Jan 8, 2024 · 10 comments
Closed

[Validator] Breaking changes in Collection #53455

axzx opened this issue Jan 8, 2024 · 10 comments

Comments

@axzx
Copy link
Contributor

axzx commented Jan 8, 2024

Symfony version(s) affected

5.4

Description

Before this symfony/validator@b125357

$violations = $this->validator->validate(
            $input,
            new Assert\Collection([
                'channelName' => null,
                'reportType' => null,
                'period' => new Assert\Choice(
                    choices: (new CreateReportScheduleSpecification())->getPeriodAllowableValues(),
                ),
            ]),
        );

After this symfony/validator@b125357

$violations = $this->validator->validate(
            $input,
            new Assert\Collection([
                'fields' => [
                    'channelName' => null,
                    'reportType' => null,
                    'period' => new Assert\Choice(
                        choices: (new CreateReportScheduleSpecification())->getPeriodAllowableValues(),
                    ),
                ],
            ]),
        );

@nicolas-grekas My application stopped working as before after this commit.

How to reproduce

$input = [
            'channelName' => $input->getArgument('channelName'),
            'reportType' => $input->getArgument('reportType'),
            'period' => $input->getArgument('period'),
        ];

$violations = $this->validator->validate(
            $input,
            new Assert\Collection([
                'channelName' => null,
                'reportType' => null,
                'period' => new Assert\Choice(
                    choices: (new CreateReportScheduleSpecification())->getPeriodAllowableValues(),
                ),
            ]),
        );

Possible Solution

No response

Additional Context

No response

@axzx axzx added the Bug label Jan 8, 2024
@axzx axzx changed the title [Validator] [Validator] Breaking changes in Collection Jan 8, 2024
@xabbuh
Copy link
Member

xabbuh commented Jan 8, 2024

@axzx There is a pending PR (see #53443) that tries to fix cases that we broke with previous PRs fixing other bugs. I will take your report into account there too.

@HypeMC
Copy link
Contributor

HypeMC commented Feb 4, 2024

@axzx Could you see if #53755 fixes your issue?

@axzx
Copy link
Contributor Author

axzx commented Feb 6, 2024

@HypeMC

I tested your solution. I have this code:

$constraints['packAccessory'] = [new Assert\NotNull()];
$violations = $this->validator->validate(
            $inputValues,
            new Assert\Collection($constraints),
//            new Assert\Collection([
//                'fields' => $constraints,
//            ]),
        );

I run the tests and get an error 500:

The options \"packAccessory\" do not exist in constraint \"Symfony\\Component\\Validator\\Constraints\\Collection\".

I tested second version:

$violations = $this->validator->validate(
            $inputValues,
//            new Assert\Collection($constraints),
            new Assert\Collection([
                'fields' => $constraints,
            ]),
        );

I run the tests and get an error 422:

"detail":"[fields]: This field is missing."

@HypeMC
Copy link
Contributor

HypeMC commented Feb 6, 2024

@axzx I'm having trouble reproducing your problem. What is the value of $inputValues?

@axzx
Copy link
Contributor Author

axzx commented Feb 8, 2024

@HypeMC

$inputValues = ['packAccessory' => null];
$constraints = ['packAccessory' => [new Assert\NotNull()]];
$violations = $this->validator->validate(
            $inputValues,
            new Assert\Collection($constraints),
        );

@HypeMC
Copy link
Contributor

HypeMC commented Feb 8, 2024

@axzx I've tried your example and didn't get the error. Are you sure you're using the fix?

@axzx
Copy link
Contributor Author

axzx commented Feb 8, 2024

@HypeMC

OK, your solution actually worked, but it stopped working in a certain case.

what about this:

$inputValues = [];
$constraints = [];
$violations = $this->validator->validate(
            $inputValues,
            new Assert\Collection(['fields' => $constraints]),
        );

?

I get an error response:
"detail":"[fields]: This field is missing."

Applications will again stop working after this change.

@HypeMC
Copy link
Contributor

HypeMC commented Feb 8, 2024

@axzx Yes, unfortunately, this all comes down to the fact that there's no way of knowing whether fields refers to the fields option or to a field named fields.

@axzx
Copy link
Contributor Author

axzx commented Feb 8, 2024

@HypeMC
The name fields should be reserved;)

Could the fields option be dropped?

@HypeMC
Copy link
Contributor

HypeMC commented Feb 8, 2024

@axzx Reserving fields or dropping the option are both BC breaks. I'm not sure there's a solution here that works for all cases.

nicolas-grekas added a commit that referenced this issue Feb 9, 2024
… (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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants