[Form] Fixed expanded choice field to be marked invalid when unknown …

…choices are submitted
symfony · fabpot · Sep 10, 2013 · May 5, 2013 · May 5, 2013 · May 5, 2013
commit 9542d72dc049bad180b56ece4619ef9773f4cabf
diff --git a/src/Symfony/Component/Form/Extension/Core/DataTransformer/BooleanToStringTransformer.php b/src/Symfony/Component/Form/Extension/Core/DataTransformer/BooleanToStringTransformer.php
@@ -49,15 +49,11 @@ public function __construct($trueValue)
      */
     public function transform($value)
     {
-        if (null === $value) {
-            return null;
-        }
-
         if (!is_bool($value)) {
             throw new TransformationFailedException('Expected a Boolean.');
         }
 
-        return true === $value ? $this->trueValue : null;
+        return $value ? $this->trueValue : null;
     }
 
     /**

diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/FixCheckboxInputListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/FixCheckboxInputListener.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Form\Extension\Core\EventListener;
 
+use Symfony\Component\Form\Exception\TransformationFailedException;
 use Symfony\Component\Form\FormEvents;
 use Symfony\Component\Form\FormEvent;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -38,10 +39,43 @@ public function __construct(ChoiceListInterface $choiceList)
 
     public function preSubmit(FormEvent $event)
     {
-        $values = (array) $event->getData();
-        $indices = $this->choiceList->getIndicesForValues($values);
+        $data = $event->getData();
 
-        $event->setData(count($indices) > 0 ? array_combine($indices, $values) : array());
+        if (is_array($data)) {
+            // Flip the submitted values for faster lookup
+            // It's better to flip this array than $existingValues because
+            // $submittedValues is generally smaller.
+            $submittedValues = array_flip($data);
+
+            // Since expanded choice fields are completely loaded anyway, we
+            // can just as well get the values again without losing performance.
+            $existingValues = $this->choiceList->getValues();
+
+            // Clear the data array and fill it with correct indices
+            $data = array();
+
+            foreach ($existingValues as $index => $value) {
+                if (isset($submittedValues[$value])) {
+                    // Value was submitted
+                    $data[$index] = $value;
+                    unset($submittedValues[$value]);
+                }
+            }
+
+            if (count($submittedValues) > 0) {
+                throw new TransformationFailedException(sprintf(
+                    'The following choices were not found: "%s"',
+                    implode('", "', array_keys($submittedValues))
+                ));
+            }
+        } elseif ('' === $data || null === $data) {
+            // Empty values are always accepted.
+            $data = array();
+        }
+
+        // Else leave the data unchanged to provoke an error during submission
+
+        $event->setData($data);
     }
 
     /**

diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php
@@ -42,10 +42,22 @@ public function __construct(ChoiceListInterface $choiceList, $placeholderPresent
 
     public function preSubmit(FormEvent $event)
     {
-        $value = $event->getData();
-        $index = current($this->choiceList->getIndicesForValues(array($value)));
+        $data = $event->getData();
 
-        $event->setData(false !== $index ? array($index => $value) : ($this->placeholderPresent ? array('placeholder' => '') : array()))   ;
+        // Since expanded choice fields are completely loaded anyway, we
+        // can just as well get the values again without losing performance.
+        $existingValues = $this->choiceList->getValues();
+
+        if (false !== ($index = array_search($data, $existingValues, true))) {
+            $data = array($index => $data);
+        } elseif ('' === $data || null === $data) {
+            // Empty values are always accepted.
+            $data = $this->placeholderPresent ? array('placeholder' => '') : array();
+        }
+
+        // Else leave the data unchanged to provoke an error during submission
+
+        $event->setData($data);
     }
 
     /**

diff --git a/src/Symfony/Component/Form/Extension/Core/Type/CheckboxType.php b/src/Symfony/Component/Form/Extension/Core/Type/CheckboxType.php
@@ -25,9 +25,14 @@ class CheckboxType extends AbstractType
      */
     public function buildForm(FormBuilderInterface $builder, array $options)
     {
-        $builder
-            ->addViewTransformer(new BooleanToStringTransformer($options['value']))
-        ;
+        // Unlike in other types, where the data is NULL by default, it
+        // needs to be a Boolean here. setData(null) is not acceptable
+        // for checkboxes and radio buttons (unless a custom model
+        // transformer handles this case).
+        // We cannot solve this case via overriding the "data" option, because
+        // doing so also calls setDataLocked(true).
+        $builder->setData(isset($options['data']) ? $options['data'] : false);
+        $builder->addViewTransformer(new BooleanToStringTransformer($options['value']));
     }
 
     /**
@@ -46,8 +51,8 @@ public function buildView(FormView $view, FormInterface $form, array $options)
      */
     public function setDefaultOptions(OptionsResolverInterface $resolver)
     {
-        $emptyData = function (FormInterface $form, $clientData) {
-            return $clientData;
+        $emptyData = function (FormInterface $form, $viewData) {
+            return $viewData;
         };
 
         $resolver->setDefaults(array(

diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php
@@ -522,83 +522,82 @@ public function submit($submittedData, $clearMissing = true)
 
         $dispatcher = $this->config->getEventDispatcher();
 
-        // Hook to change content of the data submitted by the browser
-        if ($dispatcher->hasListeners(FormEvents::PRE_SUBMIT)) {
-            $event = new FormEvent($this, $submittedData);
-            $dispatcher->dispatch(FormEvents::PRE_SUBMIT, $event);
-            $submittedData = $event->getData();
-        }
+        $modelData = null;
+        $normData = null;
+        $viewData = null;
 
-        // Check whether the form is compound.
-        // This check is preferable over checking the number of children,
-        // since forms without children may also be compound.
-        // (think of empty collection forms)
-        if ($this->config->getCompound()) {
-            if (!is_array($submittedData)) {
-                $submittedData = array();
+        try {
+            // Hook to change content of the data submitted by the browser
+            if ($dispatcher->hasListeners(FormEvents::PRE_SUBMIT)) {
+                $event = new FormEvent($this, $submittedData);
+                $dispatcher->dispatch(FormEvents::PRE_SUBMIT, $event);
+                $submittedData = $event->getData();
             }
 
-            foreach ($this->children as $name => $child) {
-                if (array_key_exists($name, $submittedData) || $clearMissing) {
-                    $child->submit(isset($submittedData[$name]) ? $submittedData[$name] : null, $clearMissing);
-                    unset($submittedData[$name]);
+            // Check whether the form is compound.
+            // This check is preferable over checking the number of children,
+            // since forms without children may also be compound.
+            // (think of empty collection forms)
+            if ($this->config->getCompound()) {
+                if (null === $submittedData) {
+                    $submittedData = array();
                 }
-            }
-
-            $this->extraData = $submittedData;
-        }
 
-        // Forms that inherit their parents' data also are not processed,
-        // because then it would be too difficult to merge the changes in
-        // the child and the parent form. Instead, the parent form also takes
-        // changes in the grandchildren (i.e. children of the form that inherits
-        // its parent's data) into account.
-        // (see InheritDataAwareIterator below)
-        if ($this->config->getInheritData()) {
-            $this->submitted = true;
+                if (!is_array($submittedData)) {
+                    throw new TransformationFailedException('Compound forms expect an array or NULL on submission.');
+                }
 
-            // When POST_SUBMIT is reached, the data is not yet updated, so pass
-            // NULL to prevent hard-to-debug bugs.
-            $dataForPostSubmit = null;
-        } else {
-            // If the form is compound, the default data in view format
-            // is reused. The data of the children is merged into this
-            // default data using the data mapper.
-            // If the form is not compound, the submitted data is also the data in view format.
-            $viewData = $this->config->getCompound() ? $this->viewData : $submittedData;
-
-            if (FormUtil::isEmpty($viewData)) {
-                $emptyData = $this->config->getEmptyData();
-
-                if ($emptyData instanceof \Closure) {
-                    /* @var \Closure $emptyData */
-                    $emptyData = $emptyData($this, $viewData);
+                foreach ($this->children as $name => $child) {
+                    if (isset($submittedData[$name]) || $clearMissing) {
+                        $child->submit(isset($submittedData[$name]) ? $submittedData[$name] : null, $clearMissing);
+                        unset($submittedData[$name]);
+                    }
                 }
 
-                $viewData = $emptyData;
+                $this->extraData = $submittedData;
             }
 
-            // Merge form data from children into existing view data
-            // It is not necessary to invoke this method if the form has no children,
-            // even if it is compound.
-            if (count($this->children) > 0) {
-                // Use InheritDataAwareIterator to process children of
-                // descendants that inherit this form's data.
-                // These descendants will not be submitted normally (see the check
-                // for $this->config->getInheritData() above)
-                $iterator = new InheritDataAwareIterator($this->children);
-                $iterator = new \RecursiveIteratorIterator($iterator);
-                $this->config->getDataMapper()->mapFormsToData($iterator, $viewData);
-            }
+            // Forms that inherit their parents' data also are not processed,
+            // because then it would be too difficult to merge the changes in
+            // the child and the parent form. Instead, the parent form also takes
+            // changes in the grandchildren (i.e. children of the form that inherits
+            // its parent's data) into account.
+            // (see InheritDataAwareIterator below)
+            if (!$this->config->getInheritData()) {
+                // If the form is compound, the default data in view format
+                // is reused. The data of the children is merged into this
+                // default data using the data mapper.
                // If the form is not compound, the submitted data is also the data in view format.
+                $viewData = $this->config->getCompound() ? $this->viewData : $submittedData;
+
+                if (FormUtil::isEmpty($viewData)) {
+                    $emptyData = $this->config->getEmptyData();
+
+                    if ($emptyData instanceof \Closure) {
+                        /* @var \Closure $emptyData */
+                        $emptyData = $emptyData($this, $viewData);
+                    }
+
+                    $viewData = $emptyData;
+                }
 
-            $modelData = null;
-            $normData = null;
+                // Merge form data from children into existing view data
+                // It is not necessary to invoke this method if the form has no children,
+                // even if it is compound.
+                if (count($this->children) > 0) {
+                    // Use InheritDataAwareIterator to process children of
+                    // descendants that inherit this form's data.
+                    // These descendants will not be submitted normally (see the check
+                    // for $this->config->getInheritData() above)
+                    $childrenIterator = new InheritDataAwareIterator($this->children);
+                    $childrenIterator = new \RecursiveIteratorIterator($childrenIterator);
+                    $this->config->getDataMapper()->mapFormsToData($childrenIterator, $viewData);
+                }
 
-            try {
                 // Normalize data to unified representation
                 $normData = $this->viewToNorm($viewData);
 
-                // Hook to change content of the data into the normalized
+                // Hook to change content of the data in the normalized
                 // representation
                 if ($dispatcher->hasListeners(FormEvents::SUBMIT)) {
                     $event = new FormEvent($this, $normData);
@@ -609,20 +608,26 @@ public function submit($submittedData, $clearMissing = true)
                 // Synchronize representations - must not change the content!
                 $modelData = $this->normToModel($normData);
                 $viewData = $this->normToView($normData);
-            } catch (TransformationFailedException $e) {
-                $this->synchronized = false;
             }
-
-            $this->submitted = true;
-            $this->modelData = $modelData;
-            $this->normData = $normData;
-            $this->viewData = $viewData;
-
-            $dataForPostSubmit = $viewData;
+        } catch (TransformationFailedException $e) {
+            $this->synchronized = false;
+
+            // If $viewData was not yet set, set it to $submittedData so that
+            // the erroneous data is accessible on the form.
+            // Forms that inherit data never set any data, because the getters
+            // forward to the parent form's getters anyway.
+            if (null === $viewData && !$this->config->getInheritData()) {
+                $viewData = $submittedData;
+            }
         }
 
+        $this->submitted = true;
+        $this->modelData = $modelData;
+        $this->normData = $normData;
+        $this->viewData = $viewData;
+
         if ($dispatcher->hasListeners(FormEvents::POST_SUBMIT)) {
-            $event = new FormEvent($this, $dataForPostSubmit);
+            $event = new FormEvent($this, $viewData);
             $dispatcher->dispatch(FormEvents::POST_SUBMIT, $event);
         }
 

diff --git a/...ny/Component/Form/Tests/Extension/Core/DataTransformer/BooleanToStringTransformerTest.php b/...ny/Component/Form/Tests/Extension/Core/DataTransformer/BooleanToStringTransformerTest.php
@@ -17,6 +17,9 @@ class BooleanToStringTransformerTest extends \PHPUnit_Framework_TestCase
 {
     const TRUE_VALUE = '1';
 
+    /**
+     * @var BooleanToStringTransformer
+     */
     protected $transformer;
 
     protected function setUp()
@@ -33,20 +36,29 @@ public function testTransform()
     {
         $this->assertEquals(self::TRUE_VALUE, $this->transformer->transform(true));
         $this->assertNull($this->transformer->transform(false));
-        $this->assertNull($this->transformer->transform(null));
     }
 
-    public function testTransformExpectsBoolean()
+    /**
+     * @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
+     */
+    public function testTransformFailsIfNull()
     {
-        $this->setExpectedException('Symfony\Component\Form\Exception\TransformationFailedException');
+        $this->transformer->transform(null);
+    }
 
+    /**
+     * @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
+     */
+    public function testTransformFailsIfString()
+    {
         $this->transformer->transform('1');
     }
 
-    public function testReverseTransformExpectsString()
+    /**
+     * @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
+     */
+    public function testReverseTransformFailsIfInteger()
     {
-        $this->setExpectedException('Symfony\Component\Form\Exception\TransformationFailedException');
-
         $this->transformer->reverseTransform(1);
     }