8000 feature #31194 [PropertyAccess] Improve errors when trying to find a … · symfony/symfony@9ab4f14 · GitHub
[go: up one dir, main page]
Skip to content

Commit 9ab4f14

Browse files
fabpotfabpot
committed
feature #31194 [PropertyAccess] Improve errors when trying to find a writable property (pierredup)
This PR was submitted for the master branch but it was merged into the 4.4 branch instead (closes #31194). Discussion ---------- [PropertyAccess] Improve errors when trying to find a writable property | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17907 | License | MIT | Doc PR | N/A When setting a property using an adder/remove, the error message is very generic if the methods don't fit the exact requirements (both the adder and remover need to be defined and accept at least one argument). This can be confusing when you already have the methods `addFoo()` and `removeFoo()` defined (but without any parameters in the signature), but the error message states that the method doesn't exist or don't have public access. So this PR tries to improve the error message if a property isn't writable by doing the following: * If only one of the add/remove methods is implemented, indicate that the other method is needed as well. * If any of the adder/remover, setter or magic methods (`__call` or `__set`) don't have the required number of parameters, make it clear that the methods need to define the correct number of parameter. * The any of the access methods were found, but don't have public access, make it clear that the method needs to be defined as public, ```php class Foo { public function addBar($value) { } public function removeBar() { } } ``` **Before:** ``` Neither the property "bar" nor one of the methods "addBar()/removeBar()", "setBar()", "bar()", "__set()" or "__call()" exist and have public access in class "Foo". ``` **After:** ``` The method "removeBar" requires at least "1" parameters, "0" found. ``` Commits ------- f90a9fd Improve errors when trying to find a writable property
2 parents 95e8a65 + f90a9fd commit 9ab4f14

File tree

6 files changed

+194
-31
lines changed

6 files changed

+194
-31
lines changed

‎src/Symfony/Component/PropertyAccess/PropertyAccessor.php

Lines changed: 87 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -636,14 +636,24 @@ private function getWriteAccessInfo(string $class, string $property, $value): ar
636636
$access[self::ACCESS_HAS_PROPERTY] = $reflClass->hasProperty($property);
637637
$camelized = $this->camelize($property);
638638
$singulars = (array) Inflector::singularize($camelized);
639+
$errors = [];
639640

640641
if ($useAdderAndRemover) {
641-
$methods = $this->findAdderAndRemover($reflClass, $singulars);
642+
foreach ($this->findAdderAndRemover($reflClass, $singulars) as $methods) {
643+
if (3 === \count($methods)) {
644+
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
645+
$access[self::ACCESS_ADDER] = $methods[self::ACCESS_ADDER];
646+
$access[self::ACCESS_REMOVER] = $methods[self::ACCESS_REMOVER];
647+
break;
648+
}
649+
650+
if (isset($methods[self::ACCESS_ADDER])) {
651+
$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]);
652+
}
642653

643-
if (null !== $methods) {
644-
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
645-
$access[self::ACCESS_ADDER] = $methods[0];
646-
$access[self::ACCESS_REMOVER] = $methods[1];
10000 654+
if (isset($methods[self::ACCESS_REMOVER])) {
655+
$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]);
656+
}
647657
}
648658
}
649659

