Skip to content

Commit 76f7978

Browse files
committed
bug #29695 [Form] Do not ignore the choice groups for caching (vudaltsov)
This PR was merged into the 3.4 branch. Discussion ---------- [Form] Do not ignore the choice groups for caching | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a While working on a different issue I suddenly came over a strange behaviour. ```php $builder ->add('choice1', ChoiceType::class, [ 'choices' => [ 'a' => 'a', 'b' => 'b', 'c' => 'c', ], 'multiple' => true, ]) ->add('choice2', ChoiceType::class, [ 'choices' => [ 'ab' => [ 'a' => 'a', 'b' => 'b', ], 'c' => 'c', ], 'multiple' => true, ]); ``` The code above will result in two identical selects: ![image](https://user-images.githubusercontent.com/2552865/50459865-b3e36980-0980-11e9-8f3d-17f9cfa9a7f8.png) The reason for this is hash generation in `Symfony\Component\Form\ChoiceList\Factory\CachingFactoryDecorator::createListFromChoices()` — it does not take array structure into account. See [the comment and the code below it](https://github.com/symfony/symfony/blob/7f46dfb1c4ce5e45a5f1918ad6223a3bbdd52a0b/src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php#L116). The comment says that the same choice list should be returned for the same collection of choices no matter the structure. This is wrong, because `ChoiceListInterface` has a method `getStructuredValues()` and thus a list instance cannot identified by a hash which does not take structure into account. I propose a simple change that fixes this and allows for similar choice fields with different groupings. ping @HeahDude Commits ------- 9007911a85 Do not ignore the choice groups for caching
2 parents 59f5385 + afe69ef commit 76f7978

File tree

2 files changed

+12
-36
lines changed

2 files changed

+12
-36
lines changed

ChoiceList/Factory/CachingFactoryDecorator.php

Lines changed: 1 addition & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -62,30 +62,6 @@ public static function generateHash($value, $namespace = '')
6262
return hash('sha256', $namespace.':'.serialize($value));
6363
}
6464

65-
/**
66-
* Flattens an array into the given output variable.
67-
*
68-
* @param array $array The array to flatten
69-
* @param array $output The flattened output
70-
*
71-
* @internal
72-
*/
73-
private static function flatten(array $array, &$output)
74-
{
75-
if (null === $output) {
76-
$output = array();
77-
}
78-
79-
foreach ($array as $key => $value) {
80-
if (\is_array($value)) {
81-
self::flatten($value, $output);
82-
continue;
83-
}
84-
85-
$output[$key] = $value;
86-
}
87-
}
88-
8965
public function __construct(ChoiceListFactoryInterface $decoratedFactory)
9066
{
9167
$this->decoratedFactory = $decoratedFactory;
@@ -113,12 +89,7 @@ public function createListFromChoices($choices, $value = null)
11389
// The value is not validated on purpose. The decorated factory may
11490
// decide which values to accept and which not.
11591

116-
// We ignore the choice groups for caching. If two choice lists are
117-
// requested with the same choices, but a different grouping, the same
118-
// choice list is returned.
119-
self::flatten($choices, $flatChoices);
120-
121-
$hash = self::generateHash(array($flatChoices, $value), 'fromChoices');
92+
$hash = self::generateHash(array($choices, $value), 'fromChoices');
12293

12394
if (!isset($this->lists[$hash])) {
12495
$this->lists[$hash] = $this->decoratedFactory->createListFromChoices($choices, $value);

Tests/ChoiceList/Factory/CachingFactoryDecoratorTest.php

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,19 +64,24 @@ public function testCreateFromChoicesComparesTraversableChoicesAsArray()
6464
$this->assertSame($list, $this->factory->createListFromChoices($choices2));
6565
}
6666

67-
public function testCreateFromChoicesFlattensChoices()
67+
public function testCreateFromChoicesGroupedChoices()
6868
{
6969
$choices1 = array('key' => array('A' => 'a'));
7070
$choices2 = array('A' => 'a');
71-
$list = new \stdClass();
71+
$list1 = new \stdClass();
72+
$list2 = new \stdClass();
7273

73-
$this->decoratedFactory->expects($this->once())
74+
$this->decoratedFactory->expects($this->at(0))
7475
->method('createListFromChoices')
7576
->with($choices1)
76-
->will($this->returnValue($list));
77+
->will($this->returnValue($list1));
78+
$this->decoratedFactory->expects($this->at(1))
79+
->method('createListFromChoices')
80+
->with($choices2)
81+
->will($this->returnValue($list2));
7782

78-
$this->assertSame($list, $this->factory->createListFromChoices($choices1));
79-
$this->assertSame($list, $this->factory->createListFromChoices($choices2));
83+
$this->assertSame($list1, $this->factory->createListFromChoices($choices1));
84+
$this->assertSame($list2, $this->factory->createListFromChoices($choices2));
8085
}
8186

8287
/**

0 commit comments

Comments
 (0)