From a289deb97358cf7295bbd1410d954f2d66c5346e Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 25 Mar 2015 15:45:45 +0100 Subject: [PATCH] [Form] Fixed new ArrayChoiceList to compare choices by their values, if enabled --- .../Form/ChoiceList/ArrayChoiceList.php | 60 +++++++++---- .../Form/ChoiceList/ArrayKeyChoiceList.php | 84 +++++++------------ .../Factory/DefaultChoiceListFactory.php | 48 ++--------- .../Tests/ChoiceList/ArrayChoiceListTest.php | 45 +++++++++- .../ChoiceList/ArrayKeyChoiceListTest.php | 57 +++++++------ .../Factory/DefaultChoiceListFactoryTest.php | 3 +- 6 files changed, 155 insertions(+), 142 deletions(-) diff --git a/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php b/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php index 0dfc0f9945..a3987cc02c 100644 --- 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 @@ class ArrayChoiceList implements ChoiceListInterface { $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 index d79747e048..918c278f06 100644 --- a/src/Symfony/Component/Form/ChoiceList/ArrayKeyChoiceList.php +++ b/src/Symfony/Component/Form/ChoiceList/ArrayKeyChoiceList.php @@ -43,21 +43,14 @@ use Symfony\Component\Form\Exception\InvalidArgumentException; * @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,51 +93,26 @@ class ArrayKeyChoiceList implements ChoiceListInterface * 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); + $choices = array_map(array(__CLASS__, 'toArrayKey'), $choices); - 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) { + $value = function ($choice) { + return (string) $choice; + }; + $this->useChoicesAsValues = true; } - $this->choices = array_map(array(__CLASS__, 'toArrayKey'), $choices); - $this->values = array_map('strval', $values); - } - - /** - * {@inheritdoc} - */ - public function getChoices() - { - return $this->choices; - } - - /** - * {@inheritdoc} - */ - public function getValues() - { - return $this->values; + parent::__construct($choices, $value); } /** @@ -152,11 +120,15 @@ class ArrayKeyChoiceList implements ChoiceListInterface */ public function getChoicesForValues(array $values) { - $values = array_map('strval', $values); + if ($this->useChoicesAsValues) { + $values = array_map('strval', $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)); + // 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)); + } + + return parent::getChoicesForValues($values); } /** @@ -166,8 +138,12 @@ class ArrayKeyChoiceList implements ChoiceListInterface { $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 index dd191eea39..f0ea07cdf6 100644 --- a/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php +++ b/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php @@ -89,10 +89,6 @@ class DefaultChoiceListFactory implements ChoiceListFactoryInterface 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 @@ class DefaultChoiceListFactory implements ChoiceListFactoryInterface // 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 @@ class DefaultChoiceListFactory implements ChoiceListFactoryInterface 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 @@ class DefaultChoiceListFactory implements ChoiceListFactoryInterface // 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); + $value = function ($choice) { + return (string) $choice; + }; } - // 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 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 index 34b22fe041..0dffd08374 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php @@ -29,7 +29,9 @@ class ArrayChoiceListTest extends AbstractChoiceListTest protected function createChoiceList() { - return new ArrayChoiceList($this->getChoices(), $this->getValues()); + $i = 0; + + return new ArrayChoiceList($this->getChoices()); } protected function getChoices() @@ -49,4 +51,45 @@ class ArrayChoiceListTest extends AbstractChoiceListTest { 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')))); + } } diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/ArrayKeyChoiceListTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/ArrayKeyChoiceListTest.php index 74cf2afb4a..78263502d6 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/ArrayKeyChoiceListTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/ArrayKeyChoiceListTest.php @@ -29,7 +29,7 @@ class ArrayKeyChoiceListTest extends AbstractChoiceListTest protected function createChoiceList() { - return new ArrayKeyChoiceList($this->getChoices(), $this->getValues()); + return new ArrayKeyChoiceList($this->getChoices()); } protected function getChoices() @@ -42,14 +42,6 @@ class ArrayKeyChoiceListTest extends AbstractChoiceListTest return array('0', '1', 'a', 'b', ''); } - /** - * @expectedException \Symfony\Component\Form\Exception\InvalidArgumentException - */ - public function testFailIfKeyMismatch() - { - new ArrayKeyChoiceList(array(0 => 'a', 1 => 'b'), array(1 => 'a', 2 => 'b')); - } - public function testUseChoicesAsValuesByDefault() { $list = new ArrayKeyChoiceList(array(1 => '', 3 => 0, 7 => '1', 10 => 1.23)); @@ -102,7 +94,7 @@ class ArrayKeyChoiceListTest extends AbstractChoiceListTest */ public function testConvertChoicesIfNecessary(array $choices, array $converted) { - $list = new ArrayKeyChoiceList($choices, range(0, count($choices) - 1)); + $list = new ArrayKeyChoiceList($choices); $this->assertSame($converted, $list->getChoices()); } @@ -134,7 +126,7 @@ class ArrayKeyChoiceListTest extends AbstractChoiceListTest */ public function testFailIfInvalidChoices(array $choices) { - new ArrayKeyChoiceList($choices, range(0, count($choices) - 1)); + new ArrayKeyChoiceList($choices); } /** @@ -157,31 +149,48 @@ class ArrayKeyChoiceListTest extends AbstractChoiceListTest /** * @dataProvider provideConvertibleValues */ - public function testConvertValuesToStrings(array $values, array $converted) + public function testConvertValuesToStrings($value, $converted) { - $list = new ArrayKeyChoiceList(range(0, count($values) - 1), $values); + $callback = function () use ($value) { + return $value; + }; - $this->assertSame($converted, $list->getValues()); + $list = new ArrayKeyChoiceList(array('choice'), $callback); + + $this->assertSame(array($converted), $list->getValues()); } public function provideConvertibleValues() { return array( - array(array(0), array('0')), - array(array(1), array('1')), - array(array('0'), array('0')), - array(array('1'), array('1')), - array(array('1.23'), array('1.23')), - array(array('foobar'), array('foobar')), + array(0, '0'), + array(1, '1'), + array('0', '0'), + array('1', '1'), + array('1.23', '1.23'), + array('foobar', 'foobar'), // The default value of choice fields is NULL. It should be treated // like the empty value for this choice list type - array(array(null), array('')), - array(array(1.23), array('1.23')), + array(null, ''), + array(1.23, '1.23'), // Always cast booleans to 0 and 1, because: // array(true => 'Yes', false => 'No') === array(1 => 'Yes', 0 => 'No') // see ChoiceTypeTest::testSetDataSingleNonExpandedAcceptsBoolean - array(array(true), array('1')), - array(array(false), array('')), + array(true, '1'), + array(false, ''), ); } + + public function testCreateChoiceListWithValueCallback() + { + $callback = function ($choice, $key) { + return $key.':'.$choice; + }; + + $choiceList = new ArrayKeyChoiceList(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'))); + } } diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/Factory/DefaultChoiceListFactoryTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/Factory/DefaultChoiceListFactoryTest.php index 42f745e29b..b144699892 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/Factory/DefaultChoiceListFactoryTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/Factory/DefaultChoiceListFactoryTest.php @@ -83,8 +83,7 @@ class DefaultChoiceListFactoryTest extends \PHPUnit_Framework_TestCase $this->obj3 = (object) array('label' => 'C', 'index' => 'y', 'value' => 1, 'preferred' => true, 'group' => 'Group 2', 'attr' => array('attr2' => 'value2')); $this->obj4 = (object) array('label' => 'D', 'index' => 'z', 'value' => 2, 'preferred' => false, 'group' => 'Group 2', 'attr' => array()); $this->list = new ArrayChoiceList( - array('A' => $this->obj1, 'B' => $this->obj2, 'C' => $this->obj3, 'D' => $this->obj4), - array('A' => '0', 'B' => '1', 'C' => '2', 'D' => '3') + array('A' => $this->obj1, 'B' => $this->obj2, 'C' => $this->obj3, 'D' => $this->obj4) ); $this->factory = new DefaultChoiceListFactory(); }