8000 merged branch bschussek/issue3293 (PR #3315) · symfony/symfony@78e6a2f · GitHub
[go: up one dir, main page]
Skip to content

Commit 78e6a2f

Browse files
fabpotfabpot
committed
merged branch bschussek/issue3293 (PR #3315)
Commits ------- da2447e [Form] Fixed MergeCollectionListener when used with a custom property path b56502f [Form] Added getParent() to PropertyPath 7e5104e [Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper (as happening in CollectionType) Discussion ---------- [Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper Bug fix: yes Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #3293, #1499 Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3293) This fixes CollectionType to properly use adders and removers. Apart from that, adders and removers now work with custom property paths. PropertyPath was extended for a method `getParent`. --------------------------------------------------------------------------- by bschussek at 2012-02-10T07:42:13Z @fabpot Ready to merge.
2 parents c614be7 + da2447e commit 78e6a2f

File tree

6 files changed

+621
-174
lines changed

6 files changed

+621
-174
lines changed

‎src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php

Lines changed: 159 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,33 @@
1313

1414
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
1515
use Symfony\Component\Form\FormEvents;
16+
use Symfony\Component\Form\Event\DataEvent;
1617
use Symfony\Component\Form\Event\FilterDataEvent;
1718
use Symfony\Component\Form\Exception\UnexpectedTypeException;
1819
use Symfony\Component\Form\Exception\FormException;
1920
use Symfony\Component\Form\Util\FormUtil;
21+
use Symfony\Component\Form\Util\PropertyPath;
2022

2123
/**
2224
* @author Bernhard Schussek <bschussek@gmail.com>
2325
*/
2426
class MergeCollectionListener implements EventSubscriberInterface
2527
{
28+
/**
29+
* Strategy for merging the new collection into the old collection
30+
*
31+
* @var integer
32+
*/
33+
const MERGE_NORMAL = 1;
34+
35+
/**
36+
* Strategy for calling add/remove methods on the parent data for all
37+
* new/removed elements in the new collection
38+
*
39+
* @var integer
40+
*/
41+
const MERGE_INTO_PARENT = 2;
42+
2643
/**
2744
* Whether elements may be added to the collection
2845
* @var Boolean
@@ -39,7 +56,7 @@ class MergeCollectionListener implements EventSubscriberInterface
3956
* Whether to search for and use adder and remover methods
4057
* @var Boolean
4158
*/
42-
private $useAccessors;
59+
private $mergeStrategy;
4360

4461
/**
4562
* The name of the adder method to look for
@@ -53,35 +70,112 @@ class MergeCollectionListener implements EventSubscriberInterface
5370
*/
5471
private $removeMethod;
5572

56-
public function __construct($allowAdd = false, $allowDelete = false, $useAccessors = true, $addMethod = null, $removeMethod = null)
73+
/**
74+
* A copy of the data before starting binding for this form
75+
* @var mixed
76+
*/
77+
private $dataSnapshot;
78+
79+
/**
80+
* Creates a new listener.
81+
*
82+
* @param Boolean $allowAdd Whether values might be added to the
83+
* collection.
84+
* @param Boolean $allowDelete Whether values might be removed from the
85+
* collection.
86+
* @param integer $mergeStrategy Which strategy to use for merging the
87+
* bound collection with the original
88+
* collection. Might be any combination of
89+
* MERGE_NORMAL and MERGE_INTO_PARENT.
< F438 code>90+
* MERGE_INTO_PARENT has precedence over
91+
* MERGE_NORMAL if an adder/remover method
92+
* is found. The default strategy is to use
93+
* both strategies.
94+
* @param string $addMethod The name of the adder method to use. If
95+
* not given, the listener tries to discover
96+
* the method automatically.
97+
* @param string $removeMethod The name of the remover method to use. If
98+
* not given, the listener tries to discover
99+
* the method automatically.
100+
*
101+
* @throws FormException If the given strategy is invalid.
102+
*/
103+
public function __construct($allowAdd = false, $allowDelete = false, $mergeStrategy = null, $addMethod = null, $removeMethod = null)
57104
{
105+
if ($mergeStrategy && !($mergeStrategy & (self::MERGE_NORMAL | self::MERGE_INTO_PARENT))) {
106+
throw new FormException('The merge strategy needs to be at least MERGE_NORMAL or MERGE_INTO_PARENT');
107+
}
108+
58109
$this->allowAdd = $allowAdd;
59110
$this->allowDelete = $allowDelete;
60-
$this->useAccessors = $useAccessors;
111+
$this->mergeStrategy = $mergeStrategy ?: self::MERGE_NORMAL | self::MERGE_INTO_PARENT;
61112
$this->addMethod = $addMethod;
62113
$this->removeMethod = $removeMethod;
63114
}
64115

65116
static public function getSubscribedEvents()
66117
{
67-
return array(FormEvents::BIND_NORM_DATA => 'onBindNormData');
118+
return array(
119+
FormEvents::PRE_BIND => 'preBind',
120+
FormEvents::BIND_NORM_DATA => 'onBindNormData',
121+
);
122+
}
123+
124+
public function preBind(DataEvent $event)
125+
{
126+
// Get a snapshot of the current state of the normalized data
127+
// to compare against later
128+
$this->dataSnapshot = $event->getForm()->getNormData();
129+
130+
if (is_object($this->dataSnapshot)) {
131+
// Make sure the snapshot remains stable and doesn't change
132+
$this->dataSnapshot = clone $this->dataSnapshot;
133+
}
134+
135+
if (null !== $this->dataSnapshot && !is_array($this->dataSnapshot) && !($this->dataSnapshot instanceof \Traversable && $this->dataSnapshot instanceof \ArrayAccess)) {
136+
throw new UnexpectedTypeException($this->dataSnapshot, 'array or (\Traversable and \ArrayAccess)');
137+
}
68138
}
69139

70140
public function onBindNormData(FilterDataEvent $event)
71141
{
72-
$originalData = $event->getForm()->getData();
142+
$originalData = $event->getForm()->getNormData();
73143

74144
// If we are not allowed to change anything, return immediately
75145
if (!$this->allowAdd && !$this->allowDelete) {
146+
// Don't set to the snapshot as then we are switching from the
147+
// original object to its copy, which might break things
76148
$event->setData($originalData);
77149
return;
78150
}
79151

80152
$form = $event->getForm();
81153
$data = $event->getData();
82-
$parentData = $form->hasParent() ? $form->getParent()->getData() : null;
154+
$childPropertyPath = null;
155+
$parentData = null;
83156
$addMethod = null;
84157
$removeMethod = null;
158+
$propertyPath = null;
159+
$plural = null;
160+
161+
if ($form->hasParent() && $form->getAttribute('property_path')) {
162+
$propertyPath = new PropertyPath($form->getAttribute('property_path'));
163+
$childPropertyPath = $propertyPath;
164+
$parentData = $form->getParent()->getClientData();
165+
$lastElement = $propertyPath->getElement($propertyPath->getLength() - 1);
166+
167+
// If the property path contains more than one element, the parent
168+
// data is the object at the parent property path
169+
if ($propertyPath->getLength() > 1) {
170+
$parentData = $propertyPath->getParent()->getValue($parentData);
171+
172+
// Property path relative to $parentData
173+
$childPropertyPath = new PropertyPath($lastElement);
174+
}
175+
176+
// The plural form is the last element of the property path
177+
$plural = ucfirst($lastElement);
178+
}
85179

86180
if (null === $data) {
87181
$data = array();
@@ -96,24 +190,34 @@ public function onBindNormData(FilterDataEvent $event)
96190
}
97191

98192
// Check if the parent has matching methods to add/remove items
99-
if ($this->useAccessors && is_object($parentData)) {
193+
if (($this->mergeStrategy & self::MERGE_INTO_PARENT) && is_object($parentData)) {
100194
$reflClass = new \ReflectionClass($parentData);
101195
$addMethodNeeded = $this->allowAdd && !$this->addMethod;
102196
$removeMethodNeeded = $this->allowDelete && !$this->removeMethod;
103197

104198
// Any of the two methods is required, but not yet known
105199
if ($addMethodNeeded || $removeMethodNeeded) {
106-
$singulars = (array) FormUtil::singularify(ucfirst($form->getName()));
200+
$singulars = (array) FormUtil::singularify($plural);
107201

108202
foreach ($singulars as $singular) {
109203
// Try to find adder, but don't override preconfigured one
110204
if ($addMethodNeeded) {
111-
$addMethod = $this->checkMethod($reflClass, 'add' . $singular);
205+
$addMethod = 'add' . $singular;
206+
207+
// False alert
208+
if (!$this->isAccessible($reflClass, $addMethod, 1)) {
209+
$addMethod = null;
210+
}
112211
}
113212

114213
// Try to find remover, but don't override preconfigured one
115214
if ($removeMethodNeeded) {
116-
$removeMethod = $this->checkMethod($reflClass, 'remove' . $singular);
215+
$removeMethod = 'remove' . $singular;
216+
217+
// False alert
218+
if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
219+
$removeMethod = null;
220+
}
117221
}
118222

119223
// Found all that we need. Abort search.
@@ -129,38 +233,37 @@ public function onBindNormData(FilterDataEvent $event)
129233

130234
// Set preconfigured adder
131235
if ($this->allowAdd && $this->addMethod) {
132-
$addMethod = $this->checkMethod($reflClass, $this->addMethod);
236+
$addMethod = $this->addMethod;
133237

134-
if (!$addMethod) {
238+
if (!$this->isAccessible($reflClass, $addMethod, 1)) {
135239
throw new FormException(sprintf(
136-
'The method "%s" could not be found on class %s',
137-
$this->addMethod,
240+
'The public method "%s" could not be found on class %s',
241+
$addMethod,
138242
$reflClass->getName()
139243
));
140244
}
141245
}
142246

143247
// Set preconfigured remover
144248
if ($this->allowDelete && $this->removeMethod) {
145-
$removeMethod = $this->checkMethod($reflClass, $this->removeMethod);
249+
$removeMethod = $this->removeMethod;
146250

147-
if (!$removeMethod) {
251+
if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
148252
throw new FormException(sprintf(
149-
'The method "%s" could not be found on class %s',
150-
$this->removeMethod,
253+
'The public method "%s" could not be found on class %s',
254+
$removeMethod,
151255
$reflClass->getName()
152256
));
153257
}
154258
}
155259
}
156260

157-
// Check which items are in $data that are not in $originalData and
158-
// vice versa
261+
// Calculate delta between $data and the snapshot created in PRE_BIND
159262
$itemsToDelete = array();
160263
< 10000 span class=pl-s1>$itemsToAdd = is_object($data) ? clone $data : $data;
161264

162-
if ($originalData) {
163-
foreach ($originalData as $originalKey => $originalItem) {
265+
if ($this->dataSnapshot) {
266+
foreach ($this->dataSnapshot as $originalItem) {
164267
foreach ($data as $key => $item) {
165268
if ($item === $originalItem) {
166269
// Item found, next original item
@@ -170,7 +273,12 @@ public function onBindNormData(FilterDataEvent $event)
170273
}
171274

172275
// Item not found, remember for deletion
173-
$itemsToDelete[$originalKey] = $originalItem;
276+
foreach ($originalData as $key => $item) {
277+
if ($item === $originalItem) {
278+
$itemsToDelete[$key] = $item;
279+
continue 2;
280+
}
281+
}
174282
}
175283
}
176284

@@ -187,43 +295,47 @@ public function onBindNormData(FilterDataEvent $event)
187295
$parentData->$addMethod($item);
188296
}
189297
}
190-
} elseif (!$originalData) {
191-
// No original data was set. Set it if allowed
192-
if ($this->allowAdd) {
193-
$originalData = $data;
194-
}
195-
} else {
196-
// Original data is an array-like structure
197-
// Add and remove items in the original variable
198-
if ($this->allowDelete) {
199-
foreach ($itemsToDelete as $key => $item) {
200-
unset($originalData[$key]);
298+
299+
$event->setData($childPropertyPath->getValue($parentData));
300+
} elseif ($this->mergeStrategy & self::MERGE_NORMAL) {
301+
if (!$originalData) {
302+
// No original data was set. Set it if allowed
303+
if ($this->allowAdd) {
304+
$originalData = $data;
305+
}
306+
} else {
307+
// Original data is an array-like structure
308+
// Add and remove items in the original variable
309+
if ($this->allowDelete) {
310+
foreach ($itemsToDelete as $key => $item) {
311+
unset($originalData[$key]);
312+
}
201313
}
202-
}
203314

204-
if ($this->allowAdd) {
205-
foreach ($itemsToAdd as $key => $item) {
206-
if (!isset($originalData[$key])) {
207-
$originalData[$key] = $item;
208-
} else {
209-
$originalData[] = $item;
315+
if ($this->allowAdd) {
316+
foreach ($itemsToAdd as $key => $item) {
317+
if (!isset($originalData[$key])) {
318+
$originalData[$key] = $item;
319+
} else {
320+
$originalData[] = $item;
321+
}
210322
}
211323
}
212324
}
213-
}
214325

215-
$event->setData($originalData);
326+
$event->setData($originalData);
327+
}
216328
}
217329

218-
private function checkMethod(\ReflectionClass $reflClass, $methodName) {
330+
private function isAccessible(\ReflectionClass $reflClass, $methodName, $numberOfRequiredParameters) {
219331
if ($reflClass->hasMethod($methodName)) {
220332
$method = $reflClass->getMethod($methodName);
221333

222-
if ($method->isPublic() && $method->getNumberOfRequiredParameters() === 1) {
223-
return $methodName;
334+
if ($method->isPublic() && $method->getNumberOfRequiredParameters() === $numberOfRequiredParameters) {
335+
return true;
224336
}
225337
}
226338

227-
return null;
339+
return false;
228340
}
229341
}

‎src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,7 @@ public function buildForm(FormBuilder $builder, array $options)
8181

8282
if ($options['expanded']) {
8383
if ($options['multiple']) {
84-
$builder
85-
->appendClientTransformer(new ChoicesToBooleanArrayTransformer($options['choice_list']))
86-
->addEventSubscriber(new MergeCollectionListener(true, true))
87-
;
84+
$builder->appendClientTransformer(new ChoicesToBooleanArrayTransformer($options['choice_list']));
8885
} else {
8986
$builder
9087
->appendClientTransformer(new ChoiceToBooleanArrayTransformer($options['choice_list']))
@@ -93,24 +90,31 @@ public function buildForm(FormBuilder $builder, array $options)
9390
}
9491
} else {
9592
if ($options['multiple']) {
96-
$builder
97-
->appendClientTransformer(new ChoicesToValuesTransformer($options['choice_list']))
98-
->addEventSubscriber(new MergeCollectionListener(
99-
true,
100-
true,
101-
// If "by_reference" is disabled (explicit calling of
102-
// the setter is desired), disable support for
103-
// adders/removers
104-
// Same as in CollectionType
105-
$options['by_reference'],
106-
$options['add_method'],
107-
$options['remove_method']
108-
))
109-
;
93+
$builder->appendClientTransformer(new ChoicesToValuesTransformer($options['choice_list']));
11094
} else {
11195
$builder->appendClientTransformer(new ChoiceToValueTransformer($options['choice_list']));
11296
}
11397
}
98+
99+
if ($options['multiple']) {
100+
// Make sure the collection created during the client->norm
101+
// transformation is merged back into the original collection
102+
$mergeStrategy = MergeCollectionListener::MERGE_NORMAL;
103+
104+
// Enable support for adders/removers unless "by_reference" is disabled
105+
// (explicit calling of the setter is desired)
106+
if ($options['by_reference']) {
107+
$mergeStrategy = $mergeStrategy | MergeCollectionListener::MERGE_INTO_PARENT;
108+
}
109+
110+
$builder->addEventSubscriber(new MergeCollectionListener(
111+
true,
112+
true,
113+
$mergeStrategy,
114+
$options['add_method'],
115+
$options['remove_method']
116+
));
117+
}
114118
}
115119

116120
/**

0 commit comments

Comments
 (0)
0