8000 merged branch bschussek/issue3839 (PR #3859) · symfony/symfony@517f8e0 · GitHub
[go: up one dir, main page]
Skip to content

Commit 517f8e0

Browse files
fabpotfabpot
committed
merged branch bschussek/issue3839 (PR #3859)
Commits ------- 61d792e [Form] Changed checkboxes in an expanded multiple-choice field to not include the choice index bc9bc4a [Form] Fixed behavior of expanded multiple-choice field when submitted without ticks 2e07256 [Form] Simplified choice list API 2645120 [Form] Fixed handling of expanded choice lists, checkboxes and radio buttons with empty values ("") Discussion ---------- [Form] Fixed handling of empty values in checkbox/radio/choice type Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #3839, #3366 Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3839) This PR fixes the processing of checkboxes and radio buttons with empty "value" attributes as well as of expanded choice forms with empty values. Additionally, some unnecessary complexity has been removed of the new ChoiceList API. --------------------------------------------------------------------------- by stof at 2012-04-10T13:56:12Z You probably need to change some things in the CHANGELOG file too --------------------------------------------------------------------------- by bschussek at 2012-04-10T14:39:24Z No. This is an update of a previous post-2.0 PR, the CHANGELOG is still accurate. --------------------------------------------------------------------------- by stof at 2012-04-10T14:46:47Z well, doesn't it require changes to the description of previous changes related to the ChoiceList ? It does in the UPGRADE file so it is weird if the CHANGELOG does not require any change. --------------------------------------------------------------------------- by bschussek at 2012-04-10T14:56:05Z Feel free to check yourself :)
2 parents 48abdaf + 61d792e commit 517f8e0

32 files changed

+448
-480
lines changed

‎CHANGELOG-2.1.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
265265
set to not required now, as stated in the docs
266266
* fixed TimeType and DateTimeType to not display seconds when "widget" is
267267
"single_text" unless "with_seconds" is set to true
268+
* checkboxes of in an expanded multiple-choice field don't include the choice
269+
in their name anymore. Their names terminate with "[]" now.
268270

269271
### HttpFoundation
270272

‎UPGRADE-2.1.md‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -158,25 +158,11 @@ UPGRADE FROM 2.0 to 2.1
158158
`getChoices()` and `getChoicesByValues()`. For the latter two, no
159159
replacement exists.
160160
161-
* The strategy for generating the `id` and `name` HTML attributes for choices
162-
in a choice field has changed.
161+
* The strategy for generating the `id` and `name` HTML attributes for
162+
checkboxes and radio buttons in a choice field has changed.
163163
164164
Instead of appending the choice value, a generated integer is now appended
165-
by default. Take care if your JavaScript relies on the old behavior. If you
166-
can guarantee that your choice values only contain ASCII letters, digits,
167-
colons and underscores, you can restore the old behavior by setting the
168-
`index_strategy` choice field option to `ChoiceList::COPY_CHOICE`.
169-
170-
* The strategy for generating the `value` HTML attribute for choices in a
171-
choice field has changed.
172-
173-
Instead of using the choice value, a generated integer is now stored. Again,
174-
take care if your JavaScript reads this value. If your choice field is a
175-
non-expanded single-choice field, or if the choices are guaranteed not to
176-
contain the empty string '' (which is the case when you added it manually
177-
or when the field is a single-choice field and is not required), you can
178-
restore the old behavior by setting the `value_strategy` choice field option
179-
to `ChoiceList::COPY_CHOICE`.
165+
by default. Take care if your JavaScript relies on that.
180166
181167
* In the choice field type's template, the structure of the `choices` variable
182168
has changed.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ protected function createIndex($entity)
319319
protected function createValue($entity)
320320
{
321321
if (count($this->identifier) === 1) {
322-
return current($this->getIdentifierValues($entity));
322+
return (string) current($this->getIdentifierValues($entity));
323323
}
324324

325325
return parent::createValue(< 6293 span class=pl-c1>$entity);

‎src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -442,8 +442,8 @@ public function testSubmitSingleExpanded()
442442
$this->assertSame($entity2, $field->getData());
443443
$this->assertFalse($field['1']->getData());
444444
$this->assertTrue($field['2']->getData());
445-
$this->assertSame('', $field['1']->getClientData());
446-
$this->assertSame('1', $field['2']->getClientData());
445+
$this->assertNull($field['1']->getClientData());
446+
$this->assertSame('2', $field['2']->getClientData());
447447
}
448448

449449
public function testSubmitMultipleExpanded()
@@ -462,7 +462,7 @@ public function testSubmitMultipleExpanded()
462462
'property' => 'name',
463463
));
464464

