From b9facfc5aeb794b0f2d8655d8356b926c543ea01 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Tue, 7 Feb 2012 10:47:01 +0100 Subject: [PATCH 1/4] [Form] Removed undefined variables in exception constructor --- .../Core/DataTransformer/ChoicesToBooleanArrayTransformer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php index 715a91e3ff..5ec30926ad 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php +++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php @@ -107,7 +107,7 @@ class ChoicesToBooleanArrayTransformer implements DataTransformerInterface } if (count($unknown) > 0) { - throw new TransformationFailedException('The choices "' . implode('", "', $unknown, $code, $previous) . '" where not found'); + throw new TransformationFailedException('The choices "' . implode('", "', $unknown) . '" where not found'); } return $result; From 88ef52d27230c0b06d8b2e2e3e38c4934894e634 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Tue, 7 Feb 2012 10:51:21 +0100 Subject: [PATCH 2/4] [Form] Improved FormType::getDefaultOptions() to see default options defined in parent types In turn, FormType::getParent() does not see default options anymore. --- CHANGELOG-2.1.md | 2 + UPGRADE-2.1.md | 19 ++++++++ .../Doctrine/Form/Type/DoctrineType.php | 1 - .../Form/Extension/Core/Type/ChoiceType.php | 4 +- .../Form/Extension/Core/Type/DateTimeType.php | 2 +- .../Form/Extension/Core/Type/DateType.php | 2 +- .../Form/Extension/Core/Type/TimeType.php | 2 +- .../Form/Extension/Core/Type/TimezoneType.php | 2 +- src/Symfony/Component/Form/FormFactory.php | 47 ++++++++++++++----- 9 files changed, 62 insertions(+), 19 deletions(-) diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index 7f44c792e2..02b8140b69 100644 --- a/CHANGELOG-2.1.md +++ b/CHANGELOG-2.1.md @@ -202,6 +202,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * added options "add_method" and "remove_method" to collection and choice type * forms now don't create an empty object anymore if they are completely empty and not required. The empty value for such forms is null. + * FormType::getDefaultOptions() now sees default options defined by parent types + * [BC BREAK] FormType::getParent() does not see default options anymore ### HttpFoundation diff --git a/UPGRADE-2.1.md b/UPGRADE-2.1.md index 1e16654334..c47d5f977c 100644 --- a/UPGRADE-2.1.md +++ b/UPGRADE-2.1.md @@ -229,3 +229,22 @@ UPGRADE FROM 2.0 to 2.1 return false; } } + +* The options passed to `getParent` of the form types don't contain default + options anymore + + You should check with `isset` if options exist before checking their value. + + Before: + + public function getParent() + { + return 'single_text' === $options['widget'] ? 'text' : 'choice'; + } + + After: + + public function getParent() + { + return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice'; + } diff --git a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php index 1c634fb5b0..2b9161477e 100644 --- a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php +++ b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php @@ -50,7 +50,6 @@ abstract class DoctrineType extends AbstractType 'property' => null, 'query_builder' => null, 'loader' => null, - 'choices' => null, 'group_by' => null, ); diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index ff4a4b5ee9..447ae6a1c8 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -149,7 +149,7 @@ class ChoiceType extends AbstractType 'multiple' => false, 'expanded' => false, 'choice_list' => null, - 'choices' => array(), + 'choices' => null, 'preferred_choices' => array(), 'value_strategy' => ChoiceList::GENERATE, 'index_strategy' => ChoiceList::GENERATE, @@ -166,7 +166,7 @@ class ChoiceType extends AbstractType */ public function getParent(array $options) { - return $options['expanded'] ? 'form' : 'field'; + return isset($options['expanded']) && $options['expanded'] ? 'form' : 'field'; } /** diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php index 1c63d21641..7906a2ba22 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php @@ -200,7 +200,7 @@ class DateTimeType extends AbstractType */ public function getParent(array $options) { - return 'single_text' === $options['widget'] ? 'field' : 'form'; + return isset($options['widget']) && 'single_text' === $options['widget'] ? 'field' : 'form'; } /** diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateType.php index b98a222e3e..b8411fd6a3 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/DateType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/DateType.php @@ -211,7 +211,7 @@ class DateType extends AbstractType */ public function getParent(array $options) { - return 'single_text' === $options['widget'] ? 'field' : 'form'; + return isset($options['widget']) && 'single_text' === $options['widget'] ? 'field' : 'form'; } /** diff --git a/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php b/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php index 7bfe50dc6b..81465cb547 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php @@ -184,7 +184,7 @@ class TimeType extends AbstractType */ public function getParent(array $options) { - return 'single_text' === $options['widget'] ? 'field' : 'form'; + return isset($options['widget']) && 'single_text' === $options['widget'] ? 'field' : 'form'; } /** diff --git a/src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php b/src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php index ef308faeeb..8d1f74b3eb 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php @@ -31,7 +31,7 @@ class TimezoneType extends AbstractType 'value_strategy' => ChoiceList::COPY_CHOICE, ); - if (!isset($options['choice_list']) && !isset($options['choices'])) { + if (empty($options['choice_list']) && empty($options['choices'])) { $defaultOptions['choices'] = self::getTimezones(); } diff --git a/src/Symfony/Component/Form/FormFactory.php b/src/Symfony/Component/Form/FormFactory.php index 1f658303e8..e82e0567ed 100644 --- a/src/Symfony/Component/Form/FormFactory.php +++ b/src/Symfony/Component/Form/FormFactory.php @@ -213,16 +213,20 @@ class FormFactory implements FormFactoryInterface */ public function createNamedBuilder($type, $name, $data = null, array $options = array(), FormBuilder $parent = null) { - $builder = null; - $types = array(); - $knownOptions = array(); - $passedOptions = array_keys($options); - $optionValues = array(); - if (!array_key_exists('data', $options)) { $options['data'] = $data; } + $builder = null; + $types = array(); + $defaultOptions = array(); + $optionValues = array(); + $passedOptions = $options; + + // Bottom-up determination of the type hierarchy + // Start with the actual type and look for the parent type + // The complete hierarchy is saved in $types, the first entry being + // the root and the last entry being the leaf (the concrete type) while (null !== $type) { if ($type instanceof FormTypeInterface) { if ($type->getName() == $type->getParent($options)) { @@ -236,7 +240,23 @@ class FormFactory implements FormFactoryInterface throw new UnexpectedTypeException($type, 'string or Symfony\Component\Form\FormTypeInterface'); } - $defaultOptions = $type->getDefaultOptions($options); + array_unshift($types, $type); + + // getParent() cannot see default options set by this type nor + // default options set by parent types + // As a result, the options always have to be checked for + // existence with isset() before using them in this method. + $type = $type->getParent($options); + } + + // Top-down determination of the options and default options + foreach ($types as $type) { + // Merge the default options of all types to an array of default + // options. Default options of children override default options + // of parents. + // Default options of ancestors are already visible in the $options + // array passed to the following methods. + $defaultOptions = array_replace($defaultOptions, $type->getDefaultOptions($options)); $optionValues = array_merge_recursive($optionValues, $type->getAllowedOptionValues($options)); foreach ($type->getExtensions() as $typeExtension) { @@ -244,20 +264,23 @@ class FormFactory implements FormFactoryInterface $optionValues = array_merge_recursive($optionValues, $typeExtension->getAllowedOptionValues($options)); } - $options = array_replace($defaultOptions, $options); - $knownOptions = array_merge($knownOptions, array_keys($defaultOptions)); - array_unshift($types, $type); - $type = $type->getParent($options); + // In each turn, the options are replaced by the combination of + // the currently known default options and the passed options. + // It is important to merge with $passedOptions and not with + // $options, otherwise default options of parents would override + // default options of child types. + $options = array_replace($defaultOptions, $passedOptions); } $type = end($types); + $knownOptions = array_keys($defaultOptions); $diff = array_diff(self::$requiredOptions, $knownOptions); if (count($diff) > 0) { throw new TypeDefinitionException(sprintf('Type "%s" should support the option(s) "%s"', $type->getName(), implode('", "', $diff))); } - $diff = array_diff($passedOptions, $knownOptions); + $diff = array_diff(array_keys($passedOptions), $knownOptions); if (count($diff) > 1) { throw new CreationException(sprintf('The options "%s" do not exist. Known options are: "%s"', implode('", "', $diff), implode('", "', $knownOptions))); From 3b1b57030bbe92bd36d4fab974598c9b1f61e0dc Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Tue, 7 Feb 2012 11:10:02 +0100 Subject: [PATCH 3/4] [Form] Fixed: The "date", "time" and "datetime" types can be initialized with \DateTime objects --- .../Form/Extension/Core/Type/DateTimeType.php | 5 +++ .../Form/Extension/Core/Type/DateType.php | 5 +++ .../Form/Extension/Core/Type/TimeType.php | 5 +++ .../Extension/Core/Type/DateTimeTypeTest.php | 8 +++++ .../Form/Extension/Core/Type/DateTypeTest.php | 8 +++++ .../Extension/Core/Type/FieldTypeTest.php | 36 +++++++++++++++++++ .../Form/Extension/Core/Type/TimeTypeTest.php | 8 +++++ 7 files changed, 75 insertions(+) diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php index 7906a2ba22..ba6846007e 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php @@ -158,6 +158,11 @@ class DateTimeType extends AbstractType 'widget' => null, // This will overwrite "empty_value" child options 'empty_value' => null, + // If initialized with a \DateTime object, FieldType initializes + // this option to "\DateTime". Since the internal, normalized + // representation is not \DateTime, but an array, we need to unset + // this option. + 'data_class' => null, ); } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateType.php index b8411fd6a3..c477aa39e2 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/DateType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/DateType.php @@ -183,6 +183,11 @@ class DateType extends AbstractType // them like immutable value objects 'by_reference' => false, 'error_bubbling' => false, + // If initialized with a \DateTime object, FieldType initializes + // this option to "\DateTime". Since the internal, normalized + // representation is not \DateTime, but an array, we need to unset + // this option. + 'data_class' => null, ); } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php b/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php index 81465cb547..9ec85cd24e 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php @@ -156,6 +156,11 @@ class TimeType extends AbstractType // them like immutable value objects 'by_reference' => false, 'error_bubbling' => false, + // If initialized with a \DateTime object, FieldType initializes + // this option to "\DateTime". Since the internal, normalized + // representation is not \DateTime, but an array, we need to unset + // this option. + 'data_class' => null, ); } diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php index 27f3108d9a..2cd18ad4ef 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php @@ -258,4 +258,12 @@ class DateTimeTypeTest extends LocalizedTestCase $this->assertEquals(array(new FormError('Customized invalid message', array())), $form['date']->getErrors()); $this->assertEquals(array(new FormError('Customized invalid message', array())), $form['time']->getErrors()); } + + // Bug fix + public function testInitializeWithDateTime() + { + // Throws an exception if "data_class" option is not explicitely set + // to null in the type + $form = $this->factory->create('datetime', new \DateTime()); + } } diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php index 1be7affc70..8bd78fcf94 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php @@ -530,4 +530,12 @@ class DateTypeTest extends LocalizedTestCase $this->assertSame('single_text', $view->get('widget')); } + + // Bug fix + public function testInitializeWithDateTime() + { + // Throws an exception if "data_class" option is not explicitely set + // to null in the type + $form = $this->factory->create('date', new \DateTime()); + } } diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/FieldTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/FieldTypeTest.php index e170628351..036f829aa7 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/FieldTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/FieldTypeTest.php @@ -221,6 +221,42 @@ class FieldTypeTest extends TypeTestCase $this->assertEquals($author, $form->getData()); } + public function testBindWithEmptyDataCreatesObjectIfInitiallyBoundWithObject() + { + $form = $this->factory->create('form', null, array( + // data class is inferred from the passed object + 'data' => new Author(), + 'required' => false, + )); + $form->add($this->factory->createNamed('field', 'firstName')); + $form->add($this->factory->createNamed('field', 'lastName')); + + $form->setData(null); + // partially empty, still an object is created + $form->bind(array('firstName' => 'Bernhard', 'lastName' => '')); + + $author = new Author(); + $author->firstName = 'Bernhard'; + $author->setLastName(''); + + $this->assertEquals($author, $form->getData()); + } + + public function testBindWithEmptyDataDoesNotCreateObjectIfDataClassIsNull() + { + $form = $this->factory->create('form', null, array( + 'data' => new Author(), + 'data_class' => null, + 'required' => false, + )); + $form->add($this->factory->createNamed('field', 'firstName')); + + $form->setData(null); + $form->bind(array('firstName' => 'Bernhard')); + + $this->assertSame(array('firstName' => 'Bernhard'), $form->getData()); + } + public function testBindEmptyWithEmptyDataCreatesNoObjectIfNotRequired() { $form = $this->factory->create('form', null, array( diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php index 34676ddf19..89e250790a 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php @@ -401,4 +401,12 @@ class TimeTypeTest extends LocalizedTestCase $this->assertTrue($form->isPartiallyFilled()); } + + // Bug fix + public function testInitializeWithDateTime() + { + // Throws an exception if "data_class" option is not explicitely set + // to null in the type + $form = $this->factory->create('time', new \DateTime()); + } } From 22c8f8087cd7eb6179de1c4a473a76a339c16005 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 9 Feb 2012 17:13:33 +0100 Subject: [PATCH 4/4] [Form] Fixed issues mentioned in the PR comments --- UPGRADE-2.1.md | 4 ++-- .../Core/DataTransformer/ChoicesToBooleanArrayTransformer.php | 2 +- .../Component/Form/Extension/Core/Type/DateTimeTypeTest.php | 2 +- .../Tests/Component/Form/Extension/Core/Type/DateTypeTest.php | 2 +- .../Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/UPGRADE-2.1.md b/UPGRADE-2.1.md index c47d5f977c..b94b511d79 100644 --- a/UPGRADE-2.1.md +++ b/UPGRADE-2.1.md @@ -237,14 +237,14 @@ UPGRADE FROM 2.0 to 2.1 Before: - public function getParent() + public function getParent(array $options) { return 'single_text' === $options['widget'] ? 'text' : 'choice'; } After: - public function getParent() + public function getParent(array $options) { return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice'; } diff --git a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php index 5ec30926ad..f38102f298 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php +++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php @@ -107,7 +107,7 @@ class ChoicesToBooleanArrayTransformer implements DataTransformerInterface } if (count($unknown) > 0) { - throw new TransformationFailedException('The choices "' . implode('", "', $unknown) . '" where not found'); + throw new TransformationFailedException('The choices "' . implode('", "', $unknown) . '" were not found'); } return $result; diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php index 2cd18ad4ef..ca10dc8485 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php @@ -264,6 +264,6 @@ class DateTimeTypeTest extends LocalizedTestCase { // Throws an exception if "data_class" option is not explicitely set // to null in the type - $form = $this->factory->create('datetime', new \DateTime()); + $this->factory->create('datetime', new \DateTime()); } } diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php index 8bd78fcf94..425e3c71a0 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php @@ -536,6 +536,6 @@ class DateTypeTest extends LocalizedTestCase { // Throws an exception if "data_class" option is not explicitely set // to null in the type - $form = $this->factory->create('date', new \DateTime()); + $this->factory->create('date', new \DateTime()); } } diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php index 89e250790a..7d92f0b0e5 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php @@ -407,6 +407,6 @@ class TimeTypeTest extends LocalizedTestCase { // Throws an exception if "data_class" option is not explicitely set // to null in the type - $form = $this->factory->create('time', new \DateTime()); + $this->factory->create('time', new \DateTime()); } }