Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] Fix using known option names as field names#53133

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@HypeMC
Copy link
Member

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#52993
LicenseMIT

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());

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
MemberAuthor

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?

@HypeMCHypeMCforce-pushed thefix-collection-field-name-is-option branch 2 times, most recently frome92d8a1 to51d1960CompareDecember 23, 2023 15:22
@HypeMCHypeMCforce-pushed thefix-collection-field-name-is-option branch from51d1960 to51ec528CompareDecember 23, 2023 15:36
@fabpot
Copy link
Member

Thank you@HypeMC.

@fabpotfabpot merged commitd34c8c4 intosymfony:5.4Dec 24, 2023
@HypeMCHypeMC deleted the fix-collection-field-name-is-option branchDecember 24, 2023 14:50
This was referencedDec 30, 2023
@tscni
Copy link
Contributor

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

@xabbuh
Copy link
Member

@tscni Can you please try#53383?

nicolas-grekas added a commit that referenced this pull requestJan 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       | MITCommits-------098c14c re-allow an empty list of fields
symfony-splitter pushed a commit to symfony/validator that referenced this pull requestJan 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        |Fixsymfony/symfony#53133 (comment)| License       | MITCommits-------098c14cc92 re-allow an empty list of fields
@dresslerdemos
Copy link

dresslerdemos commentedJan 5, 2024•
edited
Loading

I tried this (#53383) fix and it still breaks for an empty list of constraints, which I confirmed was working inv5.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 usingfields, 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 and m-stilling reacted with eyes emoji

@nerones
Copy link

just had the same problem as@dresslerdemos

@xabbuh
Copy link
Member

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

@xabbuh
Copy link
Member

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

nicolas-grekas added a commit that referenced this pull requestFeb 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       | MITContinuation 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 requestFeb 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        |Fixsymfony/symfony#53133 (comment), Fix #53455| License       | MITContinuation 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

@xabbuhxabbuhxabbuh approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

8 participants

@HypeMC@fabpot@tscni@xabbuh@dresslerdemos@nerones@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp