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.

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

[Form] weaken the rejection of submitted array data #29911

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php
Expand Up @@ -1521,4 +1521,18 @@ public function testSubmitNullMultipleUsesDefaultEmptyData()
$this->assertEquals($collection, $form->getNormData());
$this->assertEquals($collection, $form->getData());
}

public function testSubmitArray()
{
$entity1 = new SingleIntIdEntity(1, 'Foo');
$this->persist([$entity1]);

$form = $this->factory->create(static::TESTED_TYPE, null, [
'em' => 'default',
'class' => self::SINGLE_IDENT_CLASS,
]);
$form->submit([]);

$this->assertFalse($form->isSynchronized());
}
}
Expand Up @@ -59,6 +59,7 @@ public function configureOptions(OptionsResolver $resolver)
'value' => '1',
'empty_data' => $emptyData,
'compound' => false,
'accept_multiple_values' => false,
]);
}

Expand Down
Expand Up @@ -328,6 +328,8 @@ public function configureOptions(OptionsResolver $resolver)
'placeholder' => $placeholderDefault,
'error_bubbling' => false,
'compound' => $compound,
// submitted arrays are dealt with in data transformers
'accept_multiple_values' => true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if that's the best name. I could also imagine something like accept_array_data.

Copy link

@YetiCGN YetiCGN Jan 17, 2019

Choose a reason for hiding this comment

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

Suggestion: accept_unstructured_data? "Array" could be mistaken for a flat array without special string keys, just like multiple values on a TextType.

Copy link

Choose a reason for hiding this comment

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

'scalar' => false?

// The view data is always a string, even if the "data" option
// is manually set to an object.
// See https://github.com/symfony/symfony/pull/5582
Expand Down
Expand Up @@ -12,9 +12,18 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class ColorType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('accept_multiple_values', false);
}

/**
* {@inheritdoc}
*/
Expand Down
Expand Up @@ -233,6 +233,9 @@ public function configureOptions(OptionsResolver $resolver)
// this option.
'data_class' => null,
'compound' => $compound,
'accept_multiple_values' => function (Options $options) {
return $options['compound'];
},
'empty_data' => $emptyData,
'labels' => [],
]
Expand Down
Expand Up @@ -235,6 +235,9 @@ public function configureOptions(OptionsResolver $resolver)
// this option.
'data_class' => null,
'compound' => $compound,
'accept_multiple_values' => function (Options $options) {
return $options['compound'];
},
'empty_data' => function (Options $options) {
return $options['compound'] ? [] : '';
},
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/DateType.php
Expand Up @@ -276,6 +276,9 @@ public function configureOptions(OptionsResolver $resolver)
// this option.
'data_class' => null,
'compound' => $compound,
'accept_multiple_values' => function (Options $options) {
return $options['compound'];
},
'empty_data' => function (Options $options) {
return $options['compound'] ? [] : '';
},
Expand Down
Expand Up @@ -12,9 +12,18 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class EmailType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('accept_multiple_values', false);
}

/**
* {@inheritdoc}
*/
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/FileType.php
Expand Up @@ -105,6 +105,8 @@ public function configureOptions(OptionsResolver $resolver)
'data_class' => $dataClass,
'empty_data' => $emptyData,
'multiple' => false,
// invalid submitted arrays will be detected when the submitted data are processed by the type
'accept_multiple_values' => true,
'allow_file_upload' => true,
]);
}
Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/FormType.php
Expand Up @@ -179,10 +179,18 @@ public function configureOptions(OptionsResolver $resolver)
'post_max_size_message' => 'The uploaded file was too large. Please try to upload a smaller file.',
'upload_max_size_message' => $uploadMaxSizeMessage, // internal
'allow_file_upload' => false,
'accept_multiple_values' => function (Options $options) {
if ($options['compound']) {
return true;
}

return null;
},
]);

$resolver->setAllowedTypes('label_attr', 'array');
$resolver->setAllowedTypes('upload_max_size_message', ['callable']);
$resolver->setAllowedTypes('accept_multiple_values', ['bool', 'null']);
}

/**
Expand Down
Expand Up @@ -27,6 +27,7 @@ public function configureOptions(OptionsResolver $resolver)
// Pass errors to the parent
'error_bubbling' => true,
'compound' => false,
'accept_multiple_values' => false,
]);
}

Expand Down
Expand Up @@ -43,6 +43,7 @@ public function configureOptions(OptionsResolver $resolver)
// Integer cast rounds towards 0, so do the same when displaying fractions
'rounding_mode' => IntegerToLocalizedStringTransformer::ROUND_DOWN,
'compound' => false,
'accept_multiple_values' => false,
]);

$resolver->setAllowedValues('rounding_mode', [
Expand Down
Expand Up @@ -56,6 +56,7 @@ public function configureOptions(OptionsResolver $resolver)
'divisor' => 1,
'currency' => 'EUR',
'compound' => false,
'accept_multiple_values' => false,
]);

$resolver->setAllowedTypes('scale', 'int');
Expand Down
Expand Up @@ -41,6 +41,7 @@ public function configureOptions(OptionsResolver $resolver)
'grouping' => false,
'rounding_mode' => NumberToLocalizedStringTransformer::ROUND_HALF_UP,
'compound' => false,
'accept_multiple_values' => false,
]);

$resolver->setAllowedValues('rounding_mode', [
Expand Down
Expand Up @@ -35,6 +35,7 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults([
'always_empty' => true,
'accept_multiple_values' => false,
'trim' => false,
]);
}
Expand Down
Expand Up @@ -35,6 +35,7 @@ public function configureOptions(OptionsResolver $resolver)
'scale' => 0,
'type' => 'fractional',
'compound' => false,
'accept_multiple_values' => false,
]);

$resolver->setAllowedValues('type', [
Expand Down
Expand Up @@ -12,9 +12,18 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class RangeType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('accept_multiple_values', false);
}

/**
* {@inheritdoc}
*/
Expand Down
Expand Up @@ -12,9 +12,18 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class SearchType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('accept_multiple_values', false);
}

/**
* {@inheritdoc}
*/
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/TelType.php
Expand Up @@ -12,9 +12,18 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class TelType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('accept_multiple_values', false);
}

/**
* {@inheritdoc}
*/
Expand Down
Expand Up @@ -37,6 +37,7 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults([
'compound' => false,
'accept_multiple_values' => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

@xabbuh this should set to true until 5.0 to avoid the bc break which was introduce in 3.4.21, else applications which did work before 3.4.21 still would crash.

]);
}

Expand Down
Expand Up @@ -14,6 +14,7 @@
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\OptionsResolver\OptionsResolver;

class TextareaType extends AbstractType
{
Expand All @@ -25,6 +26,14 @@ public function buildView(FormView $view, FormInterface $form, array $options)
$view->vars['pattern'] = null;
}

/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('accept_multiple_values', false);
}

/**
* {@inheritdoc}
*/
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/TimeType.php
Expand Up @@ -273,6 +273,9 @@ public function configureOptions(OptionsResolver $resolver)
return $options['compound'] ? [] : '';
},
'compound' => $compound,
'accept_multiple_values' => function (Options $options) {
return $options['compound'];
},
'choice_translation_domain' => false,
]);

Expand Down
Expand Up @@ -34,6 +34,7 @@ public function buildForm(FormBuilderInterface $builder, array $options)
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('default_protocol', 'http');
$resolver->setDefault('accept_multiple_values', false);

$resolver->setAllowedTypes('default_protocol', ['null', 'string']);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Form/Form.php
Expand Up @@ -535,7 +535,7 @@ public function submit($submittedData, $clearMissing = true)
} elseif (!$this->config->getOption('allow_file_upload') && $this->config->getRequestHandler()->isFileUpload($submittedData)) {
$submittedData = null;
$this->transformationFailure = new TransformationFailedException('Submitted data was expected to be text or number, file upload given.');
} elseif (\is_array($submittedData) && !$this->config->getCompound() && !$this->config->hasOption('multiple')) {
} elseif (\is_array($submittedData) && !$this->config->getCompound() && false === $this->config->getOption('accept_multiple_values', null)) {
$submittedData = null;
$this->transformationFailure = new TransformationFailedException('Submitted data was expected to be text or number, array given.');
}
Expand Down
44 changes: 37 additions & 7 deletions src/Symfony/Component/Form/Tests/CompoundFormTest.php
Expand Up @@ -1036,20 +1036,50 @@ public function testDisabledButtonIsNotSubmitted()
$this->assertFalse($submit->isSubmitted());
}

public function testArrayTransformationFailureOnSubmit()
public function testArrayTransformationFailureOnSubmitWhenMultipleValuesNotAccepted()
{
$this->form->add($this->getBuilder('foo')->setCompound(false)->getForm());
$this->form->add($this->getBuilder('bar', null, null, ['multiple' => false])->setCompound(false)->getForm());

$this->form->add($this->getBuilder('foo', null, null, ['accept_multiple_values' => false])->getForm());
$this->form->submit([
'foo' => ['foo'],
'bar' => ['bar'],
'foo' => [
'foo',
'bar' => [
'baz' => 'baz',
],
],
]);

$this->assertNull($this->form->get('foo')->getData());
$this->assertSame('Submitted data was expected to be text or number, array given.', $this->form->get('foo')->getTransformationFailure()->getMessage());
}

public function testArrayDataIsAllowedWhenFormIsCompound()
{
$this->form->add($this->getBuilder('foo', null, null, ['compound' => true])->getForm());
$this->form->submit([
'foo' => [
'foo',
'bar' => [
'baz' => 'baz',
],
],
]);

$this->assertSame(['foo', 'bar' => ['baz' => 'baz']], $this->form->get('foo')->getData());
}

public function testArrayDataIsAllowedWhenNotExplicitlyForbidden()
{
$this->form->add($this->getBuilder('foo')->getForm());
$this->form->submit([
'foo' => [
'foo',
'bar' => [
'baz' => 'baz',
],
],
]);

$this->assertSame(['bar'], $this->form->get('bar')->getData());
$this->assertSame(['foo', 'bar' => ['baz' => 'baz']], $this->form->get('foo')->getData());
}

public function testFileUpload()
Expand Down
Expand Up @@ -167,6 +167,14 @@ public function testSubmitNullUsesDefaultEmptyData($emptyData = 'empty', $expect
$this->assertSame($expectedData, $form->getData());
}

public function testSubmitArray()
{
$form = $this->factory->create($this->getTestedType());
$form->submit([]);

$this->assertSame($form->getConfig()->getOption('accept_multiple_values'), $form->isSynchronized());
}

protected function getTestedType()
{
return static::TESTED_TYPE;
Expand Down
Expand Up @@ -34,4 +34,9 @@ public function testSubmitNullUsesDefaultEmptyData($emptyData = 'empty', $expect
{
parent::testSubmitNullUsesDefaultEmptyData($emptyData, $expectedData);
}

public function testSubmitArray()
{
$this->markTestSkipped();
}
}
Expand Up @@ -190,4 +190,9 @@ public function testSubmitNullUsesDefaultEmptyData($emptyData = 'empty', $expect
$this->assertSame($expectedData, $form->getNormData());
$this->assertSame($expectedData, $form->getData());
}

protected function supportsMultipleValues()
{
return false;
}
}