[Form] FormValidator removed code related to removed cascade_validation option

[peterrehm]

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]

Args, should be merged into 3.0.

[peterrehm]

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

[peterrehm]

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]

I would rename:

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

and testValidateConstraintsEvenIfNoCascadeValidation
to testValidateChildConstraintsEvenWithoutValidConstraint.

[peterrehm]

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

[webmozart]

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

[peterrehm]

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

[webmozart]

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.

[peterrehm]

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

• testValidateConstraintsEvenIfNoValidConstraint
• testValidateIfChildWithValidConstraint
• testDontValidateIfParentWithoutValidConstraint

[webmozart]

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?

[peterrehm]

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.

[webmozart]

@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.

[webmozart]

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

• testValidateChildIfValidConstraint
• testDontValidateChildIfNoValidConstraint
• testValidateConstraintsOptionEvenIfNoValidConstraint

[peterrehm]

Any further feedback?

[Tobion]

👍 to be merged in 3.0

Status: Reviewed

[sustmi]

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]

@webmozart Updated.

[webmozart]

👍 apart from my last remark

[peterrehm]

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

[xabbuh]

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]

Updated.

[fabpot]

Thank you @peterrehm.