Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 44bb346

Browse filesBrowse files
fabpotfabpot
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 ------- 9007911 Do not ignore the choice groups for caching
2 parents c573cfb + 9007911 commit 44bb346
Copy full SHA for 44bb346

File tree

Expand file treeCollapse file tree

2 files changed

+12
-36
lines changed
Filter options
Expand file treeCollapse file tree

2 files changed

+12
-36
lines changed

‎src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
+1-30Lines 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);

‎src/Symfony/Component/Form/Tests/ChoiceList/Factory/CachingFactoryDecoratorTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Form/Tests/ChoiceList/Factory/CachingFactoryDecoratorTest.php
+11-6Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.