From 9bab1e8abaf57946af9c56d398562909dba46fb5 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Sun, 11 Nov 2018 23:03:02 +0100 Subject: [PATCH 1/3] [Form] Fixed empty data for compound date types --- .../Form/Extension/Core/Type/DateTimeType.php | 14 +++++++++ .../Form/Extension/Core/Type/DateType.php | 17 +++++++++++ .../Form/Extension/Core/Type/TimeType.php | 17 +++++++++++ .../Extension/Core/Type/DateTimeTypeTest.php | 27 +++++++++++++++++ .../Extension/Core/Type/DateTypeTest.php | 27 ++++++++++++----- .../Extension/Core/Type/TimeTypeTest.php | 29 ++++++++++++++++++- 6 files changed, 122 insertions(+), 9 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php index c3f95f59af..64d2c2d2f4 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php @@ -90,6 +90,9 @@ class DateTimeType extends AbstractType )); } } else { + // when the form is compound the entries of the array are ignored in favor of children data + // so we need to handle the cascade setting here + $emptyData = $builder->getEmptyData() ?: array(); // Only pass a subset of the options to children $dateOptions = array_intersect_key($options, array_flip(array( 'years', @@ -105,6 +108,10 @@ class DateTimeType extends AbstractType 'invalid_message_parameters', ))); + if (isset($emptyData['date'])) { + $dateOptions['empty_data'] = $emptyData['date']; + } + $timeOptions = array_intersect_key($options, array_flip(array( 'hours', 'minutes', @@ -121,6 +128,10 @@ class DateTimeType extends AbstractType 'invalid_message_parameters', ))); + if (isset($emptyData['time'])) { + $timeOptions['empty_data'] = $emptyData['time']; + } + if (false === $options['label']) { $dateOptions['label'] = false; $timeOptions['label'] = false; @@ -226,6 +237,9 @@ class DateTimeType extends AbstractType // this option. 'data_class' => null, 'compound' => $compound, + 'empty_data' => function (Options $options) { + return $options['compound'] ? array() : ''; + }, )); // Don't add some defaults in order to preserve the defaults diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateType.php index ad4499899a..ce41c3c7d3 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/DateType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/DateType.php @@ -75,7 +75,21 @@ class DateType extends AbstractType $yearOptions = $monthOptions = $dayOptions = array( 'error_bubbling' => true, + 'empty_data' => '', ); + // when the form is compound the entries of the array are ignored in favor of children data + // so we need to handle the cascade setting here + $emptyData = $builder->getEmptyData() ?: array(); + + if (isset($emptyData['year'])) { + $yearOptions['empty_data'] = $emptyData['year']; + } + if (isset($emptyData['month'])) { + $monthOptions['empty_data'] = $emptyData['month']; + } + if (isset($emptyData['day'])) { + $dayOptions['empty_data'] = $emptyData['day']; + } if (isset($options['invalid_message'])) { $dayOptions['invalid_message'] = $options['invalid_message']; @@ -272,6 +286,9 @@ class DateType extends AbstractType // this option. 'data_class' => null, 'compound' => $compound, + 'empty_data' => function (Options $options) { + return $options['compound'] ? array() : ''; + }, 'choice_translation_domain' => false, )); diff --git a/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php b/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php index 4a27cd440b..facfc098b7 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/TimeType.php @@ -70,7 +70,15 @@ class TimeType extends AbstractType } else { $hourOptions = $minuteOptions = $secondOptions = array( 'error_bubbling' => true, + 'empty_data' => '', ); + // when the form is compound the entries of the array are ignored in favor of children data + // so we need to handle the cascade setting here + $emptyData = $builder->getEmptyData() ?: array(); + + if (isset($emptyData['hour'])) { + $hourOptions['empty_data'] = $emptyData['hour']; + } if (isset($options['invalid_message'])) { $hourOptions['invalid_message'] = $options['invalid_message']; @@ -138,10 +146,16 @@ class TimeType extends AbstractType $builder->add('hour', self::$widgets[$options['widget']], $hourOptions); if ($options['with_minutes']) { + if (isset($emptyData['minute'])) { + $minuteOptions['empty_data'] = $emptyData['minute']; + } $builder->add('minute', self::$widgets[$options['widget']], $minuteOptions); } if ($options['with_seconds']) { + if (isset($emptyData['second'])) { + $secondOptions['empty_data'] = $emptyData['second']; + } $builder->add('second', self::$widgets[$options['widget']], $secondOptions); } @@ -265,6 +279,9 @@ class TimeType extends AbstractType // representation is not \DateTime, but an array, we need to unset // this option. 'data_class' => null, + 'empty_data' => function (Options $options) { + return $options['compound'] ? array() : ''; + }, 'compound' => $compound, 'choice_translation_domain' => false, )); diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTimeTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTimeTypeTest.php index 2531332f05..f0e45f5d56 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTimeTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTimeTypeTest.php @@ -631,4 +631,31 @@ class DateTimeTypeTest extends BaseTypeTest $this->assertSame($expectedData, $form->getNormData()); $this->assertSame($expectedData, $form->getData()); } + + /** + * @dataProvider provideEmptyData + */ + public function testSubmitNullUsesDateEmptyData($widget, $emptyData, $expectedData) + { + $form = $this->factory->create(static::TESTED_TYPE, null, array( + 'widget' => $widget, + 'empty_data' => $emptyData, + )); + $form->submit(null); + + $this->assertSame($emptyData, $form->getViewData()); + $this->assertEquals($expectedData, $form->getNormData()); + $this->assertEquals($expectedData, $form->getData()); + } + + public function provideEmptyData() + { + $expectedData = \DateTime::createFromFormat('Y-m-d H:i', '2018-11-11 21:23'); + + return array( + 'Simple field' => array('single_text', '2018-11-11T21:23:00', $expectedData), + 'Compound text field' => array('text', array('date' => array('year' => '2018', 'month' => '11', 'day' => '11'), 'time' => array('hour' => '21', 'minute' => '23')), $expectedData), + 'Compound choice field' => array('choice', array('date' => array('year' => '2018', 'month' => '11', 'day' => '11'), 'time' => array('hour' => '21', 'minute' => '23')), $expectedData), + ); + } } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTypeTest.php index 0f03b1851d..a15a6c2b32 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTypeTest.php @@ -1011,25 +1011,36 @@ class DateTypeTest extends BaseTypeTest )); $form->submit(null); - // view transformer write back empty strings in the view data + // view transformer writes back empty strings in the view data $this->assertSame(array('year' => '', 'month' => '', 'day' => ''), $form->getViewData()); $this->assertSame($expectedData, $form->getNormData()); $this->assertSame($expectedData, $form->getData()); } - public function testSingleTextSubmitNullUsesDefaultEmptyData() + /** + * @dataProvider provideEmptyData + */ + public function testSubmitNullUsesDateEmptyData($widget, $emptyData, $expectedData) { - $emptyData = '2018-11-11'; $form = $this->factory->create(static::TESTED_TYPE, null, array( - 'widget' => 'single_text', + 'widget' => $widget, 'empty_data' => $emptyData, )); $form->submit(null); - $date = new \DateTime($emptyData); - $this->assertSame($emptyData, $form->getViewData()); - $this->assertEquals($date, $form->getNormData()); - $this->assertEquals($date, $form->getData()); + $this->assertEquals($expectedData, $form->getNormData()); + $this->assertEquals($expectedData, $form->getData()); + } + + public function provideEmptyData() + { + $expectedData = \DateTime::createFromFormat('Y-m-d H:i:s', '2018-11-11 00:00:00'); + + return array( + 'Simple field' => array('single_text', '2018-11-11', $expectedData), + 'Compound text fields' => array('text', array('year' => '2018', 'month' => '11', 'day' => '11'), $expectedData), + 'Compound choice fields' => array('choice', array('year' => '2018', 'month' => '11', 'day' => '11'), $expectedData), + ); } } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/TimeTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/TimeTypeTest.php index 98d4f902fa..5ed4a236f1 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/TimeTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/TimeTypeTest.php @@ -805,9 +805,36 @@ class TimeTypeTest extends BaseTypeTest )); $form->submit(null); - // view transformer write back empty strings in the view data + // view transformer writes back empty strings in the view data $this->assertSame(array('hour' => '', 'minute' => ''), $form->getViewData()); $this->assertSame($expectedData, $form->getNormData()); $this->assertSame($expectedData, $form->getData()); } + + /** + * @dataProvider provideEmptyData + */ + public function testSubmitNullUsesDateEmptyData($widget, $emptyData, $expectedData) + { + $form = $this->factory->create(static::TESTED_TYPE, null, array( + 'widget' => $widget, + 'empty_data' => $emptyData, + )); + $form->submit(null); + + $this->assertSame($emptyData, $form->getViewData()); + $this->assertEquals($expectedData, $form->getNormData()); + $this->assertEquals($expectedData, $form->getData()); + } + + public function provideEmptyData() + { + $expectedData = \DateTime::createFromFormat('Y-m-d H:i', '1970-01-01 21:23'); + + return array( + 'Simple field' => array('single_text', '21:23', $expectedData), + 'Compound text field' => array('text', array('hour' => '21', 'minute' => '23'), $expectedData), + 'Compound choice field' => array('choice', array('hour' => '21', 'minute' => '23'), $expectedData), + ); + } } From de40f5d07b119195ec76b5fc135169667d653839 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Mon, 12 Nov 2018 11:47:20 +0100 Subject: [PATCH 2/3] [Form] Minor fixes in docs and cs --- src/Symfony/Component/Form/Form.php | 24 ++++++++++--------- .../Component/Form/NativeRequestHandler.php | 4 ++-- src/Symfony/Component/Form/Util/FormUtil.php | 2 +- .../Component/Form/Util/OrderedHashMap.php | 2 +- .../Form/Util/OrderedHashMapIterator.php | 4 +--- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index 9234073831..95186fae76 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/src/Symfony/Component/Form/Form.php @@ -32,16 +32,18 @@ use Symfony\Component\PropertyAccess\PropertyPath; * * (1) the "model" format required by the form's object * (2) the "normalized" format for internal processing - * (3) the "view" format used for display + * (3) the "view" format used for display simple fields + * or map children model data for compound fields * * A date field, for example, may store a date as "Y-m-d" string (1) in the * object. To facilitate processing in the field, this value is normalized * to a DateTime object (2). In the HTML representation of your form, a - * localized string (3) is presented to and modified by the user. + * localized string (3) may be presented to and modified by the user, or it could be an array of values + * to be mapped to choices fields. * * In most cases, format (1) and format (2) will be the same. For example, * a checkbox field uses a Boolean value for both internal processing and - * storage in the object. In these cases you simply need to set a value + * storage in the object. In these cases you simply need to set a view * transformer to convert between formats (2) and (3). You can do this by * calling addViewTransformer(). * @@ -49,7 +51,7 @@ use Symfony\Component\PropertyAccess\PropertyPath; * demonstrate this, let's extend our above date field to store the value * either as "Y-m-d" string or as timestamp. Internally we still want to * use a DateTime object for processing. To convert the data from string/integer - * to DateTime you can set a normalization transformer by calling + * to DateTime you can set a model transformer by calling * addModelTransformer(). The normalized data is then converted to the displayed * data as described before. * @@ -218,7 +220,7 @@ class Form implements \IteratorAggregate, FormInterface } if (null === $this->getName() || '' === $this->getName()) { - return; + return null; } $parent = $this->parent; @@ -341,8 +343,8 @@ class Form implements \IteratorAggregate, FormInterface $modelData = $event->getData(); } - // Treat data as strings unless a value transformer exists - if (!$this->config->getViewTransformers() && !$this->config->getModelTransformers() && is_scalar($modelData)) { + // Treat data as strings unless a transformer exists + if (is_scalar($modelData) && !$this->config->getViewTransformers() && !$this->config->getModelTransformers()) { $modelData = (string) $modelData; } @@ -1068,7 +1070,7 @@ class Form implements \IteratorAggregate, FormInterface } /** - * Normalizes the value if a normalization transformer is set. + * Normalizes the value if a model transformer is set. * * @param mixed $value The value to transform * @@ -1090,7 +1092,7 @@ class Form implements \IteratorAggregate, FormInterface } /** - * Reverse transforms a value if a normalization transformer is set. + * Reverse transforms a value if a model transformer is set. * * @param string $value The value to reverse transform * @@ -1114,7 +1116,7 @@ class Form implements \IteratorAggregate, FormInterface } /** - * Transforms the value if a value transformer is set. + * Transforms the value if a view transformer is set. * * @param mixed $value The value to transform * @@ -1145,7 +1147,7 @@ class Form implements \IteratorAggregate, FormInterface } /** - * Reverse transforms a value if a value transformer is set. + * Reverse transforms a value if a view transformer is set. * * @param string $value The value to reverse transform * diff --git a/src/Symfony/Component/Form/NativeRequestHandler.php b/src/Symfony/Component/Form/NativeRequestHandler.php index ccebdab6d8..94210d51e8 100644 --- a/src/Symfony/Component/Form/NativeRequestHandler.php +++ b/src/Symfony/Component/Form/NativeRequestHandler.php @@ -15,7 +15,7 @@ use Symfony\Component\Form\Exception\UnexpectedTypeException; use Symfony\Component\Form\Util\ServerParams; /** - * A request handler using PHP's super globals $_GET, $_POST and $_SERVER. + * A request handler using PHP super globals $_GET, $_POST and $_SERVER. * * @author Bernhard Schussek */ @@ -213,7 +213,7 @@ class NativeRequestHandler implements RequestHandlerInterface if (self::$fileKeys === $keys) { if (UPLOAD_ERR_NO_FILE === $data['error']) { - return; + return null; } return $data; diff --git a/src/Symfony/Component/Form/Util/FormUtil.php b/src/Symfony/Component/Form/Util/FormUtil.php index 0862179f54..53053f9d5b 100644 --- a/src/Symfony/Component/Form/Util/FormUtil.php +++ b/src/Symfony/Component/Form/Util/FormUtil.php @@ -27,7 +27,7 @@ class FormUtil * Returns whether the given data is empty. * * This logic is reused multiple times throughout the processing of - * a form and needs to be consistent. PHP's keyword `empty` cannot + * a form and needs to be consistent. PHP keyword `empty` cannot * be used as it also considers 0 and "0" to be empty. * * @param mixed $data diff --git a/src/Symfony/Component/Form/Util/OrderedHashMap.php b/src/Symfony/Component/Form/Util/OrderedHashMap.php index 6a97559850..26e45a4622 100644 --- a/src/Symfony/Component/Form/Util/OrderedHashMap.php +++ b/src/Symfony/Component/Form/Util/OrderedHashMap.php @@ -128,7 +128,7 @@ class OrderedHashMap implements \ArrayAccess, \IteratorAggregate, \Countable $key = array() === $this->orderedKeys // If the array is empty, use 0 as key ? 0 - // Imitate PHP's behavior of generating a key that equals + // Imitate PHP behavior of generating a key that equals // the highest existing integer key + 1 : 1 + (int) max($this->orderedKeys); } diff --git a/src/Symfony/Component/Form/Util/OrderedHashMapIterator.php b/src/Symfony/Component/Form/Util/OrderedHashMapIterator.php index 3de636392d..93a7caa58d 100644 --- a/src/Symfony/Component/Form/Util/OrderedHashMapIterator.php +++ b/src/Symfony/Component/Form/Util/OrderedHashMapIterator.php @@ -56,8 +56,6 @@ class OrderedHashMapIterator implements \Iterator private $current; /** - * Creates a new iterator. - * * @param array $elements The elements of the map, indexed by their * keys * @param array $orderedKeys The keys of the map in the order in which @@ -84,7 +82,7 @@ class OrderedHashMapIterator implements \Iterator */ public function __destruct() { - // Use array_splice() instead of isset() to prevent holes in the + // Use array_splice() instead of unset() to prevent holes in the // array indices, which would break the initialization of $cursorId array_splice($this->managedCursors, $this->cursorId, 1); } From 5f8bd898b41491aac84d27504015bc7d7216f6f9 Mon Sep 17 00:00:00 2001 From: Edvin Hultberg Date: Tue, 13 Nov 2018 14:44:43 +0100 Subject: [PATCH 3/3] Command::addOption should allow int in $default The constructor for InputOption allows int on the $default parameter, but not Command::addOption $default parameter fixup: apply coding standards patch --- src/Symfony/Component/Console/Command/Command.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/Console/Command/Command.php b/src/Symfony/Component/Console/Command/Command.php index 00df2d4454..75d062d56d 100644 --- a/src/Symfony/Component/Console/Command/Command.php +++ b/src/Symfony/Component/Console/Command/Command.php @@ -381,11 +381,11 @@ class Command /** * Adds an option. * - * @param string $name The option name - * @param string|array $shortcut The shortcuts, can be null, a string of shortcuts delimited by | or an array of shortcuts - * @param int|null $mode The option mode: One of the VALUE_* constants - * @param string $description A description text - * @param string|string[]|bool|null $default The default value (must be null for self::VALUE_NONE) + * @param string $name The option name + * @param string|array $shortcut The shortcuts, can be null, a string of shortcuts delimited by | or an array of shortcuts + * @param int|null $mode The option mode: One of the VALUE_* constants + * @param string $description A description text + * @param string|string[]|int|bool|null $default The default value (must be null for self::VALUE_NONE) * * @throws InvalidArgumentException If option mode is invalid or incompatible *