[Form] Fixed new ArrayChoiceList to compare choices by their values, …

…if enabled
symfony · webmozart · Apr 1, 2015 · Sep 26, 2014 · Mar 25, 2015 · Mar 25, 2015
commit a289deb97358cf7295bbd1410d954f2d66c5346e
diff --git a/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php b/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php
@@ -11,7 +11,7 @@

 namespace Symfony\Component\Form\ChoiceList;

-use Symfony\Component\Form\Exception\InvalidArgumentException;
+use Symfony\Component\Form\Exception\UnexpectedTypeException;

 /**
 * A list of choices with arbitrary data types.
@@ -39,33 +39,46 @@ class ArrayChoiceList implements ChoiceListInterface
     */
    protected $values = array();

+    /**
+     * The callback for creating the value for a choice.
+     *
+     * @var callable
+     */
+    protected $valueCallback;
+
    /**
     * Creates a list with the given choices and values.
     *
     * The given choice array must have the same array keys as the value array.
     *
-     * @param array    $choices The selectable choices
-     * @param string[] $values  The string values of the choices
-     *
-     * @throws InvalidArgumentException If the keys of the choices don't match
-     *                                  the keys of the values
+     * @param array    $choices        The selectable choices
+     * @param callable $value          The callable for creating the value for a
+     *                                 choice. If `null` is passed, incrementing
+     *                                 integers are used as values
+     * @param bool     $compareByValue Whether to use the value callback to
+     *                                 compare choices. If `null`, choices are
+     *                                 compared by identity
     */
