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 ------- f90a9fd771 Improve errors when trying to find a writable property
2019-07-15 08:46:38 +02:00 · 2019-07-15 08:46:38 +02:00 · 9ab4f14a6e
commit 9ab4f14a6e
parent 95e8a651c2 f90a9fd771
6 changed files with 194 additions and 31 deletions
--- a/src/Symfony/Component/PropertyAccess/PropertyAccessor.php
+++ b/src/Symfony/Component/PropertyAccess/PropertyAccessor.php
@ -636,14 +636,24 @@ class PropertyAccessor implements PropertyAccessorInterface
        $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 (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_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 (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]);
+                }
            }
        }

@ -667,30 +677,69 @@ class PropertyAccessor implements PropertyAccessorInterface
                // 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);
+                        }
+                    }
+
+                    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
+                        );
+                    }
+                }
            }
        }

@ -754,13 +803,21 @@ class PropertyAccessor implements PropertyAccessorInterface
        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;
        }
    }

--- a/src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidArgumentLength.php
+++ b/src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidArgumentLength.php
@ -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)
+    {
+    }
+}
--- a/src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidMethods.php
+++ b/src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestAdderRemoverInvalidMethods.php
@ -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)
+    {
+    }
+}
--- a/src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestClassSetValue.php
+++ b/src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestClassSetValue.php
@ -29,4 +29,8 @@ class TestClassSetValue
    {
        $this->value = $value;
    }
+
+    private function setFoo()
+    {
+    }
 }
--- a/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php
+++ b/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php
@ -189,7 +189,7 @@ abstract class PropertyAccessorCollectionTest extends PropertyAccessorArrayAcces

    /**
     * @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()
    {
--- a/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
+++ b/src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
@ -17,6 +17,8 @@ use Symfony\Component\PropertyAccess\Exception\NoSuchIndexException;
 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;
@ -762,4 +764,54 @@ class PropertyAccessorTest extends TestCase

        $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);
+    }
 }