Skip to content

Commit

Permalink
do not validate non-submitted form fields in PATCH requests
Browse files Browse the repository at this point in the history
  • Loading branch information
xabbuh committed Feb 21, 2019
1 parent 823ccf0 commit a60d802
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 51 deletions.
Expand Up @@ -41,7 +41,7 @@ public function validate($form, Constraint $formConstraint)

$validator = $this->context->getValidator()->inContext($this->context);

if ($form->isSynchronized()) {
if ($form->isSubmitted() && $form->isSynchronized()) {
// Validate the form data only if transformation succeeded
$groups = self::getValidationGroups($form);
$data = $form->getData();
Expand Down Expand Up @@ -90,7 +90,7 @@ public function validate($form, Constraint $formConstraint)
}
}
}
} else {
} elseif (!$form->isSynchronized()) {
$childrenSynchronized = true;

/** @var FormInterface $child */
Expand Down
Expand Up @@ -273,9 +273,6 @@ private function reconstructPath(ViolationPath $violationPath, FormInterface $or
*/
private function acceptsErrors(FormInterface $form)
{
// Ignore non-submitted forms. This happens, for example, in PATCH
// requests.
// https://github.com/symfony/symfony/pull/10567
return $form->isSubmitted() && ($this->allowNonSynchronized || $form->isSynchronized());
return $this->allowNonSynchronized || $form->isSynchronized();
}
}
Expand Up @@ -61,9 +61,8 @@ public function testValidate()
{
$object = new \stdClass();
$options = ['validation_groups' => ['group1', 'group2']];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);

Expand All @@ -82,9 +81,8 @@ public function testValidateConstraints()
'validation_groups' => ['group1', 'group2'],
'constraints' => [$constraint1, $constraint2],
];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$form->submit([]);

// First default constraints
$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
Expand All @@ -110,10 +108,9 @@ public function testValidateChildIfValidConstraint()
'validation_groups' => ['group1', 'group2'],
'constraints' => [new Valid()],
];
$form = $this->getBuilder('name', '\stdClass', $options)->getForm();
$form = $this->getCompoundForm($object, $options);
$parent->add($form);

$form->setData($object);
$parent->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);

Expand Down Expand Up @@ -146,8 +143,8 @@ public function testDontValidateIfParentWithoutValidConstraint()
public function testMissingConstraintIndex()
{
$object = new \stdClass();
$form = new FormBuilder('name', '\stdClass', $this->dispatcher, $this->factory);
$form = $form->setData($object)->getForm();
$form = $this->getCompoundForm($object);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, ['Default']);

Expand All @@ -170,10 +167,9 @@ public function testValidateConstraintsOptionEvenIfNoValidConstraint()
'validation_groups' => ['group1', 'group2'],
'constraints' => [$constraint1, $constraint2],
];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$parent->add($form);
$parent->submit([]);

$this->expectValidateValueAt(0, 'data', $object, $constraint1, 'group1');
$this->expectValidateValueAt(1, 'data', $object, $constraint2, 'group2');
Expand Down Expand Up @@ -361,9 +357,8 @@ public function testHandleGroupSequenceValidationGroups()
{
$object = new \stdClass();
$options = ['validation_groups' => new GroupSequence(['group1', 'group2'])];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, new GroupSequence(['group1', 'group2']));
$this->expectValidateAt(1, 'data', $object, new GroupSequence(['group1', 'group2']));
Expand All @@ -377,9 +372,8 @@ public function testHandleCallbackValidationGroups()
{
$object = new \stdClass();
$options = ['validation_groups' => [$this, 'getValidationGroups']];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);

Expand All @@ -392,9 +386,8 @@ public function testDontExecuteFunctionNames()
{
$object = new \stdClass();
$options = ['validation_groups' => 'header'];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, ['header']);

