[Form] FormValidator removed code related to removed `cascade_validation` option #18081

peterrehm · 2016-03-09T18:54:53Z

Q	A
Branch	3.0
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#18063
License	MIT

peterrehm · 2016-03-09T18:55:25Z

Args, should be merged into 3.0.

peterrehm · 2016-03-09T19:13:37Z

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

peterrehm · 2016-03-09T21:27:46Z

Does it make sense to keep https://github.com/symfony/symfony/pull/18081/files#diff-86f250a5f79b284f5926aa0f63e1c836R130 and https://github.com/symfony/symfony/pull/18081/files#diff-86f250a5f79b284f5926aa0f63e1c836R164?

sustmi · 2016-03-09T22:50:39Z

I would rename:

testDontValidateIfParentWithoutCascadeValidation
to testDontValidateIfChildWithoutValidConstraint
because there is another test testValidateIfChildWithValidConstraint testing the opposite.

and testValidateConstraintsEvenIfNoCascadeValidation
to testValidateChildConstraintsEvenWithoutValidConstraint.

peterrehm · 2016-03-10T06:38:42Z

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

webmozart · 2016-03-10T08:07:45Z

src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php

    }

-    public function testDontValidateIfParentWithoutCascadeValidation()
+    public function testDontValidateIfParentWithoutValidConstraint()


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

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

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 the cascade_validation option.

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

testValidateConstraintsEvenIfNoValidConstraint

testValidateIfChildWithValidConstraint

testDontValidateIfParentWithoutValidConstraint

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?

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

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

the constraints option: Constraints set in this option are always validated (given their groups match).

the Valid constraint: This constraint (either set in the validation metadata - e.g. as annotation - or in the constraints option) results in the constraints defined ine the metadata of the data being validated.

Example:

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

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

There is no Valid constraint in the constraints option of the address field in the form.

There is no Valid constraint on the Customer::$address property.

If you add the Valid constraint in either of these places, the Length constraint is validated as well.

We could clarify the tests for example by renaming testValidateConstraintsEvenIfNoValidConstraint to testValidateConstraintsOptionEvenIfParentWithoutValidConstraint etc.

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

testValidateChildIfValidConstraint

testDontValidateChildIfNoValidConstraint

testValidateConstraintsOptionEvenIfNoValidConstraint

peterrehm · 2016-03-16T07:43:31Z

Any further feedback?

Tobion · 2016-03-19T14:10:58Z

👍 to be merged in 3.0

Status: Reviewed

sustmi · 2016-03-19T16:16:01Z

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

testValidateConstraintsEvenIfNoValidConstraint (originally testValidateConstraintsEvenIfNoCascadeValidation) 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 to testChildFormDataAreValidatedByFormConstraintsEvenWithoutValidConstraint.

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's children property has always constraint @Valid; see https://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 (originally testDontValidateIfParentWithoutCascadeValidation) tests that no validation is run on child form data when it has no constraints.
I would rename the test to testChildFormDataAreNotValidatedByFormConstraintsIfNoConstraints. And I guess this test makes sense only as a sanity check.

EDIT: Added byFormConstraints 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 · 2016-03-29T20:35:53Z

@webmozart Updated.

webmozart · 2016-04-01T08:11:27Z

👍 apart from my last remark

peterrehm · 2016-04-03T16:14:58Z

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

xabbuh · 2016-04-04T07:06:33Z

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 165

PHP 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 · 2016-04-04T18:32:38Z

Updated.

fabpot · 2016-04-07T15:45:20Z

Thank you @peterrehm.

…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 | MIT Commits ------- 05fe6f9 [Form] FormValidator removed code related to removed option

carsonbot added the Status: Needs Review label Mar 9, 2016

javiereguiluz added Form Bug labels Mar 9, 2016

webmozart reviewed Mar 10, 2016
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 19, 2016

Removed code for deprecated cascade_validation option

0cddc90

Renamed test methods

17d1125

fabpot closed this Apr 7, 2016

fabpot mentioned this pull request May 3, 2016

Release v3.0.5 #18697

Merged

peterrehm deleted the cascade-validation branch May 24, 2016 06:56

dialogik mentioned this pull request Jun 13, 2016

Form validation functionality disappearing? #19045

Closed

Uh oh!

[Form] FormValidator removed code related to removed cascade_validation option #18081

[Form] FormValidator removed code related to removed cascade_validation option #18081

Uh oh!

Conversation

peterrehm commented Mar 9, 2016

Uh oh!

peterrehm commented Mar 9, 2016

Uh oh!

peterrehm commented Mar 9, 2016

Uh oh!

peterrehm commented Mar 9, 2016

Uh oh!

sustmi commented Mar 9, 2016

Uh oh!

peterrehm commented Mar 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterrehm commented Mar 16, 2016

Uh oh!

Tobion commented Mar 19, 2016

Uh oh!

sustmi commented Mar 19, 2016

Uh oh!

peterrehm commented Mar 29, 2016

Uh oh!

webmozart commented Apr 1, 2016

Uh oh!

peterrehm commented Apr 3, 2016

Uh oh!

xabbuh commented Apr 4, 2016

Uh oh!

peterrehm commented Apr 4, 2016

Uh oh!

fabpot commented Apr 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[Form] FormValidator removed code related to removed `cascade_validation` option #18081

[Form] FormValidator removed code related to removed `cascade_validation` option #18081