@@ -667,30 +677,69 @@ private function getWriteAccessInfo(string $class, string $property, $value): ar
667677
// we call the getter and hope the __call do the job
668678
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_MAGIC;
669679
$access[self::ACCESS_NAME] = $setter;
670-
} elseif (null !== $methods = $this->findAdderAndRemover($reflClass, $singulars)) {
671-
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
672-
$access[self::ACCESS_NAME] = sprintf(
673-
'The property "%s" in class "%s" can be defined with the methods "%s()" but '.
674-
'the new value must be an array or an instance of \Traversable, '.
675-
'"%s" given.',
676-
$property,
677-
$reflClass->name,
678-
implode('()", "', $methods),
679-
\is_object($value) ? \get_class($value) : \gettype($value)
680-
);
681680
} else {
682-
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
683-
$access[self::ACCESS_NAME] = sprintf(
684-
'Neither the property "%s" nor one of the methods %s"%s()", "%s()", '.
685-
'"__set()" or "__call()" exist and have public access in class "%s".',
686-
$property,
687-
implode('', array_map(function ($singular) {
688-
return '"add'.$singular.'()"/"remove'.$singular.'()", ';
689-
}, $singulars)),
690-
$setter,
691-
$getsetter,
692-
$reflClass->name
693-
);
681+
foreach ($this->findAdderAndRemover($reflClass, $singulars) as $methods) {
682+
if (3 === \count($methods)) {
683+
$errors[] = sprintf(
684+
'The property "%s" in class "%s" can be defined with the methods "%s()" but '.
685+
'the new value must be an array or an instance of \Traversable, '.
686+
'"%s" given.',
687+
$property,
688+
$reflClass->name,
689+
implode('()", "', [$methods[self::ACCESS_ADDER], $methods[self::ACCESS_REM FDCC OVER]]),
690+
\is_object($value) ? \get_class($value) : \gettype($value)
691+
);
692+
}
693+
}
694+
695+
if (!isset($access[self::ACCESS_NAME])) {
696+
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
697+
698+
$triedMethods = [
699+
$setter => 1,
700+
$getsetter => 1,
701+
'__set' => 2,
702+
'__call' => 2,
703+
];
704+
705+
foreach ($singulars as $singular) {
706+
$triedMethods['add'.$singular] = 1;
707+
$triedMethods['remove'.$singular] = 1;
708+
}
709+
710+
foreach ($triedMethods as $methodName => $parameters) {
711+
if (!$reflClass->hasMethod($methodName)) {
712+
continue;
713+
}
714+
715+
$method = $reflClass->getMethod($methodName);
716+
717+
if (!$method->isPublic()) {
718+
$errors[] = sprintf('The method "%s" in class "%s" was found but does not have public access', $methodName, $reflClass->name);
719+
continue;
720+
}
721+
722+
if ($method->getNumberOfRequiredParameters() > $parameters || $method->getNumberOfParameters() < $parameters) {
723+
$errors[] = sprintf('The method "%s" in class "%s" requires %d arguments, but should accept only %d', $methodName, $reflClass->name, $method->getNumberOfRequiredParameters(), $parameters);
724+
}
725+
}
726+
727+
if (\count($errors)) {
728+
$access[self::ACCESS_NAME] = implode('. ', $errors).'.';
729+
} else {
730+
$access[self::ACCESS_NAME] = sprintf(
731+
'Neither the property "%s" nor one of the methods %s"%s()", "%s()", '.
732+
'"__set()" or "__call()" exist and have public access in class "%s".',
733+
$property,
734+
implode('', array_map( F438 function ($singular) {
735+
return '"add'.$singular.'()"/"remove'.$singular.'()", ';
736+
}, $singulars)),
737+
$setter,
738+
$getsetter,
739+
$reflClass->name
740+
);
741+
}
742+
}
694743
}
695744
}
696745

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

758808
$addMethodFound = $this->isMethodAccessible($reflClass, $addMethod, 1);
809+
810+
if ($addMethodFound) {
811+
$result[self::ACCESS_ADDER] = $addMethod;
812+
}
813+
759814
$removeMethodFound = $this->isMethodAccessible($reflClass, $removeMethod, 1);
760815

761-
if ($addMethodFound && $removeMethodFound) {
762-
return [$addMethod, $removeMethod];
816+
if ($removeMethodFound) {
817+
$result[self::ACCESS_REMOVER] = $removeMethod;
763818
}
819+
820+
yield $result;
764821
}
765822
}
766823

‎src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidArgumentLength.php

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\PropertyAccess\Tests\Fixtures;
13+
14+
class TestAdderRemoverInvalidArgumentLength
15+
{
16+
public function addFoo()
17+
{
18+
}
19+
20+
public function removeFoo($var1, $var2)
21+
{
22+
}
23+
24+
public function setBar($var1, $var2)
25+
{
26+
}
27+
}

‎src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidMethods.php

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\PropertyAccess\Tests\Fixtures;
13+
14+
class TestAdderRemoverInvalidMethods
15+
{
16+
public function addFoo($foo)
17+
{
18+
}
19+
20+
public function removeBar($foo)
21+
{
22+
}
23+
}

‎src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestClassSetValue.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,4 +29,8 @@ public function __construct($value)
2929
{
3030
$this->value = $value;
3131
}
32+
33+
private function setFoo()
34+
{
35+
}
3236
}

‎src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ public function testIsWritableReturnsFalseIfNoAdderNorRemoverExists()
189189

190190
/**
191191
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
192-
* 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./
192+
* @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./
193193
*/
194194
public function testSetValueFailsIfAdderAndRemoverExistButValueIsNotTraversable()
195195
{

‎src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
use Symfony\Component\PropertyAccess\PropertyAccess;
1818
use Symfony\Component\PropertyAccess\PropertyAccessor;
1919
use Symfony\Component\PropertyAccess\Tests\Fixtures\ReturnTyped;
20+
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestAdderRemoverInvalidArgumentLength;
21+
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestAdderRemoverInvalidMethods;
2022
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClass;
2123
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassIsWritable;
2224
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassMagicCall;
@@ -762,4 +764,54 @@ public function testAdderAndRemoverArePreferredOverSetterForSameSingularAndPlura
762764

763765
$this->assertEquals(['aeroplane'], $object->getAircraft());
764766
}
767+
768+
/**
769+
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
770+
* @expectedExceptionMessageRegExp /.*The add method "addFoo" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidMethods" was found, but the corresponding remove method "removeFoo" was not found\./
771+
*/
772+
public function testAdderWithoutRemover()
773+
{
774+
$object = new TestAdderRemoverInvalidMethods();
775+
$this->propertyAccessor->setValue($object, 'foos', [1, 2]);
776+
}
777+
778+
/**
779+
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
780+
* @expectedExceptionMessageRegExp /.*The remove method "removeBar" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidMethods" was found, but the corresponding add method "addBar" was not found\./
781+
*/
782+
public function testRemoverWithoutAdder()
783+
{
784+
$object = new TestAdderRemoverInvalidMethods();
785+
$this->propertyAccessor->setValue($object, 'bars', [1, 2]);
786+
}
787+
788+
/**
789+
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
790+
* @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\./
791+
*/
792+
public function testAdderAndRemoveNeedsTheExactParametersDefined()
793+
{
794+
$object = new TestAdderRemoverInvalidArgumentLength();
795+
$this->propertyAccessor->setValue($object, 'foo', [1, 2]);
796+
}
797+
798+
/**
799+
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
800+
* @expectedExceptionMessageRegExp /.*The method "setBar" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestAdderRemoverInvalidArgumentLength" requires 2 arguments, but should accept only 1\./
801+
*/
802+
public function testSetterNeedsTheExactParametersDefined()
803+
{
804+
$object = new TestAdderRemoverInvalidArgumentLength();
805+
$this->propertyAccessor->setValue($object, 'bar', [1, 2]);
806+
}
807+
808+
/**
809+
* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
810+
* @expectedExceptionMessageRegExp /.*The method "setFoo" in class "Symfony\\Component\\PropertyAccess\\Tests\\Fixtures\\TestClassSetValue" was found but does not have public access./
811+
*/
812+
public function testSetterNeedsPublicAccess()
813+
{
814+
$object = new TestClassSetValue(0);
815+
$this->propertyAccessor->setValue($object, 'foo', 1);
816+
}
765817
}

0 commit comments

Comments
 (0)
0