Skip to content

[PropertyAccess] Improve errors when trying to find a writable property #31194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 15, 2019
Merged

[PropertyAccess] Improve errors when trying to find a writable property #31194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 87 additions & 30 deletions src/Symfony/Component/PropertyAccess/PropertyAccessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,14 +636,24 @@ private function getWriteAccessInfo(string $class, string $property, $value): ar
$access[self::ACCESS_HAS_PROPERTY] = $reflClass->hasProperty($property);
$camelized = $this->camelize($property);
$singulars = (array) Inflector::singularize($camelized);
$errors = [];

if ($useAdderAndRemover) {
$methods = $this->findAdderAndRemover($reflClass, $singulars);
foreach ($this->findAdderAndRemover($reflClass, $singulars) as $methods) {
if (3 === \count($methods)) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
$access[self::ACCESS_ADDER] = $methods[self::ACCESS_ADDER];
$access[self::ACCESS_REMOVER] = $methods[self::ACCESS_REMOVER];
break;
}

if (isset($methods[self::ACCESS_ADDER])) {
$errors[] = sprintf('The add method "%s" in class "%s" was found, but the corresponding remove method "%s" was not found', $methods['methods'][self::ACCESS_ADDER], $reflClass->name, $methods['methods'][self::ACCESS_REMOVER]);
}

if (null !== $methods) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
$access[self::ACCESS_ADDER] = $methods[0];
$access[self::ACCESS_REMOVER] = $methods[1];
if (isset($methods[self::ACCESS_REMOVER])) {
$errors[] = sprintf('The remove method "%s" in class "%s" was found, but the corresponding add method "%s" was not found', $methods['methods'][self::ACCESS_REMOVER], $reflClass->name, $methods['methods'][self::ACCESS_ADDER]);
}
}
}

Expand All @@ -667,30 +677,69 @@ private function getWriteAccessInfo(string $class, string $property, $value): ar
// we call the getter and hope the __call do the job
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_MAGIC;
$access[self::ACCESS_NAME] = $setter;
} elseif (null !== $methods = $this->findAdderAndRemover($reflClass, $singulars)) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
$access[self::ACCESS_NAME] = sprintf(
'The property "%s" in class "%s" can be defined with the methods "%s()" but '.
'the new value must be an array or an instance of \Traversable, '.
'"%s" given.',
$property,
$reflClass->name,
implode('()", "', $methods),
\is_object($value) ? \get_class($value) : \gettype($value)
);
} else {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
$access[self::ACCESS_NAME] = sprintf(
'Neither the property "%s" nor one of the methods %s"%s()", "%s()", '.
'"__set()" or "__call()" exist and have public access in class "%s".',
$property,
implode('', array_map(function ($singular) {
return '"add'.$singular.'()"/"remove'.$singular.'()", ';
}, $singulars)),
$setter,
$getsetter,
$reflClass->name
);
foreach ($this->findAdderAndRemover($reflClass, $singulars) as $methods) {
if (3 === \count($methods)) {
$errors[] = sprintf(
'The property "%s" in class "%s" can be defined with the methods "%s()" but '.
'the new value must be an array or an instance of \Traversable, '.
'"%s" given.',
$property,
$reflClass->name,
implode('()", "', [$methods[self::ACCESS_ADDER], $methods[self::ACCESS_REMOVER]]),
\is_object($value) ? \get_class($value) : \gettype($value)
);
}
}

if (!isset($access[self::ACCESS_NAME])) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;

$triedMethods = [
$setter => 1,
$getsetter => 1,
'__set' => 2,
'__call' => 2,
];

foreach ($singulars as $singular) {
$triedMethods['add'.$singular] = 1;
$triedMethods['remove'.$singular] = 1;
}

foreach ($triedMethods as $methodName => $parameters) {
if (!$reflClass->hasMethod($methodName)) {
continue;
}

$method = $reflClass->getMethod($methodName);

if (!$method->isPublic()) {
$errors[] = sprintf('The method "%s" in class "%s" was found but does not have public access', $methodName, $reflClass->name);
continue;
}

if ($method->getNumberOfRequiredParameters() > $parameters || $method->getNumberOfParameters() < $parameters) {
$errors[] = sprintf('The method "%s" in class "%s" requires %d arguments, but should accept only %d', $methodName, $reflClass->name, $method->getNumberOfRequiredParameters(), $parameters);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC (but I might be completely wrong):

requires %d arguments -> that's the number of passed values right? What about was passed %d values, but you. must pass exactly %s arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is when a method is defined with the incorrect number of parameters. E.G. the method is defined as function addFoo(), but it needs to be defined as function addFoo($var). So this message tells the user that the method is currently defined to require x arguments, but it should be defined to only require y arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pierredup Can you change the current message to make it more explicit?

}
}

if (\count($errors)) {
$access[self::ACCESS_NAME] = implode('. ', $errors).'.';
} else {
$access[self::ACCESS_NAME] = sprintf(
'Neither the property "%s" nor one of the methods %s"%s()", "%s()", '.
'"__set()" or "__call()" exist and have public access in class "%s".',
$property,
implode('', array_map(function ($singular) {
return '"add'.$singular.'()"/"remove'.$singular.'()", ';
}, $singulars)),
$setter,
$getsetter,
$reflClass->name
);
}
}
}
}

