8000 merged branch bschussek/issue3732 (PR #3819) · symfony/symfony@c7b2264 · GitHub
[go: up one dir, main page]
Skip to content

Commit c7b2264

Browse files
fabpotfabpot
committed
merged branch bschussek/issue3732 (PR #3819)
Commits ------- c4e68a3 [Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class Discussion ---------- [Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #3732 Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3732) The addXxx()/removeXxx() methods should now be called correctly in ChoiceType and CollectionType. PropertyPath now favors addXxx()/removeXxx() over setXxx() for collections. For example: ``` $propertyPath = new PropertyPath('article.tags'); // Tries to use addTag()/removeTag() and only uses setTags() (et al.) // if not found $propertyPath->setValue($article, $tags); ``` For other languages than English or very irregular plurals, a custom singular can be set by separating it with a pipe: ``` $propertyPath = new PropertyPath('article.genera|genus'); ``` --------------------------------------------------------------------------- by bschussek at 2012-04-07T12:40:39Z Again, the failing build is not my fault.
2 parents 517f8e0 + c4e68a3 commit c7b2264

17 files changed

+702
-1016
lines changed

‎CHANGELOG-2.1.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
252252
* the radio type is now a child of the checkbox type
253253
* the collection, choice (with multiple selection) and entity (with multiple
254254
selection) types now make use of addXxx() and removeXxx() methods in your
255-
model
256-
* added options "add_method" and "remove_method" to collection and choice type
255+
model. For a custom, non-recognized singular form, set the "property_path"
256+
option like this: "plural|singular"
257257
* forms now don't create an empty object anymore if they are completely
258258
empty and not required. The empty value for such forms is null.
259259
* added constant Guess::VERY_HIGH_CONFIDENCE

‎src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,16 @@ public function mapDataToForms($data, array $forms)
5959
public function mapDataToForm($data, FormInterface $form)
6060
{
6161
if (!empty($data)) {
62-
if (null !== $form->getAttribute('property_path')) {
63-
$form->setData($form->getAttribute('property_path')->getValue($data));
62+
$propertyPath = $form->getAttribute('property_path');
63+
64+
if (null !== $propertyPath) {
65+
$propertyData = $propertyPath->getValue($data);
66+
67+
if (is_object($propertyData) && !$form->getAttribute('by_reference')) {
68+
$propertyData = clone $propertyData;
69+
}
70+
71+
$form->setData($propertyData);
6472
}
6573
}
6674
}
@@ -77,9 +85,9 @@ public function mapFormsToData(array $forms, &$data)
7785

7886
public function mapFormToData(FormInterface $form, &$data)
7987
{
80-
if (null !== $form->getAttribute('property_path') && $form->isSynchronized()) {
81-
$propertyPath = $form->getAttribute('property_path');
88+
$propertyPath = $form->getAttribute('property_path');
8289

90+
if (null !== $propertyPath && $form->isSynchronized()) {
8391
// If the data is identical to the value in $data, we are
8492
// dealing with a reference
8593
$isReference = $form->getData() === $propertyPath->getValue($data);

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

Lines changed: 36 additions & 246 deletions
< 10000 td data-grid-cell-id="diff-6f97220fa2fb378db051fc54ba8c4eb4ba71b65c39b58ab7904600194408ef5b-116-54-2" data-line-anchor="diff-6f97220fa2fb378db051fc54ba8c4eb4ba71b65c39b58ab7904600194408ef5bR54" data-selected="false" role="gridcell" style="background-color:var(--bgColor-default);padding-right:24px" tabindex="-1" valign="top" class="focusable-grid-cell diff-text-cell right-side-diff-cell left-side">
static public function getSubscribedEvents()
< 10000 td data-grid-cell-id="diff-6f97220fa2fb378db051fc54ba8c4eb4ba71b65c39b58ab7904600194408ef5b-278-106-2" data-line-anchor="diff-6f97220fa2fb378db051fc54ba8c4eb4ba71b65c39b58ab7904600194408ef5bL278" data-selected="false" role="gridcell" style="background-color:var(--diffBlob-deletionLine-bgColor, var(--diffBlob-deletion-bgColor-line));padding-right:24px" tabindex="-1" valign="top" class="focusable-grid-cell diff-text-cell left-side-diff-cell border-right left-side">-
if ($item === $originalItem) {
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,6 @@
2525
*/
2626
class MergeCollectionListener implements EventSubscriberInterface
2727
{
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-
4328
/**
4429
* Whether elements may be added to the collection
4530
* @var Boolean
@@ -52,131 +37,33 @@ class MergeCollectionListener implements EventSubscriberInterface
5237
*/
5338
private $allowDelete;
5439

55-
/**
56-
* Whether to search for and use adder and remover methods
57-
* @var Boolean
58-
*/
59-
private $mergeStrategy;
60-
61-
/**
62-
* The name of the adder method to look for
63-
* @var string
64-
*/
65-
private $addMethod;
66-
67-
/**
68-
* The name of the remover method to look for
69-
* @var string
70-
*/
71-
private $removeMethod;
72-
73-
/**
74-
* A copy of the data before starting binding for this form
75-
* @var mixed
76-
*/
77-
private $dataSnapshot;
78-
7940
/**
8041
* Creates a new listener.
8142
*
8243
* @param Boolean $allowAdd Whether values might be added to the
8344
* collection.
8445
* @param Boolean $allowDelete Whether values might be removed from the
8546
* 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.
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.
10247
*/
103-
public function __construct($allowAdd = false, $allowDelete = false, $mergeStrategy = null, $addMethod = null, $removeMethod = null)
48+
public function __construct($allowAdd = false, $allowDelete = false)
10449
{
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-
10950
$this->allowAdd = $allowAdd;
11051
$this->allowDelete = $allowDelete;
111-
$this->mergeStrategy = $mergeStrategy ?: self::MERGE_NORMAL | self::MERGE_INTO_PARENT;
112-
$this->addMethod = $addMethod;
113-
$this->removeMethod = $removeMethod;
11452
}
11553

11654
11755
{
11856
return array(
119-
FormEvents::PRE_BIND => 'preBind',
12057
FormEvents::BIND_NORM_DATA => 'onBindNormData',
12158
);
12259
}
12360

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-
}
138-
}
139-
14061
public function onBindNormData(FilterDataEvent $event)
14162
{
142-
$originalData = $event->getForm()->getNormData();
143-
144-
// If we are not allowed to change anything, return immediately
145-
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
148-
$event->setData($originalData);
149-
150-
return;
151-
}
63+
$dataToMergeInto = $event->getForm()->getNormData();
15264

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

18168
if (null === $data) {
18269
$data = array();
@@ -186,157 +73,60 @@ public function onBindNormData(FilterDataEvent $event)
18673
throw new UnexpectedTypeException($data, 'array or (\Traversable and \ArrayAccess)');
18774
}
18875

189-
if (null !== $originalData && !is_array($originalData) && !($originalData instanceof \Traversable && $originalData instanceof \ArrayAccess)) {
190-
throw new UnexpectedTypeException($originalData, 'array or (\Traversable and \ArrayAccess)');
76+
if (null !== $dataToMergeInto && !is_array($dataToMergeInto) && !($dataToMergeInto instanceof \Traversable && $dataToMergeInto instanceof \ArrayAccess)) {
77+
throw new UnexpectedTypeException($dataToMergeInto, 'array or (\Traversable and \ArrayAccess)');
19178
}
19279

193-
// Check if the parent has matching methods to add/remove items
194-
if (($this->mergeStrategy & self::MERGE_INTO_PARENT) && is_object($parentData)) {
195-
$reflClass = new \ReflectionClass($parentData);
196-
$addMethodNeeded = $this->allowAdd && !$this->addMethod;
197-
$removeMethodNeeded = $this->allowDelete && !$this->removeMethod;
198-
199-
// Any of the two methods is required, but not yet known
200-
if ($addMethodNeeded || $removeMethodNeeded) {
201-
$singulars = (array) FormUtil::singularify($plural);
202-
203-
foreach ($singulars as $singular) {
204-
// Try to find adder, but don't override preconfigured one
205-
if ($addMethodNeeded) {
206-
$addMethod = 'add' . $singular;
207-
208-
// False alert
209-
if (!$this->isAccessible($reflClass, $addMethod, 1)) {
210-
$addMethod = null;
211-
}
212-
}
213-
214-
// Try to find remover, but don't override preconfigured one
215-
if ($removeMethodNeeded) {
216-
$removeMethod = 'remove' . $singular;
217-
218-
// False alert
219-
if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
220-
$removeMethod = null;
221-
}
222-
}
223-
224-
// Found all that we need. Abort search.
225-
if ((!$addMethodNeeded || $addMethod) && (!$removeMethodNeeded || $removeMethod)) {
226-
break;
227-
}
228-
229-
// False alert
230-
$addMethod = null;
231-
$removeMethod = null;
232-
}
233-
}
234-
235-
// Set preconfigured adder
236-
if ($this->allowAdd && $this->addMethod) {
237-
$addMethod = $this->addMethod;
238-
239-
if (!$this->isAccessible($reflClass, $addMethod, 1)) {
240-
throw new FormException(sprintf(
241-
'The public method "%s" could not be found on class %s',
242-
$addMethod,
243-
$reflClass->getName()
244-
));
245-
}
246-
}
247-
248-
// Set preconfigured remover
249-
if ($this->allowDelete && $this->removeMethod) {
250-
$removeMethod = $this->removeMethod;
80+
// If we are not allowed to change anything, return immediately
81+
if ((!$this->allowAdd && !$this->allowDelete) || $data === $dataToMergeInto) {
82+
$event->setData($dataToMergeInto);
25183

252-
if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
253-
throw new FormException(sprintf(
254-
'The public method "%s" could not be found on class %s',
255-
$removeMethod,
256-
$reflClass->getName()
257-
));
258-
}
259-
}
84+
return;
26085
}
26186

262-
// Calculate delta between $data and the snapshot created in PRE_BIND
263-
$itemsToDelete = array();
264-
$itemsToAdd = is_object($data) ? clone $data : $data;
265-
266-
if ($this->dataSnapshot) {
267-
foreach ($this->dataSnapshot as $originalItem) {
268-
foreach ($data as $key => $item) {
269-
if ($item === $originalItem) {
87+
if (!$dataToMergeInto) {
88+
// No original data was set. Set it if allowed
89+
if ($this->allowAdd) {
90+
$dataToMergeInto = $data;
91+
}
92+
} else {
93+
// Calculate delta
94+
$itemsToAdd = is_object($data) ? clone $data : $data;
95+
$itemsToDelete = array();
96+
97+
foreach ($dataToMergeInto as $beforeKey => $beforeItem) {
98+
foreach ($data as $afterKey => $afterItem) {
99+
if ($afterItem === $beforeItem) {
270100
// Item found, next original item
271-
unset($itemsToAdd[$key]);
101+
unset($itemsToAdd[$afterKey]);
272102
continue 2;
273103
}
274104
}
275105

276106
// Item not found, remember for deletion
277-
foreach ($originalData as $key => $item) {
278
279-
$itemsToDelete[$key] = $item;
280-
continue 2;
281-
}
282-
}
283-
}
284-
}
285-
286-
if ($addMethod || $removeMethod) {
287-
// If methods to add and to remove exist, call them now, if allowed
288-
if ($removeMethod) {
289-
foreach ($itemsToDelete as $item) {
290-
$parentData->$removeMethod($item);
291-
}
107+
$itemsToDelete[] = $beforeKey;
292108
}
293109

294-
if ($addMethod) {
295-
foreach ($itemsToAdd as $item) {
296-
$parentData->$addMethod($item);
110+
// Remove deleted items before adding to free keys that are to be
111+
// replaced
112+
if ($this->allowDelete) {
113+
foreach ($itemsToDelete as $key) {
114+
unset($dataToMergeInto[$key]);
297115
}
298116
}
299117

300-
$event->setData($childPropertyPath->getValue($parentData));
301-
} elseif ($this->mergeStrategy & self::MERGE_NORMAL) {
302-
if (!$originalData) {
303-
// No original data was set. Set it if allowed
304-
if ($this->allowAdd) {
305-
$originalData = $data;
306-
}
307-
} else {
308-
// Original data is an array-like structure
309-
// Add and remove items in the original variable
310-
if ($this->allowDelete) {
311-
foreach ($itemsToDelete as $key => $item) {
312-
unset($originalData[$key]);
118+
// Add remaining items
119+
if ($this->allowAdd) {
120+
foreach ($itemsToAdd as $key => $item) {
121+
if (!isset($dataToMergeInto[$key])) {
122+
$dataToMergeInto[$key] = $item;
123+
} else {
124+
$dataToMergeInto[] = $item;
313125
}
314126
}
315-
316-
if ($this->allowAdd) {
317-
foreach ($itemsToAdd as $key => $item) {
318-
if (!isset($originalData[$key])) {
319-
$originalData[$key] = $item;
320-
} else {
321-
$originalData[] = $item;
322-
}
323-
}
324-
}
325-
}
326-
327-
$event->setData($originalData);
328-
}
329-
}
330-
331-
private function isAccessible(\ReflectionClass $reflClass, $methodName, $numberOfRequiredParameters) {
332-
if ($reflClass->hasMethod($methodName)) {
333-
$method = $reflClass->getMethod($methodName);
334-
335-
if ($method->isPublic() && $method->getNumberOfRequiredParameters() === $numberOfRequiredParameters) {
336-
return true;
337127
}
338128
}
339129

340-
return false;
130+
$event->setData($dataToMergeInto);
341131
}
342132
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,8 @@ public function onBindNormData(FilterDataEvent $event)
144144
throw new UnexpectedTypeException($data, 'array or (\Traversable and \ArrayAccess)');
145145
}
146146

147+
// The data mapper only adds, but does not remove items, so do this
148+
// here
147149
if ($this->allowDelete) {
148150
foreach ($data as $name => $child) {
149151
if (!$form->has($name)) {

0 commit comments

Comments
 (0)
0