-    public function __construct(array $choices, array $values)
+    public function __construct(array $choices, $value = null, $compareByValue = false)
    {
-        $choiceKeys = array_keys($choices);
-        $valueKeys = array_keys($values);
-
-        if ($choiceKeys !== $valueKeys) {
-            throw new InvalidArgumentException(sprintf(
-                'The keys of the choices and the values must match. The choice '.
-                'keys are: "%s". The value keys are: "%s".',
-                implode('", "', $choiceKeys),
-                implode('", "', $valueKeys)
-            ));
+        if (null !== $value && !is_callable($value)) {
+            throw new UnexpectedTypeException($value, 'null or callable');
        }

        $this->choices = $choices;
-        $this->values = array_map('strval', $values);
+        $this->values = array();
+        $this->valueCallback = $compareByValue ? $value : null;
+
+        if (null === $value) {
+            $i = 0;
+            foreach ($this->choices as $key => $choice) {
+                $this->values[$key] = (string) $i++;
+            }
+        } else {
+            foreach ($choices as $key => $choice) {
+                $this->values[$key] = (string) call_user_func($value, $choice, $key);
+            }
+        }
    }

    /**
@@ -116,6 +129,17 @@ public function getValuesForChoices(array $choices)
    {
        $values = array();

+        // Use the value callback to compare choices by their values, if present
+        if ($this->valueCallback) {
+            $givenValues = array();
+            foreach ($choices as $key => $choice) {
+                $givenValues[$key] = (string) call_user_func($this->valueCallback, $choice, $key);
+            }
+
+            return array_intersect($givenValues, $this->values);
+        }
+
+        // Otherwise compare choices by identity
        foreach ($choices as $i => $givenChoice) {
            foreach ($this->choices as $j => $choice) {
                if ($choice !== $givenChoice) {

diff --git a/src/Symfony/Component/Form/ChoiceList/ArrayKeyChoiceList.php b/src/Symfony/Component/Form/ChoiceList/ArrayKeyChoiceList.php
@@ -43,21 +43,14 @@
 * @deprecated Added for backwards compatibility in Symfony 2.7, to be removed
 *             in Symfony 3.0.
 */
-class ArrayKeyChoiceList implements ChoiceListInterface
+class ArrayKeyChoiceList extends ArrayChoiceList
 {
    /**
-     * The selectable choices.
+     * Whether the choices are used as values.
     *
-     * @var array
+     * @var bool
     */
-    private $choices = array();
-
-    /**
-     * The values of the choices.
-     *
-     * @var string[]
-     */
-    private $values = array();
+    private $useChoicesAsValues = false;

    /**
     * Casts the given choice to an array key.
@@ -100,63 +93,42 @@ public static function toArrayKey($choice)
     * values.
     *
     * @param array    $choices The selectable choices
-     * @param string[] $values  Optional. The string values of the choices
+     * @param callable $value   The callable for creating the value for a
+     *                          choice. If `null` is passed, the choices are
+     *                          cast to strings and used as values
     *
     * @throws InvalidArgumentException If the keys of the choices don't match
     *                                  the keys of the values or if any of the
     *                                  choices is not scalar
     */
-    public function __construct(array $choices, array $values = array())
+    public function __construct(array $choices, $value = null)
    {
-        if (empty($values)) {
-            // The cast to strings happens later
-            $values = $choices;
-        } else {
-            $choiceKeys = array_keys($choices);
-            $valueKeys = array_keys($values);
-
-            if ($choiceKeys !== $valueKeys) {
-                throw new InvalidArgumentException(
-                    sprintf(
-                        'The keys of the choices and the values must match. The choice '.
-                        'keys are: "%s". The value keys are: "%s".',
-                        implode('", "', $choiceKeys),
-                        implode('", "', $valueKeys)
-                    )
-                );
-            }
-        }
-
-        $this->choices = array_map(array(__CLASS__, 'toArrayKey'), $choices);
-        $this->values = array_map('strval', $values);
-    }
+        $choices = array_map(array(__CLASS__, 'toArrayKey'), $choices);

-    /**
-     * {@inheritdoc}
-     */
-    public function getChoices()
-    {
-        return $this->choices;
-    }
+        if (null === $value) {
+            $value = function ($choice) {
+                return (string) $choice;
+            };
+            $this->useChoicesAsValues = true;
+        }

-    /**
-     * {@inheritdoc}
-     */
-    public function getValues()
-    {
-        return $this->values;
+        parent::__construct($choices, $value);
    }

    /**
     * {@inheritdoc}
     */
    public function getChoicesForValues(array $values)
    {
-        $values = array_map('strval', $values);
+        if ($this->useChoicesAsValues) {
+            $values = array_map('strval', $values);
+
+            // If the values are identical to the choices, so we can just return
+            // them to improve performance a little bit
+            return array_map(array(__CLASS__, 'toArrayKey'), array_intersect($values, $this->values));
+        }

-        // The values are identical to the choices, so we can just return them
-        // to improve performance a little bit
-        return array_map(array(__CLASS__, 'toArrayKey'), array_intersect($values, $this->values));
+        return parent::getChoicesForValues($values);
    }

    /**
@@ -166,8 +138,12 @@ public function getValuesForChoices(array $choices)
    {
        $choices = array_map(array(__CLASS__, 'toArrayKey'), $choices);

-        // The choices are identical to the values, so we can just return them
-        // to improve performance a little bit
-        return array_map('strval', array_intersect($choices, $this->choices));
+        if ($this->useChoicesAsValues) {
+            // If the choices are identical to the values, we can just return
+            // them to improve performance a little bit
+            return array_map('strval', array_intersect($choices, $this->choices));
+        }
+
+        return parent::getValuesForChoices($choices);
    }
 }
diff --git a/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php b/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php
@@ -89,10 +89,6 @@ public function createListFromChoices($choices, $value = null)
            throw new UnexpectedTypeException($choices, 'array or \Traversable');
        }

-        if (null !== $value && !is_callable($value)) {
-            throw new UnexpectedTypeException($value, 'null or callable');
-        }
-
        if ($choices instanceof \Traversable) {
            $choices = iterator_to_array($choices);
        }
@@ -102,29 +98,7 @@ public function createListFromChoices($choices, $value = null)
        // in the view only.
        self::flatten($choices, $flatChoices);

-        // If no values are given, use incrementing integers as values
-        // We can not use the choices themselves, because we don't know whether
-        // choices can be converted to (duplicate-free) strings
-        if (null === $value) {
-            $values = $flatChoices;
-            $i = 0;
-
-            foreach ($values as $key => $value) {
-                $values[$key] = (string) $i++;
-            }
-
-            return new ArrayChoiceList($flatChoices, $values);
-        }
-
-        // Can't use array_map(), because array_map() doesn't pass the key
-        // Can't use array_walk(), which ignores the return value of the
-        // closure
-        $values = array();
-        foreach ($flatChoices as $key => $choice) {
-            $values[$key] = call_user_func($value, $choice, $key);
-        }
-
-        return new ArrayChoiceList($flatChoices, $values);
+        return new ArrayChoiceList($flatChoices, $value);
    }

    /**
@@ -139,10 +113,6 @@ public function createListFromFlippedChoices($choices, $value = null)
            throw new UnexpectedTypeException($choices, 'array or \Traversable');
        }

-        if (null !== $value && !is_callable($value)) {
-            throw new UnexpectedTypeException($value, 'null or callable');
-        }
-
        if ($choices instanceof \Traversable) {
            $choices = iterator_to_array($choices);
        }
@@ -157,20 +127,12 @@ public function createListFromFlippedChoices($choices, $value = null)
        // strings or integers, we are guaranteed to be able to convert them
        // to strings
        if (null === $value) {
-            $values = array_map('strval', $flatChoices);
-
-            return new ArrayKeyChoiceList($flatChoices, $values);
-        }
-
-        // Can't use array_map(), because array_map() doesn't pass the key
-        // Can't use array_walk(), which ignores the return value of the
-        // closure
-        $values = array();
-        foreach ($flatChoices as $key => $choice) {
-            $values[$key] = call_user_func($value, $choice, $key);
+            $value = function ($choice) {
+                return (string) $choice;
+            };
        }

-        return new ArrayKeyChoiceList($flatChoices, $values);
+        return new ArrayKeyChoiceList($flatChoices, $value);
    }

    /**

diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php
@@ -29,7 +29,9 @@ protected function setUp()

    protected function createChoiceList()
    {
-        return new ArrayChoiceList($this->getChoices(), $this->getValues());
+        $i = 0;
+
+        return new ArrayChoiceList($this->getChoices());
    }

    protected function getChoices()
@@ -49,4 +51,45 @@ public function testFailIfKeyMismatch()
    {
        new ArrayChoiceList(array(0 => 'a', 1 => 'b'), array(1 => 'a', 2 => 'b'));
    }
+
+    public function testCreateChoiceListWithValueCallback()
+    {
+        $callback = function ($choice, $key) {
+            return $key.':'.$choice;
+        };
+
+        $choiceList = new ArrayChoiceList(array(2 => 'foo', 7 => 'bar', 10 => 'baz'), $callback);
+
+        $this->assertSame(array(2 => '2:foo', 7 => '7:bar', 10 => '10:baz'), $choiceList->getValues());
+        $this->assertSame(array(1 => 'foo', 2 => 'baz'), $choiceList->getChoicesForValues(array(1 => '2:foo', 2 => '10:baz')));
+        $this->assertSame(array(1 => '2:foo', 2 => '10:baz'), $choiceList->getValuesForChoices(array(1 => 'foo', 2 => 'baz')));
+    }
+
+    public function testCompareChoicesByIdentityByDefault()
+    {
+        $callback = function ($choice) {
+            return $choice->value;
+        };
+
+        $obj1 = (object) array('value' => 'value1');
+        $obj2 = (object) array('value' => 'value2');
+
+        $choiceList = new ArrayChoiceList(array($obj1, $obj2), $callback);
+        $this->assertSame(array(2 => 'value2'), $choiceList->getValuesForChoices(array(2 => $obj2)));
+        $this->assertSame(array(), $choiceList->getValuesForChoices(array(2 => (object) array('value' => 'value2'))));
+    }
+
+    public function testCompareChoicesWithValueCallbackIfCompareByValue()
+    {
+        $callback = function ($choice) {
+            return $choice->value;
+        };
+
+        $obj1 = (object) array('value' => 'value1');
+        $obj2 = (object) array('value' => 'value2');
+
+        $choiceList = new ArrayChoiceList(array($obj1, $obj2), $callback, true);
+        $this->assertSame(array(2 => 'value2'), $choiceList->getValuesForChoices(array(2 => $obj2)));
+        $this->assertSame(array(2 => 'value2'), $choiceList->getValuesForChoices(array(2 => (object) array('value' => 'value2'))));
+    }
 }