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] FormValidator removed code related to removedcascade_validation option#18081

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

Closed
peterrehm wants to merge2 commits intosymfony:masterfrompeterrehm:cascade-validation
Closed

Conversation

@peterrehm
Copy link
Contributor

QA
Branch3.0
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#18063
LicenseMIT

@peterrehm
Copy link
ContributorAuthor

Args, should be merged into 3.0.

@peterrehm
Copy link
ContributorAuthor

I see some tests are failing, but only with some deps versions. Does anyone have a hint in this regards?

@sustmi
Copy link
Contributor

I would rename:

testDontValidateIfParentWithoutCascadeValidation
totestDontValidateIfChildWithoutValidConstraint
because there is another testtestValidateIfChildWithValidConstraint testing the opposite.

andtestValidateConstraintsEvenIfNoCascadeValidation
totestValidateChildConstraintsEvenWithoutValidConstraint.

@peterrehm
Copy link
ContributorAuthor

Updated, just the PHP 7 test with the low deps fails, is this maybe due to subtree split or so?

}

publicfunctiontestDontValidateIfParentWithoutCascadeValidation()
publicfunctiontestDontValidateIfParentWithoutValidConstraint()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need these tests? Don't they duplicate other tests here?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I am not sure, I thought it might be intended as it looks like the test for an explizit cascade without the option/constraint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate if you could check the other test cases whether any of them duplicates this one. If that's the case, we can safely remove this. This was mainly designed in order to test the special treatment of thecascade_validation option.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

From my understanding it looks like it is not duplicated as we have the related tests

  • testValidateConstraintsEvenIfNoValidConstraint
  • testValidateIfChildWithValidConstraint
  • testDontValidateIfParentWithoutValidConstraint

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me phrase this another way: Is there any way to make these two tests fail (individually)without making at least one other test in this test class fail at the same time?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Seems so, but I am actually a bit confused about the difference oftestValidateConstraintsEvenIfNoValidConstraint andtestDontValidateIfParentWithoutValidConstraint as it looks like in the first tests constraints in the child are validated even when there is no Valid constraint.

Copy link
Contributor

Choose a reason for hiding this comment

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

@peterrehm I guess the naming of the tests is confusing. We have two different cases:

  • theconstraints option: Constraints set in this option arealways validated (given their groups match).
  • theValid constraint: This constraint (either set in the validation metadata - e.g. as annotation - or in theconstraints option) results in the constraints defined ine the metadata of thedata being validated.

Example:

// data_class = Customer$form->add('address', FormType::class);$form->get('address')->add('street', TextType::class, ['constraints' =>newNotBlank(),]);class Customer{private$address;}class Address{/**     * @Length(min=3)     */private$street;}

In this example, by default only theNotBlank constraint is validated. The constraints defined in theAddress class, however, are not validated:

  • There is noValid constraint in theconstraints option of theaddress field in the form.
  • There is noValid constraint on theCustomer::$address property.

If you add theValid constraint in either of these places, theLength constraint is validated as well.

We could clarify the tests for example by renamingtestValidateConstraintsEvenIfNoValidConstraint totestValidateConstraintsOptionEvenIfParentWithoutValidConstraint etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks better! Could you name the three tests for the Valid constraint consistently?

  • testValidateChildIfValidConstraint
  • testDontValidateChildIfNoValidConstraint
  • testValidateConstraintsOptionEvenIfNoValidConstraint

@peterrehm
Copy link
ContributorAuthor

Any further feedback?

@Tobion
Copy link
Contributor

👍 to be merged in 3.0

Status: Reviewed

@sustmi
Copy link
Contributor

After re-reading the tests I came to the following conclusions:

testValidateConstraintsEvenIfNoValidConstraint (originallytestValidateConstraintsEvenIfNoCascadeValidation) tests that child form data is validated by constraints defined in the child form even if the child form has no@Valid constraint.
I would rename the test totestChildFormDataAreValidatedByFormConstraintsEvenWithoutValidConstraint.

testValidateIfChildWithValidConstraint tests that child form data is validated if the child form has@Valid constraint.
I am not sure if the test makes sense since (as you can see in previous test) constraints defined on subforms are by default always validated (because form'schildren property has always constraint@Valid; seehttps://github.com/symfony/symfony/blob/3.0/src/Symfony/Component/Form/Resources/config/validation.xml).
We could rename the test to:testChildFormDataAreValidatedByFormConstraintsIfValidConstraint, but I am not sure if the test makes sense. Also I think that the test should assert that some other constraints are validated too, not just the@Valid.

testDontValidateIfParentWithoutValidConstraint (originallytestDontValidateIfParentWithoutCascadeValidation) tests that no validation is run on child form data when it has no constraints.
I would rename the test totestChildFormDataAreNotValidatedByFormConstraintsIfNoConstraints. And I guess this test makes sense only as a sanity check.

EDIT: AddedbyFormConstraints into the new test names as there is a difference between validating form data by constraints defined in form and constraints defined on the class of the form data object.

@peterrehm
Copy link
ContributorAuthor

@webmozart Updated.

@webmozart
Copy link
Contributor

👍 apart from my last remark

@peterrehm
Copy link
ContributorAuthor

@webmozart: Updated, I hope it is as per your request.

@xabbuh
Copy link
Member

Looks like you need to fix the tests:

Fatal error: Cannot redeclare Symfony\Component\Form\Tests\Extension\Validator\Constraints\FormValidatorTest::testValidateConstraintsOptionEvenIfNoValidConstraint() in /home/travis/build/symfony/symfony/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php on line 165PHP Fatal error:  Cannot redeclare Symfony\Component\Form\Tests\Extension\Validator\Constraints\FormValidatorTest::testValidateConstraintsOptionEvenIfNoValidConstraint() in /home/travis/build/symfony/symfony/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php on line 165

@peterrehm
Copy link
ContributorAuthor

Updated.

@fabpot
Copy link
Member

Thank you@peterrehm.

fabpot added a commit that referenced this pull requestApr 7, 2016
…ade_validation` option (peterrehm)This PR was submitted for the master branch but it was merged into the 3.0 branch instead (closes#18081).Discussion----------[Form] FormValidator removed code related to removed `cascade_validation` option| Q             | A| ------------- | ---| Branch        | 3.0| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#18063| License       | MITCommits-------05fe6f9 [Form] FormValidator removed code related to removed  option
@fabpotfabpot closed thisApr 7, 2016
@fabpotfabpot mentioned this pull requestMay 3, 2016
@peterrehmpeterrehm deleted the cascade-validation branchMay 24, 2016 06:56
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@peterrehm@sustmi@Tobion@webmozart@xabbuh@fabpot@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp