Skip to content

Commit

Permalink
[PropertyAccess] stop overwriting once a reference is reached (3rd)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bananer @Tobion
bananer authored and Tobion committed Mar 3, 2015
1 parent 38a5ebd commit d733a88
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 21 deletions.
41 changes: 20 additions & 21 deletions src/Symfony/Component/PropertyAccess/PropertyAccessor.php
View file
Expand Up @@ -70,7 +70,6 @@ public function setValue(&$objectOrArray, $propertyPath, $value)
}

$propertyValues = & $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength() - 1);
$overwrite = true;

// Add the root object to the list
array_unshift($propertyValues, array(
Expand All @@ -81,18 +80,19 @@ public function setValue(&$objectOrArray, $propertyPath, $value)
for ($i = count($propertyValues) - 1; $i >= 0; --$i) {
$objectOrArray = & $propertyValues[$i][self::VALUE];

if ($overwrite) {
$property = $propertyPath->getElement($i);
$property = $propertyPath->getElement($i);

if ($propertyPath->isIndex($i)) {
$this->writeIndex($objectOrArray, $property, $value);
} else {
$this->writeProperty($objectOrArray, $property, $value);
}
if ($propertyPath->isIndex($i)) {
$this->writeIndex($objectOrArray, $property, $value);
} else {
$this->writeProperty($objectOrArray, $property, $value);
}

if ($propertyValues[$i][self::IS_REF]) {
return;

This comment has been minimized.

Sign in to view

Copy link
@lolautruche

lolautruche May 7, 2015

Contributor

There is a regression here 😞

With this change, it's not possible to set values into deep hashes any more.
For example, consider a property path like someProperty[foo][bar] with a class like:

class Foo
{
    /**
     * Example: $this->someProperty['foo']['bar'] = 'baz'
     * @var array
     */
    public $someProperty;
}

With this change, it will not be possible to set a value for bar any more.

$obj = new Foo();
$obj->someProperty = ['foo' => ['bar' => 'some_value']];

$propertyAccessor->setValue($obj, 'someProperty[foo][bar]', 'another_value');

echo $obj->someProperty['foo']['bar'];
// Before this patch: 'another_value'
// After this patch: 'some_value' => fail

This comment has been minimized.

Sign in to view

Copy link
@Tobion

Tobion May 7, 2015

Member

please open a new ticket, preferably as a PR with a failing test

This comment has been minimized.

Sign in to view

Copy link
@lolautruche

lolautruche May 8, 2015

Contributor

Sure, I'll do that.

This comment has been minimized.

Sign in to view

Copy link
@lolautruche

lolautruche May 11, 2015

Contributor

@Tobion: #14603
}

$value = & $objectOrArray;
$overwrite = !$propertyValues[$i][self::IS_REF];
}
}

Expand Down Expand Up @@ -127,7 +127,6 @@ public function isWritable($objectOrArray, $propertyPath)

try {
$propertyValues = $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength() - 1);
$overwrite = true;

// Add the root object to the list
array_unshift($propertyValues, array(
Expand All @@ -138,21 +137,21 @@ public function isWritable($objectOrArray, $propertyPath)
for ($i = count($propertyValues) - 1; $i >= 0; --$i) {
$objectOrArray = $propertyValues[$i][self::VALUE];

if ($overwrite) {
$property = $propertyPath->getElement($i);
$property = $propertyPath->getElement($i);

if ($propertyPath->isIndex($i)) {
if (!$objectOrArray instanceof \ArrayAccess && !is_array($objectOrArray)) {
return false;
}
} else {
if (!$this->isPropertyWritable($objectOrArray, $property)) {
return false;
}
if ($propertyPath->isIndex($i)) {
if (!$objectOrArray instanceof \ArrayAccess && !is_array($objectOrArray)) {
return false;
}
} else {
if (!$this->isPropertyWritable($objectOrArray, $property)) {
return false;
}
}

$overwrite = !$propertyValues[$i][self::IS_REF];
if ($propertyValues[$i][self::IS_REF]) {
return true;
}
}

return true;
Expand Down
7 changes: 7 additions & 0 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestClass.php
View file
Expand Up @@ -25,6 +25,7 @@ class TestClass
private $publicAccessorWithMoreRequiredParameters;
private $publicIsAccessor;
private $publicHasAccessor;
private $publicGetter;

public function __construct($value)
{
Expand All @@ -37,6 +38,7 @@ public function __construct($value)
$this->publicAccessorWithMoreRequiredParameters = $value;
$this->publicIsAccessor = $value;
$this->publicHasAccessor = $value;
$this->publicGetter = $value;
}

public function setPublicAccessor($value)
Expand Down Expand Up @@ -166,4 +168,9 @@ private function hasPrivateHasAccessor()
{
return 'foobar';
}

public function getPublicGetter()
{
return $this->publicGetter;
}
}
8 changes: 8 additions & 0 deletions src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
View file
Expand Up @@ -419,6 +419,14 @@ public function getValidPropertyPaths()
array(array('index' => array('%!@$§.' => 'Bernhard')), '[index][%!@$§.]', 'Bernhard'),
array((object) array('%!@$§' => 'Bernhard'), '%!@$§', 'Bernhard'),
array((object) array('property' => (object) array('%!@$§' => 'Bernhard')), 'property.%!@$§', 'Bernhard'),

// nested objects and arrays
array(array('foo' => new TestClass('bar')), '[foo].publicGetSetter', 'bar'),
array(new TestClass(array('foo' => 'bar')), 'publicGetSetter[foo]', 'bar'),
array(new TestClass(new TestClass('bar')), 'publicGetter.publicGetSetter', 'bar'),
array(new TestClass(array('foo' => new TestClass('bar'))), 'publicGetter[foo].publicGetSetter', 'bar'),
array(new TestClass(new TestClass(new TestClass('bar'))), 'publicGetter.publicGetter.publicGetSetter', 'bar'),
array(new TestClass(array('foo' => array('baz' => new TestClass('bar')))), 'publicGetter[foo][baz].publicGetSetter', 'bar'),
);
}

Expand Down

0 comments on commit d733a88

Please sign in to comment.