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

[Form] Fixed handling groups sequence validation#36343

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

@HeahDude
Copy link
Contributor

@HeahDudeHeahDude commentedApr 4, 2020
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFIx#9939 (comment),Fix#35556
LicenseMIT
Doc PR~

This is not the same as the original issue fixed by#36245, that was reported in#9939 (comment).

The form also fails to cascade sequence validation properly because each nested field is validated against the sequence, and one can fail at a step independently from another which could failed in another step. I've added a lot of tests to ensure this is working properly and tested in a website skeleton too.

This PR aims toclose#35556 which tries to fix the same issue but afterwards in its implementation as said in#35556 (comment).

@HeahDudeHeahDudeforce-pushed thefix/form-validate_sequences branch fromc4ab608 toa2ea4b1CompareApril 4, 2020 22:38
@HeahDudeHeahDudeforce-pushed thefix/form-validate_sequences branch 8 times, most recently fromb768608 toac17db3CompareApril 11, 2020 12:28
@greedyivan
Copy link
Contributor

greedyivan commentedApr 12, 2020
edited
Loading

This solution ruins the fields hierarchy in form errors and returns[field1].data instead ofchildren[field1].data.

And it does not meet all of the sequence of groups requirements (e.g., when all the groups included in each array are validated). As it described here fromThe second point is...:#35556 (comment)

@xabbuh
Copy link
Member

Can you provide a failing test case for what you have in mind?

@greedyivan
Copy link
Contributor

Can you provide a failing test case for what you have in mind?

There is a file with tests for that (https://github.com/symfony/symfony/pull/35556/files,FormTypeValidatorExtensionTest.php):
FormTypeValidatorExtensionTest::testGroupSequenceWithConstraintsOptionMatrix
FormTypeValidatorExtensionTest::provideGroupsSequenceAndResultData

All tests will fail because ofchildren[...].data.
And also two with sequence[['First', 'Second'], 'Third']

@HeahDudeHeahDudeforce-pushed thefix/form-validate_sequences branch fromfaf06f1 to9aa6963CompareApril 15, 2020 22:21
@HeahDude
Copy link
ContributorAuthor

Thanks@greedyivan for the review, I've add a test to cover nested array groups in sequence and push some fixes in9aa6963.

@greedyivan
Copy link
Contributor

All tests from my PR passed.

HeahDude and xabbuh reacted with thumbs up emoji

@HeahDude
Copy link
ContributorAuthor

Thank you for confirming.

@HeahDudeHeahDudeforce-pushed thefix/form-validate_sequences branch from9aa6963 todfb61c2CompareApril 18, 2020 11:27
@nicolas-grekas
Copy link
Member

Thank you@HeahDude.

@nicolas-grekasnicolas-grekas merged commit0f1a5c4 intosymfony:3.4Apr 18, 2020
@HeahDudeHeahDude deleted the fix/form-validate_sequences branchApril 18, 2020 14:02
This was referencedApr 28, 2020
xabbuh added a commit that referenced this pull requestMay 2, 2020
This PR was squashed before being merged into the 3.4 branch (closes#36627).Discussion----------[Validator] fix lazy property usage.| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |Fix#36343| License       | MIT| Doc PR        |This attempts to fix a large regression introduced in#36343, which broke recursing values returned from `getter` Constraints, because they are now wrapped  in in a `LazyProperty`. The `LazyProperty` needs to be evaluated because some checks are done on the type of `$value`, i.e `is_array` etc... in `validateGenericNode`.I'm concerned that the original PR didn't really add sufficient test coverage for the introduction of `LazyProperty`, and I'm not 100% sure that I've caught all the cases where the `instanceof` check are needed in this PR.For the tests, I added the `@dataProvider getConstraintMethods` to every test that hit the problem area of code.~~The only issue is that my fixed has broken the test introduced in#36343, `testGroupedMethodConstraintValidateInSequence`.~~~~I think I need@HeahDude to help me work through this. Maybe there is a more simple solution, one that doesn't require doing `instanceof LazyPropery` checks in multiple places, because this feels very brittle.~~EDIT: fixed that test.Commits-------281861e [Validator] fix lazy property usage.
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

@xabbuhxabbuhxabbuh approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

5 participants

@HeahDude@greedyivan@xabbuh@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp