[Form] Incorporated changes suggested in PR comments

UPGRADE-2.1.md

@@ -79,40 +79,45 @@ UPGRADE FROM 2.0 to 2.1
     the parent form.
 
 * In the template of the choice type, instead of a single "choices" variable
-  there are now two variables: "choices" and "choice_labels".
+  there are now two variables: "choices" and "choice_labels"
-  "choices" contains the choices in the values (before they were in the keys)
+  
-  and "choice_labels" contains the matching labels. Both arrays have
+    "choices" contains the choices in the values (before they were in the keys)
-  identical keys.
+    and "choice_labels" contains the matching labels. Both arrays have
+    identical keys.
 
-  Before:
+    Before:
 
-      {% for choice, label in choices %}
+        {% for choice, label in choices %}
-          <option value="{{ choice }}"{% if _form_is_choice_selected(form, choice) %} selected="selected"{% endif %}>
+            <option value="{{ choice }}"{% if _form_is_choice_selected(form, choice) %} selected="selected"{% endif %}>
-              {{ label }}
+                {{ label }}
-          </option>
+            </option>
-      {% endfor %}
+        {% endfor %}
 
-  After:
+    After:
 
-      {% for index, choice in choices %}
+        {% for index, choice in choices %}
-          <option value="{{ choice }}"{% if _form_is_choice_selected(form, choice) %} selected="selected"{% endif %}>
+            <option value="{{ choice }}"{% if _form_is_choice_selected(form, choice) %} selected="selected"{% endif %}>
-              {{ choice_labels[index] }}
+                {{ choice_labels[index] }}
-          </option>
+            </option>
-      {% endfor %}
+        {% endfor %}
 
 * The strategy for generating the HTML attributes "id" and "name"
-  of choices in a choice field has changed. Instead of appending the choice
+  of choices in a choice field has changed
-  value, a generated integer is now appended by default. Take care if your
+  
-  Javascript relies on that. If you can guarantee that your choice values only
+    Instead of appending the choice value, a generated integer is now appended
-  contain ASCII letters, digits, letters, colons and underscores, you can
+    by default. Take care if your Javascript relies on that. If you can
-  restore the old behaviour by setting the option "index_strategy" of the
+    guarantee that your choice values only contain ASCII letters, digits,
-  choice field to `ChoiceList::COPY_CHOICE`.
+    letters, colons and underscores, you can restore the old behaviour by
+    setting the option "index_strategy" of the choice field to
+    `ChoiceList::COPY_CHOICE`.
 
 * The strategy for generating the HTML attributes "value" of choices in a
-  choice field has changed. Instead of using the choice value, a generated
+  choice field has changed
-  integer is now stored. Again, take care if your Javascript reads this value.
+  
-  If your choice field is a non-expanded single-choice field, or if
+    Instead of using the choice value, a generated integer is now stored.
-  the choices are guaranteed not to contain the empty string '' (which is the
+   Again, take care if your Javascript reads this value. If your choice field
-  case when you added it manually or when the field is a single-choice field
+   is a non-expanded single-choice field, or if the choices are guaranteed not
-  and is not required), you can restore the old behaviour by setting the
+   to contain the empty string '' (which is the case when you added it manually
-  option "value_strategy" to `ChoiceList::COPY_CHOICE`.
+   or when the field is a single-choice field and is not required), you can
+   restore the old behaviour by setting the option "value_strategy" to
+   `ChoiceList::COPY_CHOICE`.

src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php

@@ -386,7 +386,7 @@ class EntityChoiceList extends ObjectChoiceList
             // The third parameter $preferredChoices is currently not supported
             parent::initialize($entities, array(), array());
         } catch (StringCastException $e) {
-            throw new StringCastException('Objects in the entity field must have a "__toString()" method defined. Alternatively you can set the "property" option.', null, $e);
+            throw new StringCastException(str_replace('argument $labelPath', 'option "property"', $e->getMessage()), null, $e);
         }
 
         $this->loaded = true;

src/Symfony/Bridge/Doctrine/Form/DataTransformer/CollectionToArrayTransformer.php