Expand All @@ -409,9 +402,8 @@ public function testHandleClosureValidationGroups()
$options = ['validation_groups' => function (FormInterface $form) {
return ['group1', 'group2'];
}];
$form = $this->getBuilder('name', '\stdClass', $options)
->setData($object)
->getForm();
$form = $this->getCompoundForm($object, $options);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);

Expand Down Expand Up @@ -455,7 +447,7 @@ public function testDontUseValidationGroupOfUnclickedButton()
->setCompound(true)
->setDataMapper(new PropertyPathMapper())
->getForm();
$form = $this->getForm('name', '\stdClass', [
$form = $this->getCompoundForm($object, [
'validation_groups' => 'form_group',
'constraints' => [new Valid()],
]);
Expand All @@ -465,7 +457,7 @@ public function testDontUseValidationGroupOfUnclickedButton()
'validation_groups' => 'button_group',
]));

$form->setData($object);
$parent->submit([]);

$this->expectValidateAt(0, 'data', $object, ['form_group']);

Expand All @@ -484,10 +476,9 @@ public function testUseInheritedValidationGroup()
->setDataMapper(new PropertyPathMapper())
->getForm();
$formOptions = ['constraints' => [new Valid()]];
$form = $this->getBuilder('name', '\stdClass', $formOptions)->getForm();
$form = $this->getCompoundForm($object, $formOptions);
$parent->add($form);

$form->setData($object);
$parent->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group']);

Expand All @@ -506,10 +497,9 @@ public function testUseInheritedCallbackValidationGroup()
->setDataMapper(new PropertyPathMapper())
->getForm();
$formOptions = ['constraints' => [new Valid()]];
$form = $this->getBuilder('name', '\stdClass', $formOptions)->getForm();
$form = $this->getCompoundForm($object, $formOptions);
$parent->add($form);

$form->setData($object);
$parent->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);

Expand All @@ -523,7 +513,7 @@ public function testUseInheritedClosureValidationGroup()
$object = new \stdClass();

$parentOptions = [
'validation_groups' => function (FormInterface $form) {
'validation_groups' => function () {
return ['group1', 'group2'];
},
];
Expand All @@ -532,10 +522,9 @@ public function testUseInheritedClosureValidationGroup()
->setDataMapper(new PropertyPathMapper())
->getForm();
$formOptions = ['constraints' => [new Valid()]];
$form = $this->getBuilder('name', '\stdClass', $formOptions)->getForm();
$form = $this->getCompoundForm($object, $formOptions);
$parent->add($form);

$form->setData($object);
$parent->submit([]);

$this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);

Expand All @@ -547,9 +536,8 @@ public function testUseInheritedClosureValidationGroup()
public function testAppendPropertyPath()
{
$object = new \stdClass();
$form = $this->getBuilder('name', '\stdClass')
->setData($object)
->getForm();
$form = $this->getCompoundForm($object);
$form->submit([]);

$this->expectValidateAt(0, 'data', $object, ['Default']);

Expand Down Expand Up @@ -690,6 +678,15 @@ private function getForm($name = 'name', $dataClass = null, array $options = [])
return $this->getBuilder($name, $dataClass, $options)->getForm();
}

private function getCompoundForm($data, array $options = [])
{
return $this->getBuilder('name', \get_class($data), $options)
->setData($data)
->setCompound(true)
->setDataMapper(new PropertyPathMapper())
->getForm();
}

