Skip to content

[Form] Fixed potential BC break on submitting data to disabled child form #38025

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

Closed
wants to merge 3 commits into from
Closed

[Form] Fixed potential BC break on submitting data to disabled child form #38025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dabe770
Fixed potential BC break on submitting data to disabled child form
yceruto Sep 1, 2020
b1099d2
added functional tests covering other validation methods
yceruto Sep 1, 2020
26025a4
test underlying object cannot change on submit if form is disabled
yceruto Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions src/Symfony/Component/Form/Form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,22 +508,24 @@ public function submit($submittedData, bool $clearMissing = true)
$dispatcher = $this->config->getEventDispatcher();

// Obviously, a disabled form should not change its data upon submission.
if ($this->isDisabled() && $this->isRoot()) {
if ($this->isDisabled()) {
$this->submitted = true;

if ($dispatcher->hasListeners(FormEvents::PRE_SUBMIT)) {
$event = new FormEvent($this, $submittedData);
$dispatcher->dispatch(FormEvents::PRE_SUBMIT, $event);
}
if ($this->isRoot()) {
if ($dispatcher->hasListeners(FormEvents::PRE_SUBMIT)) {
$event = new FormEvent($this, $submittedData);
$dispatcher->dispatch($event, FormEvents::PRE_SUBMIT);
}

if ($dispatcher->hasListeners(FormEvents::SUBMIT)) {
$event = new FormEvent($this, $this->getNormData());
$dispatcher->dispatch(FormEvents::SUBMIT, $event);
}
if ($dispatcher->hasListeners(FormEvents::SUBMIT)) {
$event = new FormEvent($this, $this->getNormData());
$dispatcher->dispatch($event, FormEvents::SUBMIT);
Copy link
Member Author

@yceruto yceruto Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added another test covering this case where the underlying object has been changed. This shouldn't happen if the entire form is disabled. Also this would break BC.

https://travis-ci.org/github/symfony/symfony/jobs/723435931#L3539

}

if ($dispatcher->hasListeners(FormEvents::POST_SUBMIT)) {
$event = new FormEvent($this, $this->getViewData());
$dispatcher->dispatch(FormEvents::POST_SUBMIT, $event);
if ($dispatcher->hasListeners(FormEvents::POST_SUBMIT)) {
$event = new FormEvent($this, $this->getViewData());
$dispatcher->dispatch($event, FormEvents::POST_SUBMIT);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here.

}
}

return $this;
Expand Down
39 changes: 38 additions & 1 deletion src/Symfony/Component/Form/Tests/CompoundFormTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public function testInvalidIfChildIsInvalid()
$this->assertFalse($this->form->isValid());
}

public function testDisabledFormsInvalidEvenChildrenInvalid()
public function testDisabledFormIsInvalidIfChildrenInvalid()
{
$form = $this->getBuilder('person')
->setDisabled(true)
Expand All @@ -71,6 +71,43 @@ public function testDisabledFormsInvalidEvenChildrenInvalid()
$this->assertFalse($form->isValid());
}

public function testUnderlyingObjectCannotChangeOnSubmitIfDisabledForm()
{
$person = new \stdClass();
$person->name = 'John Doe';

$form = $this->getBuilder('person', null, \stdClass::class)
->setData($person)
->setDisabled(true)
->setCompound(true)
->setDataMapper($this->getDataMapper())
->addEventListener(FormEvents::SUBMIT, static function (FormEvent $event) {
$event->getData()->name = 'Jane';
})
->add($this->getBuilder('name'))
->getForm();

$form->submit(['name' => 'Jacques Doe']);

$this->assertTrue($form->isValid());
$this->assertSame('John Doe', $person->name);
}

public function testDisabledChildFormCannotChangeOnSubmit()
{
$form = $this->getBuilder('person')
->setCompound(true)
->setDataMapper($this->getDataMapper())
->add($this->getBuilder('name')->setDisabled(true))
->getForm();

$this->assertNull($form->get('name')->getData());

$form->submit(['name' => 'Jacques Doe']);

$this->assertNull($form->get('name')->getData());
}

public function testSubmitForwardsNullIfNotClearMissingButValueIsExplicitlyNull()
{
$child = $this->createForm('firstName', false);
Expand Down
38 changes: 38 additions & 0 deletions ...fony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormFactoryBuilder;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Validator\Constraints\Callback;
use Symfony\Component\Validator\Constraints\Collection;
use Symfony\Component\Validator\Constraints\Expression;
use Symfony\Component\Validator\Constraints\GreaterThanOrEqual;
use Symfony\Component\Validator\Constraints\GroupSequence;
use Symfony\Component\Validator\Constraints\Length;
use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Context\ExecutionContextInterface;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory;
use Symfony\Component\Validator\Mapping\Loader\StaticMethodLoader;
Expand Down Expand Up @@ -386,6 +389,41 @@ function () {
$this->assertFalse($form->get('field2')->isValid());
$this->assertCount(1, $form->get('field2')->getErrors());
}

public function testDisabledFormIsInvalidIfRootFormValidationFails()
{
$form = $this->formFactory
->createBuilder(FormType::class, ['field1' => 0], [
'constraints' => new Callback(['callback' => static function ($data, ExecutionContextInterface $context) {
Copy link
Member Author

@yceruto yceruto Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though constraints violation linked to the root form are working correctly.

if ($data['field1'] < 1) {
$context->addViolation('Invalid');
}
}]),
])
->setDisabled(true)
->add('field1')
->getForm();

$form->submit(null);

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

public function testDisabledFormIsInvalidIfChildrenValidationFails()
{
$form = $this->formFactory->createBuilder(FormType::class, ['field1' => 0])
->setDisabled(true)
->add('field1', null, ['constraints' => new GreaterThanOrEqual(1)])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this kind of validation (tied directly to the form field) doesn't work because form fields will require to be marked as submitted too:

symfony/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php

Lines 85 to 86 in 4e0b7e5

foreach ($form->all() as $field) {
if ($field->isSubmitted()) {

and currently only the root form is being marked as "submitted".

->getForm();

$form->submit(null);

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

class Foo
Expand Down