8000 [Form] Fixed empty data on expanded ChoiceType and FileType by HeahDude · Pull Request #25948 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fixed empty data on expanded ChoiceType and FileType #25948

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

Merged
merged 1 commit into from
Feb 1, 2018
Merged

[Form] Fixed empty data on expanded ChoiceType and FileType #25948

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
use Symfony\Component\Form\Extension\Core\EventListener\MergeCollectionListener;
use Symfony\Component\Form\Extension\Core\DataTransformer\ChoiceToValueTransformer;
use Symfony\Component\Form\Extension\Core\DataTransformer\ChoicesToValuesTransformer;
use Symfony\Component\Form\Util\FormUtil;
use Symfony\Component\OptionsResolver\Options;
use Symfony\Component\OptionsResolver\OptionsResolver;

Expand Down Expand Up @@ -91,12 +90,12 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$form = $event->getForm();
$data = $event->getData();

// Since the type always use mapper an empty array will not be
// considered as empty in Form::submit(), we need to evaluate
// empty data here so its value is submitted to sub forms
if (null === $data) {
$emptyData = $form->getConfig()->getEmptyData();

if (false === FormUtil::isEmpty($emptyData) && array() !== $emptyData) {
$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
}
$data = $emptyData instanceof \Closure ? $emptyData($form, $data) : $emptyData;
Copy link
Member

Choose a reason for hiding this comment

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

is instanceof \Closure legit instead of is_callable (same below?)

Copy link
Contributor Author
@HeahDude HeahDude Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is totally legit and what must be, this is the point of this bug fix. Only a \Closure should be considered here, nothing else.

The view data can be:

  • an object (whose class must be the same as defined by the data_class option)
  • an array
  • a scalar

The three of them can be callable:

  • the object can be invokable
  • the array can be a class or an object with a method name
  • the string a function name (which triggered the related issue)

Deprecating a broken behavior on master does not make any sense to me,
I really think we need to merge this PR as a bug fix, because the previous one was wrong.

}

// Convert the submitted data to a string, if scalar, before
Expand Down
15 changes: 6 additions & 9 deletions src/Symfony/Component/Form/Extension/Core/Type/FileType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ class FileType extends AbstractType
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
// Ensure that submitted data is always an uploaded file or an array of some
$builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) use ($options) {
$form = $event->getForm();
$requestHandler = $form->getConfig()->getRequestHandler();
$data = null;

if ($options['multiple']) {
$data = array();
Expand All @@ -46,19 +46,16 @@ public function buildForm(FormBuilderInterface $builder, array $options)
}
}

// submitted data for an input file (not required) without choosing any file
if (array(null) === $data || array() === $data) {
// Since the array is never considered empty in the view data format
// on submission, we need to evaluate the configured empty data here
if (array() === $data) {
$emptyData = $form->getConfig()->getEmptyData();

$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
$data = $emptyData instanceof \Closure ? $emptyData($form, $data) : $emptyData;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this bug fix was wrong as well by replicating the first.

}

$event->setData($data);
} elseif (!$requestHandler->isFileUpload($event->getData())) {
$emptyData = $form->getConfig()->getEmptyData();

$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
$event->setData($data);
$event->setData(null);
Copy link
Contributor Author
@HeahDude HeahDude Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

And that this is a BC break introduced in #24993 which does not count as the one which adds a method to the RequestHandlerInterface introduced in that same PR, considering the intended behavior.

}
});
}
Expand Down
90 changes: 90 additions & 0 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,21 @@ public function testSubmitSingleChoiceWithEmptyData()
$this->assertSame('test', $form->getData());
}

public function testSubmitSingleChoiceWithEmptyDataAndInitialData()
{
$form = $this->factory->create(static::TESTED_TYPE, 'initial', array(
'multiple' => false,
'expanded' => false,
'choices' => array('initial', 'test'),
'choices_as_values' => true,
'empty_data' => 'test',
));

$form->submit(null);

$this->assertSame('test', $form->getData());
}

public function testSubmitMultipleChoiceWithEmptyData()
{
$form = $this->factory->create(static::TESTED_TYPE, null, array(
Expand All @@ -705,6 +720,36 @@ public function testSubmitMultipleChoiceWithEmptyData()
$this->assertSame(array('test'), $form->getData());
}

public function testSubmitMultipleChoiceWithEmptyDataAndInitialEmptyArray()
{
$form = $this->factory->create(static::TESTED_TYPE, array(), array(
'multiple' => true,
'expanded' => false,
'choices' => array('test'),
'choices_as_values' => true,
'empty_data' => array('test'),
));

$form->submit(null);

$this->assertSame(array('test'), $form->getData());
}

public function testSubmitMultipleChoiceWithEmptyDataAndInitialData()
{
$form = $this->factory->create(static::TESTED_TYPE, array('initial'), array(
'multiple' => true,
'expanded' => false,
'choices' => array('initial', 'test'),
'choices_as_values' => true,
'empty_data' => array('test'),
));

$form->submit(null);

$this->assertSame(array('test'), $form->getData());
}

public function testSubmitSingleChoiceExpandedWithEmptyData()
{
$form = $this->factory->create(static::TESTED_TYPE, null, array(
Expand All @@ -720,6 +765,21 @@ public function testSubmitSingleChoiceExpandedWithEmptyData()
$this->assertSame('test', $form->getData());
}

public function testSubmitSingleChoiceExpandedWithEmptyDataAndInitialData()
{
$form = $this->factory->create(static::TESTED_TYPE, 'initial', array(
'multiple' => false,
'expanded' => true,
'choices' => array('initial', 'test'),
'choices_as_values' => true,
'empty_data' => 'test',
));

$form->submit(null);

$this->assertSame('test', $form->getData());
}

public function testSubmitMultipleChoiceExpandedWithEmptyData()
{
$form = $this->factory->create(static::TESTED_TYPE, null, array(
Expand All @@ -735,6 +795,36 @@ public function testSubmitMultipleChoiceExpandedWithEmptyData()
$this->assertSame(array('test'), $form->getData());
}

public function testSubmitMultipleChoiceExpandedWithEmptyDataAndInitialEmptyArray()
{
$form = $this->factory->create(static::TESTED_TYPE, array(), array(
'multiple' => true,
'expanded' => true,
'choices' => array('test'),
'choices_as_values' => true,
'empty_data' => array('test'),
));

$form->submit(null);

$this->assertSame(array('test'), $form->getData());
}

public function testSubmitMultipleChoiceExpandedWithEmptyDataAndInitialData()
{
$form = $this->factory->create(static::TESTED_TYPE, array('init'), array(
'multiple' => true,
'expanded' => true,
'choices' => array('init', 'test'),
'choices_as_values' => true,
'empty_data' => array('test'),
));

$form->submit(null);

$this->assertSame(array('test'), $form->getData());
}

/**
* @group legacy
*/
Expand Down
0