private function getSubmitButton($name = 'name', array $options = [])
{
$builder = new SubmitButtonBuilder($name, $options);
Expand Down
Expand Up @@ -12,12 +12,19 @@
namespace Symfony\Component\Form\Tests\Extension\Validator;

use PHPUnit\Framework\TestCase;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Validator\Constraints\Form as FormConstraint;
use Symfony\Component\Form\Extension\Validator\ValidatorExtension;
use Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser;
use Symfony\Component\Form\Form;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormFactoryBuilder;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Mapping\CascadingStrategy;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory;
use Symfony\Component\Validator\Mapping\Loader\StaticMethodLoader;
use Symfony\Component\Validator\Mapping\TraversalStrategy;
use Symfony\Component\Validator\Tests\Fixtures\FakeMetadataFactory;
use Symfony\Component\Validator\Validation;
Expand Down Expand Up @@ -45,4 +52,78 @@ public function test2Dot5ValidationApi()
$this->assertSame(CascadingStrategy::CASCADE, $metadata->getPropertyMetadata('children')[0]->cascadingStrategy);
$this->assertSame(TraversalStrategy::IMPLICIT, $metadata->getPropertyMetadata('children')[0]->traversalStrategy);
}

public function testDataConstraintsInvalidateFormEvenIfFieldIsNotSubmitted()
{
$form = $this->createForm(FooType::class);
$form->submit(['baz' => 'foobar'], false);

$this->assertTrue($form->isSubmitted());
$this->assertFalse($form->isValid());
$this->assertFalse($form->get('bar')->isSubmitted());
$this->assertCount(1, $form->get('bar')->getErrors());
}

public function testFieldConstraintsDoNotInvalidateFormIfFieldIsNotSubmitted()
{
$form = $this->createForm(FooType::class);
$form->submit(['bar' => 'foobar'], false);

$this->assertTrue($form->isSubmitted());
$this->assertTrue($form->isValid());
}

public function testFieldConstraintsInvalidateFormIfFieldIsSubmitted()
{
$form = $this->createForm(FooType::class);
$form->submit(['bar' => 'foobar', 'baz' => ''], false);

$this->assertTrue($form->isSubmitted());
$this->assertFalse($form->isValid());
$this->assertTrue($form->get('bar')->isSubmitted());
$this->assertTrue($form->get('bar')->isValid());
$this->assertTrue($form->get('baz')->isSubmitted());
$this->assertFalse($form->get('baz')->isValid());
}

private function createForm($type)
{
$validator = Validation::createValidatorBuilder()
->setMetadataFactory(new LazyLoadingMetadataFactory(new StaticMethodLoader()))
->getValidator();
$formFactoryBuilder = new FormFactoryBuilder();
$formFactoryBuilder->addExtension(new ValidatorExtension($validator));
$formFactory = $formFactoryBuilder->getFormFactory();

return $formFactory->create($type);
}
}

class Foo
{
public $bar;
public $baz;

public static function loadValidatorMetadata(ClassMetadata $metadata)
{
$metadata->addPropertyConstraint('bar', new NotBlank());
}
}

class FooType extends AbstractType
{
public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder
->add('bar')
->add('baz', null, [
'constraints' => [new NotBlank()],
])
;
}

public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('data_class', Foo::class);
}
}
Expand Up @@ -205,7 +205,7 @@ public function testAbortDotRuleMappingIfNotSynchronized()
$this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one');
}

public function testAbortMappingIfNotSubmitted()
public function testMappingIfNotSubmitted()
{
$violation = $this->getConstraintViolation('children[address].data.street');
$parent = $this->getForm('parent');
Expand All @@ -225,10 +225,10 @@ public function testAbortMappingIfNotSubmitted()

$this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one');
$this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error, but has one');
$this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one');
$this->assertCount(1, $grandChild->getErrors(), $grandChild->getName().' should have one error');
}

public function testAbortDotRuleMappingIfNotSubmitted()
public function testDotRuleMappingIfNotSubmitted()
{
$violation = $this->getConstraintViolation('data.address');
$parent = $this->getForm('parent');
Expand All @@ -250,7 +250,7 @@ public function testAbortDotRuleMappingIfNotSubmitted()

$this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one');
$this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error, but has one');
$this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one');
$this->assertCount(1, $grandChild->getErrors(), $grandChild->getName().' should have one error');
}

public function provideDefaultTests()
Expand Down

0 comments on commit a60d802

Please sign in to comment.