diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
This PR was merged into the 3.4 branch.

Discussion
----------

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

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11493, #19788, #20805, #24453, #30011
| License       | MIT
| Doc PR        |

When a form field is not embedded as part of a HTTP PATCH requests, its
validation constraints configured through the `constraints` option must
not be evaluated. The fix from #10567 achieved this by not mapping their
violations to the underlying form field. This however also means that
constraint violations caused by validating the whole underlying data
object will never cause the form to be invalid. This breaks use cases
where some constraints may, for example, depend on the value of other
properties that were changed by the submitted data.

Commits
-------

a60d802619 do not validate non-submitted form fields in PATCH requests
This commit is contained in:
Christian Flothmann 2019-02-21 08:31:48 +01:00
commit 191403119c
5 changed files with 126 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
View File

@ -41,7 +41,7 @@ class FormValidator extends ConstraintValidator
$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();
@ -90,7 +90,7 @@ class FormValidator extends ConstraintValidator
}
}
}
} else {
} elseif (!$form->isSynchronized()) {
$childrenSynchronized = true;
/** @var FormInterface $child */

5
src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php
View File

@ -273,9 +273,6 @@ class ViolationMapper implements ViolationMapperInterface
*/
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
src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php
View File

@ -61,9 +61,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
{
$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']);
@ -82,9 +81,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
'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']);
@ -110,10 +108,9 @@ class FormValidatorTest extends ConstraintValidatorTestCase
'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']);
@ -146,8 +143,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
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']);
@ -170,10 +167,9 @@ class FormValidatorTest extends ConstraintValidatorTestCase
'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');
@ -361,9 +357,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
{
$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']));
@ -377,9 +372,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
{
$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']);
@ -392,9 +386,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
{
$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']);
@ -409,9 +402,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
$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']);
@ -455,7 +447,7 @@ class FormValidatorTest extends ConstraintValidatorTestCase
->setCompound(true)
->setDataMapper(new PropertyPathMapper())
->getForm();
$form = $this->getForm('name', '\stdClass', [
$form = $this->getCompoundForm($object, [
'validation_groups' => 'form_group',
'constraints' => [new Valid()],
]);
@ -465,7 +457,7 @@ class FormValidatorTest extends ConstraintValidatorTestCase
'validation_groups' => 'button_group',
]));
$form->setData($object);
$parent->submit([]);
$this->expectValidateAt(0, 'data', $object, ['form_group']);
@ -484,10 +476,9 @@ class FormValidatorTest extends ConstraintValidatorTestCase
->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']);
@ -506,10 +497,9 @@ class FormValidatorTest extends ConstraintValidatorTestCase
->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']);
@ -523,7 +513,7 @@ class FormValidatorTest extends ConstraintValidatorTestCase
$object = new \stdClass();
$parentOptions = [
'validation_groups' => function (FormInterface $form) {
'validation_groups' => function () {
return ['group1', 'group2'];
},
];
@ -532,10 +522,9 @@ class FormValidatorTest extends ConstraintValidatorTestCase
->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']);
@ -547,9 +536,8 @@ class FormValidatorTest extends ConstraintValidatorTestCase
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']);
@ -690,6 +678,15 @@ class FormValidatorTest extends ConstraintValidatorTestCase
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);

81
src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php
View File

@ -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;
@ -45,4 +52,78 @@ class ValidatorExtensionTest extends TestCase
$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
src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php
View File

@ -205,7 +205,7 @@ class ViolationMapperTest extends TestCase
$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');
@ -225,10 +225,10 @@ class ViolationMapperTest extends TestCase
$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');
@ -250,7 +250,7 @@ class ViolationMapperTest extends TestCase
$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()