Skip to content

Commit

Permalink
[Form] Fixed data to be written back by PropertyPath if it cannot be …
Browse files Browse the repository at this point in the history 
…handled by reference
  • Loading branch information
@webmozart
webmozart committed Jul 10, 2012
1 parent 9e32d90 commit e8bb834
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 34 deletions.
21 changes: 21 additions & 0 deletions src/Symfony/Component/Form/Tests/Util/PropertyPathCollectionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ public function getAxes() {}
class PropertyPathCollectionTest_CompositeCar
{
public function getStructure() {}

public function setStructure($structure) {}
}

class PropertyPathCollectionTest_CarStructure
Expand All @@ -105,6 +107,15 @@ public function testGetValueReadsArrayAccess()
$this->assertEquals('Bernhard', $path->getValue($object));
}

public function testGetValueReadsNestedArrayAccess()
{
$object = $this->getCollection(array('person' => array('firstName' => 'Bernhard')));

$path = new PropertyPath('[person][firstName]');

$this->assertEquals('Bernhard', $path->getValue($object));
}

/**
* @expectedException Symfony\Component\Form\Exception\InvalidPropertyException
*/
Expand All @@ -125,6 +136,16 @@ public function testSetValueUpdatesArrayAccess()
$this->assertEquals('Bernhard', $object['firstName']);
}

public function testSetValueUpdatesNestedArrayAccess()
{
$object = $this->getCollection(array());

$path = new PropertyPath('[person][firstName]');
$path->setValue($object, 'Bernhard');

$this->assertEquals('Bernhard', $object['person']['firstName']);
}

/**
* @expectedException Symfony\Component\Form\Exception\InvalidPropertyException
*/
Expand Down
107 changes: 73 additions & 34 deletions src/Symfony/Component/Form/Util/PropertyPath.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
*/
const SINGULAR_SEPARATOR = '|';

const VALUE = 0;
const IS_REF = 1;

/**
* The elements of the property path
* @var array
Expand Down Expand Up @@ -272,7 +275,9 @@ public function isIndex($index)
*/
public function getValue($objectOrArray)
{
return $this->readPropertyAt($objectOrArray, $this->length - 1);
$propertyValues =& $this->readPropertiesUntil($objectOrArray, $this->length - 1);

return $propertyValues[count($propertyValues) - 1][self::VALUE];
}

