merged branch SamsonIT/error_explaining_lack_of_adder_remover (PR #4777)
Commits
-------
1fa22d9
[Form] Output a more usable error when PropertyPath has tried to find adders and getters, but failed to find them
Discussion
----------
[Form] Output a more usable error when PropertyPath has tried to find ad...
...ders and getters, but failed to find them
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
I've refactored the writeProperty method of propertypath in order to supply a better error message when writing has failed.
The writeProperty method itself now finds singulars (if a singular was not passed) for the private findAdderAndRemover method which allowed for some duplicate code to be removed and since the writeProperty now holds this data, it can provide a more verbose exception message.
---------------------------------------------------------------------------
by bschussek at 2012-07-09T13:54:35Z
Apart from the typo this PR looks good.
---------------------------------------------------------------------------
by Burgov at 2012-07-09T14:01:04Z
fixed&squashed
This commit is contained in:
commit
c3ae854634
@ -12,6 +12,7 @@
|
|||||||
namespace Symfony\Component\Form\Tests\Util;
|
namespace Symfony\Component\Form\Tests\Util;
|
||||||
|
|
||||||
use Symfony\Component\Form\Util\PropertyPath;
|
use Symfony\Component\Form\Util\PropertyPath;
|
||||||
|
use Symfony\Component\Form\Util\FormUtil;
|
||||||
|
|
||||||
class PropertyPathCollectionTest_Car
|
class PropertyPathCollectionTest_Car
|
||||||
{
|
{
|
||||||
@ -72,6 +73,11 @@ class PropertyPathCollectionTest_CarOnlyRemover
|
|||||||
public function getAxes() {}
|
public function getAxes() {}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
class PropertyPathCollectionTest_CarNoAdderAndRemover
|
||||||
|
{
|
||||||
|
public function getAxes() {}
|
||||||
|
}
|
||||||
|
|
||||||
class PropertyPathCollectionTest_CompositeCar
|
class PropertyPathCollectionTest_CompositeCar
|
||||||
{
|
{
|
||||||
public function getStructure() {}
|
public function getStructure() {}
|
||||||
@ -195,4 +201,47 @@ abstract class PropertyPathCollectionTest extends \PHPUnit_Framework_TestCase
|
|||||||
|
|
||||||
$path->setValue($car, $axesAfter);
|
$path->setValue($car, $axesAfter);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @dataProvider noAdderRemoverData
|
||||||
|
*/
|
||||||
|
public function testNoAdderAndRemoverThrowsSensibleError($path, $message)
|
||||||
|
{
|
||||||
|
$car = $this->getMock(__CLASS__ . '_CarNoAdderAndRemover');
|
||||||
|
$axesBefore = $this->getCollection(array(1 => 'second', 3 => 'fourth'));
|
||||||
|
$axesAfter = $this->getCollection(array(0 => 'first', 1 => 'second', 2 => 'third'));
|
||||||
|
|
||||||
|
try {
|
||||||
|
$path->setValue($car, $axesAfter);
|
||||||
|
$this->fail('An expected exception was not thrown!');
|
||||||
|
} catch (\Symfony\Component\Form\Exception\InvalidPropertyException $e) {
|
||||||
|
$this->assertEquals(str_replace("{class}", get_class($car), $message), $e->getMessage());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
public function noAdderRemoverData()
|
||||||
|
{
|
||||||
|
$data = array();
|
||||||
|
|
||||||
|
$propertyPath = new PropertyPath('axes');
|
||||||
|
$expectedMessage = sprintf(
|
||||||
|
'Neither element "axes" nor method "setAxes()" exists in class '
|
||||||
|
.'"{class}", nor could adders and removers be found based on the '
|
||||||
|
.'guessed singulars: %s (provide a singular by suffixing the '
|
||||||
|
.'property path with "|{singular}" to override the guesser)',
|
||||||
|
implode(', ', (array) $singulars = FormUtil::singularify('Axes'))
|
||||||
|
);
|
||||||
|
$data[] = array($propertyPath, $expectedMessage);
|
||||||
|
|
||||||
|
$propertyPath = new PropertyPath('axes|boo');
|
||||||
|
$expectedMessage = sprintf(
|
||||||
|
'Neither element "axes" nor method "setAxes()" exists in class '
|
||||||
|
.'"{class}", nor could adders and removers be found based on the '
|
||||||
|
.'passed singular: %s',
|
||||||
|
'boo'
|
||||||
|
);
|
||||||
|
$data[] = array($propertyPath, $expectedMessage);
|
||||||
|
|
||||||
|
return $data;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -437,6 +437,8 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
|
|||||||
*/
|
*/
|
||||||
protected function writeProperty(&$objectOrArray, $property, $singular, $isIndex, $value)
|
protected function writeProperty(&$objectOrArray, $property, $singular, $isIndex, $value)
|
||||||
{
|
{
|
||||||
|
$adderRemoverError = null;
|
||||||
|
|
||||||
if ($isIndex) {
|
if ($isIndex) {
|
||||||
if (!$objectOrArray instanceof \ArrayAccess && !is_array($objectOrArray)) {
|
if (!$objectOrArray instanceof \ArrayAccess && !is_array($objectOrArray)) {
|
||||||
throw new InvalidPropertyException(sprintf('Index "%s" cannot be modified in object of type "%s" because it doesn\'t implement \ArrayAccess', $property, get_class($objectOrArray)));
|
throw new InvalidPropertyException(sprintf('Index "%s" cannot be modified in object of type "%s" because it doesn\'t implement \ArrayAccess', $property, get_class($objectOrArray)));
|
||||||
@ -446,8 +448,14 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
|
|||||||
} elseif (is_object($objectOrArray)) {
|
} elseif (is_object($objectOrArray)) {
|
||||||
$reflClass = new ReflectionClass($objectOrArray);
|
$reflClass = new ReflectionClass($objectOrArray);
|
||||||
|
|
||||||
|
// The plural form is the last element of the property path
|
||||||
|
$plural = $this->camelize($this->elements[$this->length - 1]);
|
||||||
|
|
||||||
|
// Any of the two methods is required, but not yet known
|
||||||
|
$singulars = null !== $singular ? array($singular) : (array) FormUtil::singularify($plural);
|
||||||
|
|
||||||
if (is_array($value) || $value instanceof Traversable) {
|
if (is_array($value) || $value instanceof Traversable) {
|
||||||
$methods = $this->findAdderAndRemover($reflClass, $singular);
|
$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;
|
$itemsToAdd = is_object($value) ? clone $value : $value;
|
||||||
@ -480,6 +488,13 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
|
|||||||
}
|
}
|
||||||
|
|
||||||
return;
|
return;
|
||||||
|
} else {
|
||||||
|
$adderRemoverError = ', nor could adders and removers be found based on the ';
|
||||||
|
if (null === $singular) {
|
||||||
|
$adderRemoverError .= 'guessed singulars: '.implode(', ', $singulars).' (provide a singular by suffixing the property path with "|{singular}" to override the guesser)';
|
||||||
|
} else {
|
||||||
|
$adderRemoverError .= 'passed singular: '.$singular;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -503,7 +518,7 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
|
|||||||
// needed to support \stdClass instances
|
// needed to support \stdClass instances
|
||||||
$objectOrArray->$property = $value;
|
$objectOrArray->$property = $value;
|
||||||
} else {
|
} else {
|
||||||
throw new InvalidPropertyException(sprintf('Neither element "%s" nor method "%s()" exists in class "%s"', $property, $setter, $reflClass->name));
|
throw new InvalidPropertyException(sprintf('Neither element "%s" nor method "%s()" exists in class "%s"%s', $property, $setter, $reflClass->name, $adderRemoverError));
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
throw new InvalidPropertyException(sprintf('Cannot write property "%s" in an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
|
throw new InvalidPropertyException(sprintf('Cannot write property "%s" in an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
|
||||||
@ -532,37 +547,8 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
|
|||||||
*
|
*
|
||||||
* @throws InvalidPropertyException If the property does not exist.
|
* @throws InvalidPropertyException If the property does not exist.
|
||||||
*/
|
*/
|
||||||
private function findAdderAndRemover(\ReflectionClass $reflClass, $singular)
|
private function findAdderAndRemover(\ReflectionClass $reflClass, array $singulars)
|
||||||
{
|
{
|
||||||
if (null !== $singular) {
|
|
||||||
$addMethod = 'add' . $this->camelize($singular);
|
|
||||||
$removeMethod = 'remove' . $this->camelize($singular);
|
|
||||||
|
|
||||||
if (!$this->isAccessible($reflClass, $addMethod, 1)) {
|
|
||||||
throw new InvalidPropertyException(sprintf(
|
|
||||||
'The public method "%s" with exactly one required parameter was not found on class %s',
|
|
||||||
$addMethod,
|
|
||||||
$reflClass->name
|
|
||||||
));
|
|
||||||
}
|
|
||||||
|
|
||||||
if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
|
|
||||||
throw new InvalidPropertyException(sprintf(
|
|
||||||
'The public method "%s" with exactly one required parameter was not found on class %s',
|
|
||||||
$removeMethod,
|
|
||||||
$reflClass->name
|
|
||||||
));
|
|
||||||
}
|
|
||||||
|
|
||||||
return array($addMethod, $removeMethod);
|
|
||||||
}
|
|
||||||
|
|
||||||
// The plural form is the last element of the property path
|
|
||||||
$plural = $this->camelize($this->elements[$this->length - 1]);
|
|
||||||
|
|
||||||
// Any of the two methods is required, but not yet known
|
|
||||||
$singulars = (array) FormUtil::singularify($plural);
|
|
||||||
|
|
||||||
foreach ($singulars as $singular) {
|
foreach ($singulars as $singular) {
|
||||||
$addMethod = 'add' . $singular;
|
$addMethod = 'add' . $singular;
|
||||||
$removeMethod = 'remove' . $singular;
|
$removeMethod = 'remove' . $singular;
|
||||||
|
Reference in New Issue
Block a user