465-
$field->bind(array('1' => '1', '3' => '3'));
465+
$field->bind(array('1', '3'));
466466

467467
$expected = new ArrayCollection(array($entity1, $entity3));
468468

@@ -472,8 +472,8 @@ public function testSubmitMultipleExpanded()
472472
$this->assertFalse($field['2']->getData());
473473
$this->assertTrue($field['3']->getData());
474474
$this->assertSame('1', $field['1']->getClientData());
475-
$this->assertSame('', $field['2']->getClientData());
476-
$this->assertSame('1', $field['3']->getClientData());
475+
$this->assertNull($field['2']->getClientData());
476+
$this->assertSame('3', $field['3']->getClientData());
477477
}
478478

479479
public function testOverrideChoices()

Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ protected function createIndex($model)
304304
protected function createValue($model)
305305
{
306306
if (1 === count($this->identifier)) {
307-
return current($this->getIdentifierValues($model));
307+
return (string) current($this->getIdentifierValues($model));
308308
}
309309

310310
return parent::createValue($model);

Original file line numberDiff line numberDiff line change
@@ -32,29 +32,6 @@
3232
*/
3333
class ChoiceList implements ChoiceListInterface
3434
{
35-
/**
36-
* Strategy creating new indices/values by creating a copy of the choice.
37-
*
38-
* This strategy can only be used for index creation if choices are
39-
* guaranteed to only contain ASCII letters, digits and underscores.
40-
*
41-
* It can be used for value creation if choices can safely be cast into
42-
* a (unique) string.
43-
*
44-
* @var integer
45-
*/
46-
const COPY_CHOICE = 0;
47-
48-
/**
49-
* Strategy creating new indices/values by generating a new integer.
50-
*
51-
* This strategy can always be applied, but leads to loss of information
52-
* in the HTML source code.
53-
*
54-
* @var integer
55-
*/
56-
const GENERATE = 1;
57-
5835
/**
5936
* The choices with their indices as keys.
6037
*
@@ -85,24 +62,6 @@ class ChoiceList implements ChoiceListInterface
8562
*/
8663
private $remainingViews = array();
8764

88-
/**
89-
* The strategy used for creating choice indices.
90-
*
91-
* @var integer
92-
* @see COPY_CHOICE
93-
* @see GENERATE
94-
*/
95-
private $indexStrategy;
96-
97-
/**
98-
* The strategy used for creating choice values.
99-
*
100-
* @var integer
101-
* @see COPY_CHOICE
102-
* @see GENERATE
103-
*/
104-
private $valueStrategy;
105-
10665
/**
10766
* Creates a new choice list.
10867
*
@@ -115,16 +74,9 @@ class ChoiceList implements ChoiceListInterface
11574
* should match the structure of $choices.
11675
* @param array $preferredChoices A flat array of choices that should be
11776
* presented to the user with priority.
118-
* @param integer $valueStrategy The strategy used to create choice values.
119-
* One of COPY_CHOICE and GENERATE.
120-
* @param integer $indexStrategy The strategy used to create choice indices.
121-
* One of COPY_CHOICE and GENERATE.
12277
*/
123-
public function __construct($choices, array $labels, array $preferredChoices = array(), $valueStrategy = self::GENERATE, $indexStrategy = self::GENERATE)
78+
public function __construct($choices, array $labels, array $preferredChoices = array())
12479
{
125-
$this->valueStrategy = $valueStrategy;
126-
$this->indexStrategy = $indexStrategy;
127-
12880
$this->initialize($choices, $labels, $preferredChoices);
12981
}
13082

@@ -191,13 +143,6 @@ public function getRemainingViews()
191143
public function getChoicesForValues(array $values)
192144
{
193145
$values = $this->fixValues($values);
194-
195-
// If the values are identical to the choices, we can just return them
196-
// to improve performance a little bit
197-
if (self::COPY_CHOICE === $this->valueStrategy) {
198-
return $this->fixChoices(array_intersect($values, $this->values));
199-
}
200-
201146
$choices = array();
202147

203148
foreach ($values as $j => $givenValue) {
@@ -222,13 +167,6 @@ public function getChoicesForValues(array $values)
222167
public function getValuesForChoices(array $choices)
223168
{
224169
$choices = $this->fixChoices($choices);
225-
226-
// If the values are identical to the choices, we can just return them
227-
// to improve performance a little bit
228-
if (self::COPY_CHOICE === $this->valueStrategy) {
229-
return $this->fixValues(array_intersect($choices, $this->choices));
230-
}
231-
232170
$values = array();
233171

234172
foreach ($this->choices as $i => $choice) {
@@ -398,17 +336,15 @@ protected function addChoice(&$bucketForPreferred, &$bucketForRemaining, $choice
398336
$index = $this->createIndex($choice);
399337

400338
if ('' === $index || null === $index || !Form::isValidName((string)$index)) {
401-
throw new InvalidConfigurationException('The choice list index "' . $index . '" is invalid. Please set the choice field option "index_generation" to ChoiceList::GENERATE.');
339+
throw new InvalidConfigurationException('The index "' . $index . '" created by the choice list is invalid. It should be a valid, non-empty Form name.');
402340
}
403341

404342
$value = $this->createValue($choice);
405343

406-
if (!is_scalar($value)) {
407-
throw new InvalidConfigurationException('The choice list value of type "' . gettype($value) . '" should be a scalar. Please set the choice field option "value_generation" to ChoiceList::GENERATE.');
344+
if (!is_string($value)) {
345+
throw new InvalidConfigurationException('The value created by the choice list is of type "' . gettype($value) . '", but should be a string.');
408346
}
409347

410-
// Always store values as strings to facilitate comparisons
411-
$value = $this->fixValue($value);
412348
$view = new ChoiceView($value, $label);
413349

414350
$this->choices[$index] = $this->fixChoice($choice);
@@ -448,29 +384,23 @@ protected function isPreferred($choice, $preferredChoices)
448384
*/
449385
protected function createIndex($choice)
450386
{
451-
if (self::COPY_CHOICE === $this->indexStrategy) {
452-
return $choice;
453-
}
454-
455387
return count($this->choices);
456388
}
457389

458390
/**
459391
* Creates a new unique value for this choice.
460392
*
461-
* Extension point to change the value strategy.
393+
* By default, an integer is generated since it cannot be guaranteed that
394+
* all values in the list are convertible to (unique) strings. Subclasses
395+
* can override this behaviour if they can guarantee this property.
462396
*
463397
* @param mixed $choice The choice to create a value for
464398
*
465-
* @return integer|string A unique value without character limitations.
399+
* @return string A unique string.
466400
*/
467401
protected function createValue($choice)
468402
{
469-
if (self::COPY_CHOICE === $this->valueStrategy) {
470-
return $choice;
471-
}
472-
473-
return count($this->values);
403+
return (string) count($this->values);
474404
}
475405

476406
/**

Original file line numberDiff line numberDiff line change
@@ -14,17 +14,12 @@
1414
/**
1515
* Contains choices that can be selected in a form field.
1616
*
17-
* Each choice has four different properties:
17+
* Each choice has three different properties:
1818
*
1919
* - Choice: The choice that should be returned to the application by the
2020
* choice field. Can be any scalar value or an object, but no
2121
* array.
2222
* - Label: A text representing the choice that is displayed to the user.
23-
* - Index: A uniquely identifying index that should only contain ASCII
24-
* characters, digits and underscores. This index is used to
25-
* identify the choice in the HTML "id" and "name" attributes.
26-
* It is also used as index of the arrays returned by the various
27-
* getters of this class.
2823
* - Value: A uniquely identifying value that can contain arbitrary
2924
* characters, but no arrays or objects. This value is displayed
3025
* in the HTML "value" attribute.

Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@
1919
/**
2020
* A choice list for object choices.
2121
*
22-
* Supports generation of choice labels, choice groups, choice values and
23-
* choice indices by calling getters of the object (or associated objects).
22+
* Supports generation of choice labels, choice groups and choice values
23+
* by calling getters of the object (or associated objects).
2424
*
2525
* <code>
2626
* $choices = array($user1, $user2);
@@ -54,13 +54,6 @@ class ObjectChoiceList extends ChoiceList
5454
*/
5555
private $valuePath;
5656

57-
/**
58-
* The property path used to obtain the choice index.
59-
*
60-
* @var PropertyPath
61-
*/
62-
private $indexPath;
63-
6457
/**
6558
* Creates a new object choice list.
6659
*
@@ -82,18 +75,14 @@ class ObjectChoiceList extends ChoiceList
8275
* @param string $valuePath A property path pointing to the property used
8376
* for the choice values. If not given, integers
8477
* are generated instead.
85-
* @param string $indexPath A property path pointing to the property used
86-
* for the choice indices. If not given, integers
87-
* are generated instead.
8878
*/
89-
public function __construct($choices, $labelPath = null, array $preferredChoices = array(), $groupPath = null, $valuePath = null, $indexPath = null)
79+
public function __construct($choices, $labelPath = null, array $preferredChoices = array(), $groupPath = null, $valuePath = null)
9080
{
9181
$this->labelPath = $labelPath ? new PropertyPath($labelPath) : null;
9282
$this->groupPath = $groupPath ? new PropertyPath($groupPath) : null;
9383
$this->valuePath = $valuePath ? new PropertyPath($valuePath) : null;
94-
$this->indexPath = $indexPath ? new PropertyPath($indexPath) : null;
9584

96-
parent::__construct($choices, array(), $preferredChoices, self::GENERATE, self::GENERATE);
85+
parent::__construct($choices, array(), $preferredChoices);
9786
}
9887

9988
/**
@@ -148,27 +137,6 @@ protected function initialize($choices, array $labels, array $preferredChoices)
148137
parent::initialize($choices, $labels, $preferredChoices);
149138
}
150139

151-
/**
152-
* Creates a new unique index for this choice.
153-
*
154-
* If a property path for the index was given at object creation,
155-
* the getter behind that path is now called to obtain a new value.
156-
*
157-
* Otherwise a new integer is generated.
158-
*
159-
* @param mixed $choice The choice to create an index for
160-
* @return integer|string A unique index containing only ASCII letters,
161-
* digits and underscores.
162-
*/
163-
protected function createIndex($choice)
164-
{
165-
if ($this->indexPath) {
166-
return $this->indexPath->getValue($choice);
167-
}
168-
169-
return parent::createIndex($choice);
170-
}
171-
172140
/**
173141
* Creates a new unique value for this choice.
174142
*
@@ -183,7 +151,7 @@ protected function createIndex($choice)
183151
protected function createValue($choice)
184152
{
185153
if ($this->valuePath) {
186-
return $this->valuePath->getValue($choice);
154+
return (string) $this->valuePath->getValue($choice);
187155
}
188156

189157
return parent::createValue($choice);