/**
Expand Down Expand Up @@ -306,48 +311,70 @@ public function getValue($objectOrArray)
*/
public function setValue(&$objectOrArray, $value)
{
$objectOrArray =& $this->readPropertyAt($objectOrArray, $this->length - 2);
$propertyValues =& $this->readPropertiesUntil($objectOrArray, $this->length - 2);
$overwrite = true;

if (!is_object($objectOrArray) && !is_array($objectOrArray)) {
throw new UnexpectedTypeException($objectOrArray, 'object or array');
}
// Add the root object to the list
array_unshift($propertyValues, array(
self::VALUE => &$objectOrArray,
self::IS_REF => true,
));

$property = $this->elements[$this->length - 1];
$singular = $this->singulars[$this->length - 1];
$isIndex = $this->isIndex[$this->length - 1];
for ($i = count($propertyValues) - 1; $i >= 0; --$i) {
$objectOrArray =& $propertyValues[$i][self::VALUE];

if ($overwrite) {
if (!is_object($objectOrArray) && !is_array($objectOrArray)) {
throw new UnexpectedTypeException($objectOrArray, 'object or array');
}

$this->writeProperty($objectOrArray, $property, $singular, $isIndex, $value);
$property = $this->elements[$i];
$singular = $this->singulars[$i];
$isIndex = $this->isIndex[$i];

$this->writeProperty($objectOrArray, $property, $singular, $isIndex, $value);
}

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

/**
* Reads the path from an object up to a given path index.
*
* @param object|array $objectOrArray The object or array to read from.
* @param integer $index The integer up to which should be read.
* @param integer $lastIndex The integer up to which should be read.
*
* @return mixed The value read at the end of the path.
* @return array The values read in the path.
*
* @throws UnexpectedTypeException If a value within the path is neither object nor array.
*/
protected function &readPropertyAt(&$objectOrArray, $index)
private function &readPropertiesUntil(&$objectOrArray, $lastIndex)
{
for ($i = 0; $i <= $index; ++$i) {
$propertyValues = array();

for ($i = 0; $i <= $lastIndex; ++$i) {
if (!is_object($objectOrArray) && !is_array($objectOrArray)) {
throw new UnexpectedTypeException($objectOrArray, 'object or array');
}

$property = $this->elements[$i];
$isIndex = $this->isIndex[$i];
$isArrayAccess = is_array($objectOrArray) || $objectOrArray instanceof \ArrayAccess;

// Create missing nested arrays on demand
if (is_array($objectOrArray) && !array_key_exists($this->elements[$i], $objectOrArray)) {
$objectOrArray[$this->elements[$i]] = $i + 1 < $this->length ? array() : null;
if ($isIndex && $isArrayAccess && !isset($objectOrArray[$property])) {
$objectOrArray[$property] = $i + 1 < $this->length ? array() : null;
}

$property = $this->elements[$i];
$isIndex = $this->isIndex[$i];
$propertyValue =& $this->readProperty($objectOrArray, $property, $isIndex);
$objectOrArray =& $propertyValue[self::VALUE];

$objectOrArray =& $this->readProperty($objectOrArray, $property, $isIndex);
$propertyValues[] =& $propertyValue;
}

return $objectOrArray;
return $propertyValues;
}

/**
Expand All @@ -363,11 +390,14 @@ protected function &readPropertyAt(&$objectOrArray, $index)
* @throws PropertyAccessDeniedException If the property cannot be accessed due to
* access restrictions (private or protected).
*/
protected function &readProperty(&$objectOrArray, $property, $isIndex)
private function &readProperty(&$objectOrArray, $property, $isIndex)
{
// The local variable (instead of immediate returns) is necessary
// because we want to return by reference!
$result = null;
// Use an array instead of an object since performance is
// very crucial here
$result = array(

This comment has been minimized.

Sign in to view

Copy link
@Tobion

Tobion Jul 11, 2012

Member

I don't understand why this is necessary. Why can't we simply return the value?

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Jul 11, 2012

Member

because you cannot always return by reference. If you have a getter for an array property in an object, it is often not returned by reference. And when implementing ArrayAccess, it is not always returned by reference either.

This comment has been minimized.

Sign in to view

Copy link
@webmozart

webmozart Jul 11, 2012

Author Contributor

Imagine the path address.street which calls getAddress() and then setStreet() on the returned object. If we don't return a reference to the Address object here, setAddress() needs to exist too, so that it can be called. We don't want that.

(of course objects are always passed by reference, but the above is also true for arrays)
self::VALUE => null,
self::IS_REF => false
);

if ($isIndex) {
if (!$objectOrArray instanceof \ArrayAccess && !is_array($objectOrArray)) {
Expand All @@ -376,9 +406,10 @@ protected function &readProperty(&$objectOrArray, $property, $isIndex)

if (isset($objectOrArray[$property])) {
if (is_array($objectOrArray)) {
$result =& $objectOrArray[$property];
$result[self::VALUE] =& $objectOrArray[$property];
$result[self::IS_REF] = true;
} else {
$result = $objectOrArray[$property];
$result[self::VALUE] = $objectOrArray[$property];
}
}
} elseif (is_object($objectOrArray)) {
Expand All @@ -393,38 +424,45 @@ protected function &readProperty(&$objectOrArray, $property, $isIndex)
throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $getter, $reflClass->name));
}

$result = $objectOrArray->$getter();
$result[self::VALUE] = $objectOrArray->$getter();
} elseif ($reflClass->hasMethod($isser)) {
if (!$reflClass->getMethod($isser)->isPublic()) {
throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $isser, $reflClass->name));
}

$result = $objectOrArray->$isser();
$result[self::VALUE] = $objectOrArray->$isser();
} elseif ($reflClass->hasMethod($hasser)) {
if (!$reflClass->getMethod($hasser)->isPublic()) {
throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $hasser, $reflClass->name));
}