@@ -26,9 +26,9 @@ class CollectionToArrayTransformer implements DataTransformerInterface
     /**
      * Transforms a collection into an array.
      *
-     * @param  Collection|object $collection A collection of entities, a single entity or NULL
+     * @param  Collection $collection A collection of entities
      *
-     * @return mixed An array of choice keys, a single key or NULL
+     * @return mixed An array of entities
      */
     public function transform($collection)
     {
@@ -46,9 +46,9 @@ class CollectionToArrayTransformer implements DataTransformerInterface
     /**
      * Transforms choice keys into entities.
      *
-     * @param  mixed $keys        An array of keys, a single key or NULL
+     * @param  mixed $keys  An array of entities
      *
-     * @return Collection|object  A collection of entities, a single entity or NULL
+     * @return Collection   A collection of entities
      */
     public function reverseTransform($array)
     {

src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceList.php

@@ -19,7 +19,7 @@ use Symfony\Component\Form\Exception\UnexpectedTypeException;
  *
  * @author Bernhard Schussek <bschussek@gmail.com>
  */
-abstract class ChoiceList implements ChoiceListInterface
+class ChoiceList implements ChoiceListInterface
 {
     /**
      * Strategy creating new indices/values by creating a copy of the choice.
@@ -132,8 +132,7 @@ abstract class ChoiceList implements ChoiceListInterface
      * @param integer $indexStrategy The strategy used to create choice indices.
      *                               One of COPY_CHOICE and GENERATE.
      */
-    public function __construct($choices, array $labels,
+    public function __construct($choices, array $labels, array $preferredChoices = array(), $valueStrategy = self::GENERATE, $indexStrategy = self::GENERATE)
-        array $preferredChoices = array(), $valueStrategy, $indexStrategy)
     {
         $this->valueStrategy = $valueStrategy;
         $this->indexStrategy = $indexStrategy;
@@ -152,10 +151,6 @@ abstract class ChoiceList implements ChoiceListInterface
      */
     protected function initialize($choices, array $labels, array $preferredChoices)
     {
-        if (!is_array($choices) && !$choices instanceof \Traversable) {
-            throw new UnexpectedTypeException($choices, 'array or \Traversable');
-        }
-
         $this->choices = array();
         $this->values = array();
         $this->labels = array();
@@ -277,7 +272,7 @@ abstract class ChoiceList implements ChoiceListInterface
 
         // If the values are identical to the choices, we can just return them
         // to improve performance a little bit
-        if ($this->valueStrategy === self::COPY_CHOICE) {
+        if (self::COPY_CHOICE === $this->valueStrategy) {
             return $this->fixChoices(array_intersect($values, $this->values));
         }
 
@@ -289,7 +284,7 @@ abstract class ChoiceList implements ChoiceListInterface
                     $choices[] = $this->choices[$i];
                     unset($values[$j]);
 
-                    if (count($values) === 0) {
+                    if (0 === count($values)) {
                         break 2;
                     }
                 }
@@ -314,7 +309,7 @@ abstract class ChoiceList implements ChoiceListInterface
 
         // If the values are identical to the choices, we can just return them
         // to improve performance a little bit
-        if ($this->valueStrategy === self::COPY_CHOICE) {
+        if (self::COPY_CHOICE === $this->valueStrategy) {
             return $this->fixValues(array_intersect($choices, $this->choices));
         }
 
@@ -326,7 +321,7 @@ abstract class ChoiceList implements ChoiceListInterface
                     $values[] = $this->values[$i];
                     unset($choices[$j]);
 
-                    if (count($choices) === 0) {
+                    if (0 === count($choices)) {
                         break 2;
                     }
                 }
@@ -356,7 +351,7 @@ abstract class ChoiceList implements ChoiceListInterface
                     $indices[] = $i;
                     unset($choices[$j]);
 
-                    if (count($choices) === 0) {
+                    if (0 === count($choices)) {
                         break 2;
                     }
                 }
@@ -386,7 +381,7 @@ abstract class ChoiceList implements ChoiceListInterface
                     $indices[] = $i;
                     unset($values[$j]);
 
-                    if (count($values) === 0) {
+                    if (0 === count($values)) {
                         break 2;
                     }
                 }
@@ -413,6 +408,10 @@ abstract class ChoiceList implements ChoiceListInterface
      */
     protected function addChoices(&$bucketForPreferred, &$bucketForRemaining, $choices, $labels, array $preferredChoices)
     {
+        if (!is_array($choices) && !$choices instanceof \Traversable) {
+            throw new UnexpectedTypeException($choices, 'array or \Traversable');
+        }
+
         // Add choices to the nested buckets
         foreach ($choices as $group => $choice) {
             if (is_array($choice)) {
@@ -537,7 +536,7 @@ abstract class ChoiceList implements ChoiceListInterface
      */
     protected function createIndex($choice)
     {
-        if ($this->indexStrategy === self::COPY_CHOICE) {
+        if (self::COPY_CHOICE === $this->indexStrategy) {
             return $choice;
         }
 
@@ -555,7 +554,7 @@ abstract class ChoiceList implements ChoiceListInterface
      */
     protected function createValue($choice)
     {
-        if ($this->valueStrategy === self::COPY_CHOICE) {
+        if (self::COPY_CHOICE === $this->valueStrategy) {
             return $choice;
         }
 

src/Symfony/Component/Form/Extension/Core/ChoiceList/ComplexChoiceList.php

@@ -1,44 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\Form\Extension\Core\ChoiceList;
-
-use Symfony\Component\Form\Util\FormUtil;
-
-use Symfony\Component\Form\Exception\UnexpectedTypeException;
-
-/**
- * A choice list that can store arbitrary scalar and object choices.
- *
- * Arrays as choices are not supported.
- *
- * @author Bernhard Schussek <bschussek@gmail.com>
- */
-class ComplexChoiceList extends ChoiceList
-{
-    /**
-     * Creates a new complex choice list.
-     *
-     * @param array $choices The array of choices. Choices may also be given
-     *                       as hierarchy of unlimited depth. Hierarchies are
-     *                       created by creating nested arrays. The title of
-     *                       the sub-hierarchy can be stored in the array
-     *                       key pointing to the nested array.
-     * @param array $labels  The array of labels. The structure of this array
-     *                       should match the structure of $choices.
-     * @param array $preferredChoices A flat array of choices that should be
-     *                                presented to the user with priority.
-     */
-    public function __construct(array $choices, array $labels, array $preferredChoices = array())
-    {
-        parent::__construct($choices, $labels, $preferredChoices, self::GENERATE, self::GENERATE);
-    }
-}

src/Symfony/Component/Form/Extension/Core/ChoiceList/ObjectChoiceList.php

@@ -121,7 +121,7 @@ class ObjectChoiceList extends ChoiceList
                     $group = null;
                 }
 
-                if ($group === null) {
+                if (null === $group) {
                     $groupedChoices[$i] = $choice;
                 } else {
                     if (!isset($groupedChoices[$group])) {
@@ -194,7 +194,7 @@ class ObjectChoiceList extends ChoiceList
             } elseif (method_exists($choice, '__toString')) {
                 $labels[$i] = (string) $choice;
             } else {
-                throw new StringCastException('Objects passed to the choice field must have a "__toString()" method defined. Alternatively you can set the $labelPath argument to choose the property used as label.');
+                throw new StringCastException('A "__toString()" method was not found on the objects of type "' . get_class($choice) . '" passed to the choice field. To read a custom getter instead, set the argument $labelPath to the desired property path.');
             }
         }
     }

tests/Symfony/Tests/Component/Form/Extension/Core/ChoiceList/ChoiceListTest.php

@@ -11,9 +11,9 @@
 
 namespace Symfony\Tests\Component\Form\Extension\Core\ChoiceList;
 
-use Symfony\Component\Form\Extension\Core\ChoiceList\ComplexChoiceList;
+use Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceList;
 
-class ComplexChoiceListTest extends \PHPUnit_Framework_TestCase
+class ChoiceListTest extends \PHPUnit_Framework_TestCase
 {
     private $obj1;
 
@@ -34,7 +34,7 @@ class ComplexChoiceListTest extends \PHPUnit_Framework_TestCase
         $this->obj3 = new \stdClass();
         $this->obj4 = new \stdClass();
 
-        $this->list = new ComplexChoiceList(
+        $this->list = new ChoiceList(
             array(
                 'Group 1' => array($this->obj1, $this->obj2),
                 'Group 2' => array($this->obj3, $this->obj4),
@@ -60,7 +60,7 @@ class ComplexChoiceListTest extends \PHPUnit_Framework_TestCase
 
     public function testInitArray()
     {
-        $this->list = new ComplexChoiceList(
+        $this->list = new ChoiceList(
             array($this->obj1, $this->obj2, $this->obj3, $this->obj4),
             array('A', 'B', 'C', 'D'),
             array($this->obj2)