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] Fix using known option names as field names #53133

Merged
merged 1 commit into from Dec 24, 2023

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Dec 19, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52993
License MIT

Instead of assuming $fields is not the fields array when any of the keys is a known option, let's assume it is the fields array if all keys are known options. Of course, this approach won't work if someone names all their fields after known options, but I don't believe that will actually happen.

Comment on lines 44 to 46
$knowOptions = array_map(function (\ReflectionParameter $parameter) {
return $parameter->name;
}, (new \ReflectionMethod(__METHOD__))->getParameters());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$knowOptions = array_map(function (\ReflectionParameter $parameter) {
return $parameter->name;
}, (new \ReflectionMethod(__METHOD__))->getParameters());
static $knowOptions;
$knownOptions ??= array_column((new \ReflectionMethod(__METHOD__))->getParameters(), 'name');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I had to adjust the code for PHP 7.2. I'm wondering, is the static variable necessary since this all happens in the constructor?

@HypeMC HypeMC force-pushed the fix-collection-field-name-is-option branch 2 times, most recently from e92d8a1 to 51d1960 Compare December 23, 2023 15:22
@HypeMC HypeMC force-pushed the fix-collection-field-name-is-option branch from 51d1960 to 51ec528 Compare December 23, 2023 15:36
@fabpot
Copy link
Member

fabpot commented Dec 24, 2023

Thank you @HypeMC.

@fabpot fabpot merged commit d34c8c4 into symfony:5.4 Dec 24, 2023
8 of 11 checks passed
@HypeMC HypeMC deleted the fix-collection-field-name-is-option branch December 24, 2023 14:50
@tscni
Copy link
Contributor

tscni commented Jan 2, 2024

Fyi, this breaks code where $fields is an empty array

@xabbuh
Copy link
Member

xabbuh commented Jan 3, 2024

@tscni Can you please try #53383?

nicolas-grekas added a commit that referenced this pull request Jan 5, 2024
This PR was merged into the 5.4 branch.

Discussion
----------

[Validator] re-allow an empty list of fields

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #53133 (comment)
| License       | MIT

Commits
-------

098c14c re-allow an empty list of fields
symfony-splitter pushed a commit to symfony/validator that referenced this pull request Jan 5, 2024
This PR was merged into the 5.4 branch.

Discussion
----------

[Validator] re-allow an empty list of fields

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony/symfony#53133 (comment)
| License       | MIT

Commits
-------

098c14cc92 re-allow an empty list of fields
@dresslerdemos
Copy link

dresslerdemos commented Jan 5, 2024

I tried this (#53383) fix and it still breaks for an empty list of constraints, which I confirmed was working in v5.4.29:

    public function testEmptyConstraintList()
    {
        // Symfony\Component\Validator\Exception\InvalidOptionsException : The options "foo" do not exist in constraint "Symfony\Component\Validator\Constraints\Collection".
        new Collection(
            [
                'foo' => []
            ],
            null,
            null,
            true,
            false
        );

        self::assertTrue(true);
    }

For my use case I can mitigate this by using fields, but I wanted to give a heads up regarding backward compatibility.

    public function testEmptyConstraintList2()
    {
        new Collection(
            ['fields' => [
                'foo' => []
            ]],
            null,
            null,
            true,
            false
        );

        self::assertTrue(true);
    }

@nerones
Copy link

nerones commented Jan 5, 2024

just had the same problem as @dresslerdemos

@xabbuh
Copy link
Member

xabbuh commented Jan 5, 2024

I have a fix pending locally. I’ll keep you posted.

@xabbuh
Copy link
Member

xabbuh commented Jan 6, 2024

@dresslerdemos @nerones Can you check if #53443 fixes it for you?

nicolas-grekas added a commit that referenced this pull request 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
symfony-splitter pushed a commit to symfony/validator that referenced this pull request 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 symfony/symfony#53133 (comment), Fix #53455
| License       | MIT

Continuation of #53443.

Commits
-------

f6217d87e6 [Validator] Fix fields without constraints in `Collection`
b341535558 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

Successfully merging this pull request may close these issues.

None yet

8 participants