$result = $objectOrArray->$hasser();
$result[self::VALUE] = $objectOrArray->$hasser();
} elseif ($reflClass->hasMethod('__get')) {
// needed to support magic method __get
$result = $objectOrArray->$property;
$result[self::VALUE] = $objectOrArray->$property;
} elseif ($reflClass->hasProperty($property)) {
if (!$reflClass->getProperty($property)->isPublic()) {
throw new PropertyAccessDeniedException(sprintf('Property "%s" is not public in class "%s". Maybe you should create the method "%s()" or "%s()"?', $property, $reflClass->name, $getter, $isser));
}

$result =& $objectOrArray->$property;
$result[self::VALUE] =& $objectOrArray->$property;
$result[self::IS_REF] = true;
} elseif (property_exists($objectOrArray, $property)) {
// needed to support \stdClass instances
$result =& $objectOrArray->$property;
$result[self::VALUE] =& $objectOrArray->$property;
$result[self::IS_REF] = true;
} else {
throw new InvalidPropertyException(sprintf('Neither property "%s" nor method "%s()" nor method "%s()" exists in class "%s"', $property, $getter, $isser, $reflClass->name));
}
} else {
throw new InvalidPropertyException(sprintf('Cannot read property "%s" from an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
}

// Objects are always passed around by reference
if (is_object($result[self::VALUE])) {
$result[self::IS_REF] = true;
}

return $result;
}

Expand All @@ -441,7 +479,7 @@ protected function &readProperty(&$objectOrArray, $property, $isIndex)
* @throws PropertyAccessDeniedException If the property cannot be accessed due to
* access restrictions (private or protected).
*/
protected function writeProperty(&$objectOrArray, $property, $singular, $isIndex, $value)
private function writeProperty(&$objectOrArray, $property, $singular, $isIndex, $value)
{
$adderRemoverError = null;

Expand All @@ -466,7 +504,8 @@ protected function writeProperty(&$objectOrArray, $property, $singular, $isIndex
// At this point the add and remove methods have been found
$itemsToAdd = is_object($value) ? clone $value : $value;
$itemToRemove = array();
$previousValue = $this->readProperty($objectOrArray, $property, $isIndex);
$propertyValue = $this->readProperty($objectOrArray, $property, $isIndex);

This comment has been minimized.

Sign in to view

Copy link
@Tobion

Tobion Jul 11, 2012

Member

Shouldn't this be =& as readProperty returns by reference?

This comment has been minimized.

Sign in to view

Copy link
@webmozart

webmozart Jul 11, 2012

Author Contributor

Using =& here leads to a parser error. readProperty() returns a reference, so the reference is stored. There is no need to indicate that again.

This comment has been minimized.

Sign in to view

Copy link
@Tobion

Tobion Jul 11, 2012

Member

But in other places like line 371 you used =&. And AFAIK both function declarions and assignment operations should indicate by reference.

This comment has been minimized.

Sign in to view

Copy link
@webmozart

webmozart Jul 12, 2012

Author Contributor

To make a long story short, if you assign a variable, you can assign it by reference. If you assign the result of a function etc., you cannot (what would you refer to?). That's the way PHP behaves, so there's nothing that can be changed here.

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Jul 12, 2012

Contributor

I don't know what you want to achieve, but if you want to take advantage of the return by reference, then you must assign by reference even if you already have declared the function as returning a ref:

<?php

function &returnsRef(&$a) { return $a; }

$a = 0;
$b = returnsRef($a);
$b = 1;
var_dump($a); // integer(0)

$a = 0;
$b =& returnsRef($a);
$b = 1;
var_dump($a); // integer(1)

This comment has been minimized.

Sign in to view

Copy link
@webmozart

webmozart Jul 12, 2012

Author Contributor

Yes, this is clear. My first statement was wrong that this leads to a parser error here, since readProperty already returns by reference. It leads to an error further down the code where we invoke getters and assign their result (and I was mistakingly speaking about that part).

The correct answer should have been that =& is not necessary here since we neither modify $propertyValue nor $previousValue.
$previousValue = $propertyValue[self::VALUE];

if (is_array($previousValue) || $previousValue instanceof Traversable) {
foreach ($previousValue as $previousItem) {
Expand Down Expand Up @@ -538,7 +577,7 @@ protected function writeProperty(&$objectOrArray, $property, $singular, $isIndex
*
* @return string The camelized version of the string.
*/
protected function camelize($string)
private function camelize($string)
{
return preg_replace_callback('/(^|_|\.)+(.)/', function ($match) { return ('.' === $match[1] ? '_' : '').strtoupper($match[2]); }, $string);
}
Expand Down

0 comments on commit e8bb834

Please sign in to comment.