From 9aee2ad999c78bddf3b5eae52c8a3fc918cb2c48 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Sun, 30 Mar 2014 18:19:15 +0200 Subject: [PATCH] [PropertyAccess] Removed the argument $value from isWritable() To keep isWritable() and setValue() consistent, the exception thrown when only an adder was present, but no remover (or vice versa), was removed. --- .../Exception/InvalidArgumentException.php | 21 ++ .../PropertyAccess/PropertyAccessor.php | 205 ++++++++++-------- .../PropertyAccessorInterface.php | 17 +- .../Tests/PropertyAccessorCollectionTest.php | 36 +-- 4 files changed, 144 insertions(+), 135 deletions(-) create mode 100644 src/Symfony/Component/PropertyAccess/Exception/InvalidArgumentException.php diff --git a/src/Symfony/Component/PropertyAccess/Exception/InvalidArgumentException.php b/src/Symfony/Component/PropertyAccess/Exception/InvalidArgumentException.php new file mode 100644 index 0000000000..47bc7e150d --- /dev/null +++ b/src/Symfony/Component/PropertyAccess/Exception/InvalidArgumentException.php @@ -0,0 +1,21 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\PropertyAccess\Exception; + +/** + * Base InvalidArgumentException for the PropertyAccess component. + * + * @author Bernhard Schussek + */ +class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface +{ +} diff --git a/src/Symfony/Component/PropertyAccess/PropertyAccessor.php b/src/Symfony/Component/PropertyAccess/PropertyAccessor.php index 8c86d88303..c52b598cb9 100644 --- a/src/Symfony/Component/PropertyAccess/PropertyAccessor.php +++ b/src/Symfony/Component/PropertyAccess/PropertyAccessor.php @@ -11,6 +11,7 @@ namespace Symfony\Component\PropertyAccess; +use Symfony\Component\PropertyAccess\Exception\InvalidArgumentException; use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException; use Symfony\Component\PropertyAccess\Exception\NoSuchIndexException; use Symfony\Component\PropertyAccess\Exception\UnexpectedTypeException; @@ -53,7 +54,12 @@ class PropertyAccessor implements PropertyAccessorInterface if (is_string($propertyPath)) { $propertyPath = new PropertyPath($propertyPath); } elseif (!$propertyPath instanceof PropertyPathInterface) { - throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface'); + throw new InvalidArgumentException(sprintf( + 'The property path should be a string or an instance of '. + '"Symfony\Component\PropertyAccess\PropertyPathInterface". '. + 'Got: "%s"', + is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath) + )); } $propertyValues =& $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength(), $this->ignoreInvalidIndices); @@ -69,7 +75,12 @@ class PropertyAccessor implements PropertyAccessorInterface if (is_string($propertyPath)) { $propertyPath = new PropertyPath($propertyPath); } elseif (!$propertyPath instanceof PropertyPathInterface) { - throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface'); + throw new InvalidArgumentException(sprintf( + 'The property path should be a string or an instance of '. + '"Symfony\Component\PropertyAccess\PropertyPathInterface". '. + 'Got: "%s"', + is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath) + )); } $propertyValues =& $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength() - 1); @@ -90,13 +101,11 @@ class PropertyAccessor implements PropertyAccessorInterface } $property = $propertyPath->getElement($i); - //$singular = $propertyPath->singulars[$i]; - $singular = null; if ($propertyPath->isIndex($i)) { $this->writeIndex($objectOrArray, $property, $value); } else { - $this->writeProperty($objectOrArray, $property, $singular, $value); + $this->writeProperty($objectOrArray, $property, $value); } } @@ -113,7 +122,12 @@ class PropertyAccessor implements PropertyAccessorInterface if (is_string($propertyPath)) { $propertyPath = new PropertyPath($propertyPath); } elseif (!$propertyPath instanceof PropertyPathInterface) { - throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface'); + throw new InvalidArgumentException(sprintf( + 'The property path should be a string or an instance of '. + '"Symfony\Component\PropertyAccess\PropertyPathInterface". '. + 'Got: "%s"', + is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath) + )); } try { @@ -132,12 +146,17 @@ class PropertyAccessor implements PropertyAccessorInterface /** * {@inheritdoc} */ - public function isWritable($objectOrArray, $propertyPath, $value) + public function isWritable($objectOrArray, $propertyPath) { if (is_string($propertyPath)) { $propertyPath = new PropertyPath($propertyPath); } elseif (!$propertyPath instanceof PropertyPathInterface) { - throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface'); + throw new InvalidArgumentException(sprintf( + 'The property path should be a string or an instance of '. + '"Symfony\Component\PropertyAccess\PropertyPathInterface". '. + 'Got: "%s"', + is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath) + )); } try { @@ -165,13 +184,12 @@ class PropertyAccessor implements PropertyAccessorInterface return false; } } else { - if (!$this->isPropertyWritable($objectOrArray, $property, $value)) { + if (!$this->isPropertyWritable($objectOrArray, $property)) { return false; } } } - $value = $objectOrArray; $overwrite = !$propertyValues[$i][self::IS_REF]; } @@ -346,7 +364,7 @@ class PropertyAccessor implements PropertyAccessorInterface } /** - * Sets the value of the property at the given index in the path + * Sets the value of an index in a given array-accessible value. * * @param \ArrayAccess|array $array An array or \ArrayAccess object to write to * @param string|integer $index The index to write at @@ -364,74 +382,33 @@ class PropertyAccessor implements PropertyAccessorInterface } /** - * Sets the value of the property at the given index in the path + * Sets the value of a property in the given object * - * @param object|array $object The object or array to write to - * @param string $property The property to write - * @param string|null $singular The singular form of the property name or null - * @param mixed $value The value to write + * @param object $object The object to write to + * @param string $property The property to write + * @param mixed $value The value to write * * @throws NoSuchPropertyException If the property does not exist or is not * public. */ - private function writeProperty(&$object, $property, $singular, $value) + private function writeProperty(&$object, $property, $value) { - $guessedAdders = ''; - if (!is_object($object)) { throw new NoSuchPropertyException(sprintf('Cannot write property "%s" to an array. Maybe you should write the property path as "[%s]" instead?', $property, $property)); } $reflClass = new \ReflectionClass($object); $plural = $this->camelize($property); - - // Any of the two methods is required, but not yet known - $singulars = null !== $singular ? array($singular) : (array) StringUtil::singularify($plural); + $singulars = (array) StringUtil::singularify($plural); if (is_array($value) || $value instanceof \Traversable) { $methods = $this->findAdderAndRemover($reflClass, $singulars); + // Use addXxx() and removeXxx() to write the collection if (null !== $methods) { - // At this point the add and remove methods have been found - // 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(); - $propertyValue = $this->readProperty($object, $property); - $previousValue = $propertyValue[self::VALUE]; - - if (is_array($previousValue) || $previousValue instanceof \Traversable) { - foreach ($previousValue as $previousItem) { - foreach ($value as $key => $item) { - if ($item === $previousItem) { - // Item found, don't add - unset($itemsToAdd[$key]); - - // Next $previousItem - continue 2; - } - } - - // Item not found, add to remove list - $itemToRemove[] = $previousItem; - } - } - - foreach ($itemToRemove as $item) { - call_user_func(array($object, $methods[1]), $item); - } - - foreach ($itemsToAdd as $item) { - call_user_func(array($object, $methods[0]), $item); - } + $this->writeCollection($object, $property, $value, $methods[0], $methods[1]); return; - } else { - // It is sufficient to include only the adders in the error - // message. If the user implements the adder but not the remover, - // an exception will be thrown in findAdderAndRemover() that - // the remover has to be implemented as well. - $guessedAdders = '"add'.implode('()", "add', $singulars).'()", '; } } @@ -459,43 +436,98 @@ class PropertyAccessor implements PropertyAccessorInterface 'Neither the property "%s" nor one of the methods %s"%s()", '. '"__set()" or "__call()" exist and have public access in class "%s".', $property, - $guessedAdders, + implode('', array_map(function ($singular) { + return '"add'.$singular.'()"/"remove'.$singular.'()", '; + }, $singulars)), $setter, $reflClass->name )); } } - private function isPropertyWritable($object, $property, $value) + /** + * Adjusts a collection-valued property by calling add*() and remove*() + * methods. + * + * @param object $object The object to write to + * @param string $property The property to write + * @param array|\Traversable $collection The collection to write + * @param string $addMethod The add*() method + * @param string $removeMethod The remove*() method + */ + private function writeCollection($object, $property, $collection, $addMethod, $removeMethod) + { + // At this point the add and remove methods have been found + // Use iterator_to_array() instead of clone in order to prevent side effects + // see https://github.com/symfony/symfony/issues/4670 + $itemsToAdd = is_object($collection) ? iterator_to_array($collection) : $collection; + $itemToRemove = array(); + $propertyValue = $this->readProperty($object, $property); + $previousValue = $propertyValue[self::VALUE]; + + if (is_array($previousValue) || $previousValue instanceof \Traversable) { + foreach ($previousValue as $previousItem) { + foreach ($collection as $key => $item) { + if ($item === $previousItem) { + // Item found, don't add + unset($itemsToAdd[$key]); + + // Next $previousItem + continue 2; + } + } + + // Item not found, add to remove list + $itemToRemove[] = $previousItem; + } + } + + foreach ($itemToRemove as $item) { + call_user_func(array($object, $removeMethod), $item); + } + + foreach ($itemsToAdd as $item) { + call_user_func(array($object, $addMethod), $item); + } + } + + /** + * Returns whether a property is writable in the given object. + * + * @param object $object The object to write to + * @param string $property The property to write + * + * @return Boolean Whether the property is writable + */ + private function isPropertyWritable($object, $property) { if (!is_object($object)) { - throw new NoSuchPropertyException(sprintf('Cannot write property "%s" to an array. Maybe you should write the property path as "[%s]" instead?', $property, $property)); + return false; } $reflClass = new \ReflectionClass($object); + + $setter = 'set'.$this->camelize($property); + $classHasProperty = $reflClass->hasProperty($property); + + if ($this->isMethodAccessible($reflClass, $setter, 1) + || $this->isMethodAccessible($reflClass, '__set', 2) + || ($classHasProperty && $reflClass->getProperty($property)->isPublic()) + || (!$classHasProperty && property_exists($object, $property)) + || ($this->magicCall && $this->isMethodAccessible($reflClass, '__call', 2))) { + return true; + } + $plural = $this->camelize($property); // Any of the two methods is required, but not yet known $singulars = (array) StringUtil::singularify($plural); - if (is_array($value) || $value instanceof \Traversable) { - try { - if (null !== $this->findAdderAndRemover($reflClass, $singulars)) { - return true; - } - } catch (NoSuchPropertyException $e) { - return false; - } + if (null !== $this->findAdderAndRemover($reflClass, $singulars)) { + return true; } - $setter = 'set'.$this->camelize($property); - $classHasProperty = $reflClass->hasProperty($property); - - return $this->isMethodAccessible($reflClass, $setter, 1) - || $this->isMethodAccessible($reflClass, '__set', 2) - || ($classHasProperty && $reflClass->getProperty($property)->isPublic()) - || (!$classHasProperty && property_exists($object, $property)) - || ($this->magicCall && $this->isMethodAccessible($reflClass, '__call', 2)); + return false; } /** @@ -517,8 +549,6 @@ class PropertyAccessor implements PropertyAccessorInterface * @param array $singulars The singular form of the property name or null * * @return array|null An array containing the adder and remover when found, null otherwise - * - * @throws NoSuchPropertyException If the property does not exist */ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singulars) { @@ -534,19 +564,6 @@ class PropertyAccessor implements PropertyAccessorInterface if ($addMethodFound && $removeMethodFound) { return array($addMethod, $removeMethod); } - - if ($addMethodFound xor $removeMethodFound && null === $exception) { - $exception = new NoSuchPropertyException(sprintf( - 'Found the public method "%s()", but did not find a public "%s()" on class %s', - $addMethodFound ? $addMethod : $removeMethod, - $addMethodFound ? $removeMethod : $addMethod, - $reflClass->name - )); - } - } - - if (null !== $exception) { - throw $exception; } return null; diff --git a/src/Symfony/Component/PropertyAccess/PropertyAccessorInterface.php b/src/Symfony/Component/PropertyAccess/PropertyAccessorInterface.php index b9da0e4937..7e1cd24ae8 100644 --- a/src/Symfony/Component/PropertyAccess/PropertyAccessorInterface.php +++ b/src/Symfony/Component/PropertyAccess/PropertyAccessorInterface.php @@ -43,9 +43,10 @@ interface PropertyAccessorInterface * @param string|PropertyPathInterface $propertyPath The property path to modify * @param mixed $value The value to set at the end of the property path * - * @throws Exception\NoSuchPropertyException If a property does not exist or is not public. - * @throws Exception\UnexpectedTypeException If a value within the path is neither object - * nor array + * @throws Exception\InvalidArgumentException If the property path is invalid + * @throws Exception\NoSuchPropertyException If a property does not exist or is not public. + * @throws Exception\UnexpectedTypeException If a value within the path is neither object + * nor array */ public function setValue(&$objectOrArray, $propertyPath, $value); @@ -75,7 +76,8 @@ interface PropertyAccessorInterface * * @return mixed The value at the end of the property path * - * @throws Exception\NoSuchPropertyException If a property does not exist or is not public. + * @throws Exception\InvalidArgumentException If the property path is invalid + * @throws Exception\NoSuchPropertyException If a property does not exist or is not public. */ public function getValue($objectOrArray, $propertyPath); @@ -87,11 +89,12 @@ interface PropertyAccessorInterface * * @param object|array $objectOrArray The object or array to check * @param string|PropertyPathInterface $propertyPath The property path to check - * @param mixed $value The value to set at the end of the property path * * @return Boolean Whether the value can be set + * + * @throws Exception\InvalidArgumentException If the property path is invalid */ - public function isWritable($objectOrArray, $propertyPath, $value); + public function isWritable($objectOrArray, $propertyPath); /** * Returns whether a property path can be read from an object graph. @@ -103,6 +106,8 @@ interface PropertyAccessorInterface * @param string|PropertyPathInterface $propertyPath The property path to check * * @return Boolean Whether the property path can be read + * + * @throws Exception\InvalidArgumentException If the property path is invalid */ public function isReadable($objectOrArray, $propertyPath); } diff --git a/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php b/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php index 18211c822b..7e4a49247c 100644 --- a/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php +++ b/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php @@ -172,41 +172,7 @@ abstract class PropertyAccessorCollectionTest extends \PHPUnit_Framework_TestCas /** * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException - * @expectedExceptionMessage Found the public method "addAxis()", but did not find a public "removeAxis()" on class Mock_PropertyAccessorCollectionTest_CarOnlyAdder - */ - public function testSetValueFailsIfOnlyAdderFound() - { - $car = $this->getMock(__CLASS__.'_CarOnlyAdder'); - $axesBefore = $this->getCollection(array(1 => 'second', 3 => 'fourth')); - $axesAfter = $this->getCollection(array(0 => 'first', 1 => 'second', 2 => 'third')); - - $car->expects($this->any()) - ->method('getAxes') - ->will($this->returnValue($axesBefore)); - - $this->propertyAccessor->setValue($car, 'axes', $axesAfter); - } - - /** - * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException - * @expectedExceptionMessage Found the public method "removeAxis()", but did not find a public "addAxis()" on class Mock_PropertyAccessorCollectionTest_CarOnlyRemover - */ - public function testSetValueFailsIfOnlyRemoverFound() - { - $car = $this->getMock(__CLASS__.'_CarOnlyRemover'); - $axesBefore = $this->getCollection(array(1 => 'second', 3 => 'fourth')); - $axesAfter = $this->getCollection(array(0 => 'first', 1 => 'second', 2 => 'third')); - - $car->expects($this->any()) - ->method('getAxes') - ->will($this->returnValue($axesBefore)); - - $this->propertyAccessor->setValue($car, 'axes', $axesAfter); - } - - /** - * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException - * @expectedExceptionMessage Neither the property "axes" nor one of the methods "addAx()", "addAxe()", "addAxis()", "setAxes()", "__set()" or "__call()" exist and have public access in class "Mock_PropertyAccessorCollectionTest_CarNoAdderAndRemover + * @expectedExceptionMessage Neither the property "axes" nor one of the methods "addAx()"/"removeAx()", "addAxe()"/"removeAxe()", "addAxis()"/"removeAxis()", "setAxes()", "__set()" or "__call()" exist and have public access in class "Mock_PropertyAccessorCollectionTest_CarNoAdderAndRemover */ public function testSetValueFailsIfNoAdderNorRemoverFound() {