Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Form] do not validate non-submitted form fields in PATCH requests #30265

Merged
merged 1 commit into from Feb 21, 2019
Merged

[Form] do not validate non-submitted form fields in PATCH requests #30265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
View file
Open in desktop
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
5 changes: 1 addition & 4 deletions src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php
View file
Open in desktop
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();
}
}
79 changes: 38 additions & 41 deletions src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php
View file
Open in desktop
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
81 changes: 81 additions & 0 deletions src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php
View file
Open in desktop
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);
}
}
8 changes: 4 additions & 4 deletions src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php
View file
Open in desktop
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