Expand Down Expand Up @@ -754,13 +803,21 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula
foreach ($singulars as $singular) {
$addMethod = 'add'.$singular;
$removeMethod = 'remove'.$singular;
$result = ['methods' => [self::ACCESS_ADDER => $addMethod, self::ACCESS_REMOVER => $removeMethod]];

$addMethodFound = $this->isMethodAccessible($reflClass, $addMethod, 1);

if ($addMethodFound) {
$result[self::ACCESS_ADDER] = $addMethod;
}

$removeMethodFound = $this->isMethodAccessible($reflClass, $removeMethod, 1);

if ($addMethodFound && $removeMethodFound) {
return [$addMethod, $removeMethod];
if ($removeMethodFound) {
$result[self::ACCESS_REMOVER] = $removeMethod;
}

yield $result;
}
}

Expand Down
27 changes: 27 additions & 0 deletions ...Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidArgumentLength.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\PropertyAccess\Tests\Fixtures;

class TestAdderRemoverInvalidArgumentLength
{
public function addFoo()
{
}

public function removeFoo($var1, $var2)
{
}

public function setBar($var1, $var2)
{
}
}
23 changes: 23 additions & 0 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidMethods.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\PropertyAccess\Tests\Fixtures;

class TestAdderRemoverInvalidMethods
{
public function addFoo($foo)
{
}

public function removeBar($foo)
{
}
}
4 changes: 4 additions & 0 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestClassSetValue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,8 @@ public function __construct($value)
{
$this->value = $value;
}

private function setFoo()
{
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ public function testIsWritableReturnsFalseIfNoAdderNorRemoverExists()

/**
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* expectedExceptionMessageRegExp /The property "axes" in class "Mock_PropertyAccessorCollectionTest_Car[^"]*" can be defined with the methods "addAxis()", "removeAxis()" but the new value must be an array or an instance of \Traversable, "string" given./
* @expectedExceptionMessageRegExp /Could not determine access type for property "axes" in class "Mock_PropertyAccessorCollectionTest_Car[^"]*": The property "axes" in class "Mock_PropertyAccessorCollectionTest_Car[^"]*" can be defined with the methods "addAxis\(\)", "removeAxis\(\)" but the new value must be an array or an instance of \\Traversable, "string" given./
*/
public function testSetValueFailsIfAdderAndRemoverExistButValueIsNotTraversable()
{
Expand Down
52 changes: 52 additions & 0 deletions src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessor;
use Symfony\Component\PropertyAccess\Tests\Fixtures\ReturnTyped;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestAdderRemoverInvalidArgumentLength;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestAdderRemoverInvalidMethods;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClass;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassIsWritable;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassMagicCall;
Expand Down Expand Up @@ -762,4 +764,54 @@ public function testAdderAndRemoverArePreferredOverSetterForSameSingularAndPlura

$this->assertEquals(['aeroplane'], $object->getAircraft());
}

/**
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* @expectedExceptionMessageRegExp /.*The add method "addFoo" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidMethods" was found, but the corresponding remove method "removeFoo" was not found\./
*/
public function testAdderWithoutRemover()
{
$object = new TestAdderRemoverInvalidMethods();
$this->propertyAccessor->setValue($object, 'foos', [1, 2]);
}

/**
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* @expectedExceptionMessageRegExp /.*The remove method "removeBar" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidMethods" was found, but the corresponding add method "addBar" was not found\./
*/
public function testRemoverWithoutAdder()
{
$object = new TestAdderRemoverInvalidMethods();
$this->propertyAccessor->setValue($object, 'bars', [1, 2]);
}

/**
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* @expectedExceptionMessageRegExp /.*The method "addFoo" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidArgumentLength" requires 0 arguments, but should accept only 1\. The method "removeFoo" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidArgumentLength" requires 2 arguments, but should accept only 1\./
*/
public function testAdderAndRemoveNeedsTheExactParametersDefined()
{
$object = new TestAdderRemoverInvalidArgumentLength();
$this->propertyAccessor->setValue($object, 'foo', [1, 2]);
}

/**
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* @expectedExceptionMessageRegExp /.*The method "setBar" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidArgumentLength" requires 2 arguments, but should accept only 1\./
*/
public function testSetterNeedsTheExactParametersDefined()
{
$object = new TestAdderRemoverInvalidArgumentLength();
$this->propertyAccessor->setValue($object, 'bar', [1, 2]);
}

/**
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* @expectedExceptionMessageRegExp /.*The method "setFoo" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestClassSetValue" was found but does not have public access./
*/
public function testSetterNeedsPublicAccess()
{
$object = new TestClassSetValue(0);
$this->propertyAccessor->setValue($object, 'foo', 1);
}
}