merged branch bschussek/issue4670 (PR #5406)
Commits
-------
04fd5f1
[Form] Fixed PropertyPath to not modify Collection instances (not even their clones)
Discussion
----------
[Form] Fixed PropertyPath to not modify Collection instances
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4670
Todo: -
---------------------------------------------------------------------------
by pocallaghan at 2012-08-31T14:20:52Z
As far as I can see the pull request does fix the issue, which makes sense based on the code change (I didn't know iterator_to_array existed, good call). One thing I would say, I'm not sure on the use in the change to the test case. It's not clear to me what additional protection this extra assertion gives, as both the old and new code seem to pass.
---------------------------------------------------------------------------
by bschussek at 2012-08-31T14:21:46Z
The new assertion is there because not even the old code (`clone`) was tested.
---------------------------------------------------------------------------
by stof at 2012-08-31T14:37:38Z
@bschussek but was it failing without the code change ?
---------------------------------------------------------------------------
by bschussek at 2012-08-31T22:12:00Z
@stof It was not, but I was unable to write a good test for the change within reasonable time. I added an explanatory comment instead.
This commit is contained in:
commit
797ba6846a
|
@ -168,6 +168,7 @@ abstract class PropertyPathCollectionTest extends \PHPUnit_Framework_TestCase
|
||||||
$axesBefore = $this->getCollection(array(1 => 'second', 3 => 'fourth', 4 => 'fifth'));
|
$axesBefore = $this->getCollection(array(1 => 'second', 3 => 'fourth', 4 => 'fifth'));
|
||||||
$axesMerged = $this->getCollection(array(1 => 'first', 2 => 'second', 3 => 'third'));
|
$axesMerged = $this->getCollection(array(1 => 'first', 2 => 'second', 3 => 'third'));
|
||||||
$axesAfter = $this->getCollection(array(1 => 'second', 5 => 'first', 6 => 'third'));
|
$axesAfter = $this->getCollection(array(1 => 'second', 5 => 'first', 6 => 'third'));
|
||||||
|
$axesMergedCopy = is_object($axesMerged) ? clone $axesMerged : $axesMerged;
|
||||||
|
|
||||||
// Don't use a mock in order to test whether the collections are
|
// Don't use a mock in order to test whether the collections are
|
||||||
// modified while iterating them
|
// modified while iterating them
|
||||||
|
@ -178,6 +179,9 @@ abstract class PropertyPathCollectionTest extends \PHPUnit_Framework_TestCase
|
||||||
$path->setValue($car, $axesMerged);
|
$path->setValue($car, $axesMerged);
|
||||||
|
|
||||||
$this->assertEquals($axesAfter, $car->getAxes());
|
$this->assertEquals($axesAfter, $car->getAxes());
|
||||||
|
|
||||||
|
// The passed collection was not modified
|
||||||
|
$this->assertEquals($axesMergedCopy, $axesMerged);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testSetValueCallsAdderAndRemoverForNestedCollections()
|
public function testSetValueCallsAdderAndRemoverForNestedCollections()
|
||||||
|
|
|
@ -485,7 +485,9 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
|
||||||
$methods = $this->findAdderAndRemover($reflClass, $singulars);
|
$methods = $this->findAdderAndRemover($reflClass, $singulars);
|
||||||
if (null !== $methods) {
|
if (null !== $methods) {
|
||||||
// At this point the add and remove methods have been found
|
// At this point the add and remove methods have been found
|
||||||
$itemsToAdd = is_object($value) ? clone $value : $value;
|
// Use iterator_to_array() instead of clone in order to prevent side effects
|
||||||
|
// see https://github.com/symfony/symfony/issues/4670
|
||||||
|
$itemsToAdd = is_object($value) ? iterator_to_array($value) : $value;
|
||||||
$itemToRemove = array();
|
$itemToRemove = array();
|
||||||
$propertyValue = $this->readProperty($objectOrArray, $property, $isIndex);
|
$propertyValue = $this->readProperty($objectOrArray, $property, $isIndex);
|
||||||
$previousValue = $propertyValue[self::VALUE];
|
$previousValue = $propertyValue[self::VALUE];
|
||||||
|
|
Reference in New Issue