merged branch bschussek/property-access-public-check (PR #7711)

This PR was merged into the master branch.

Discussion
----------

[PropertyAccess] Changed PropertyAccessor to continue searching when a non-public method/property are found

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | not necessary

Commits
-------

2a666cb [PropertyAccess] Changed PropertyAccessor to continue searching when a non-public method/property are found

UPGRADE-2.3.md

@@ -35,3 +35,55 @@ UPGRADE FROM 2.2 to 2.3
    "validation_groups" => false
    "validation_groups" => array()
    ```
+
+### PropertyAccess
+
+ * PropertyAccessor was changed to continue its search for a property or method
+   even if a non-public match was found. This means that the property "author"
+   in the following class will now correctly be found:
+
+   ```
+   class Article
+   {
+       public $author;
+
+       private function getAuthor()
+       {
+           // ...
+       }
+   }
+   ```
+
+   Although this is uncommon, similar cases exist in practice.
+
+   Instead of the PropertyAccessDeniedException that was thrown here, the more
+   generic NoSuchPropertyException is thrown now if no public property nor
+   method are found by the PropertyAccessor. PropertyAccessDeniedException was
+   removed completely.
+
+   Before:
+
+   ```
+   use Symfony\Component\PropertyAccess\Exception\PropertyAccessDeniedException;
+   use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
+
+   try {
+       $value = $accessor->getValue($article, 'author');
+   } catch (PropertyAccessDeniedException $e) {
+       // Method/property was found but not public
+   } catch (NoSuchPropertyException $e) {
+       // Method/property was not found
+   }
+   ```
+
+   After:
+
+   ```
+   use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
+
+   try {
+       $value = $accessor->getValue($article, 'author');
+   } catch (NoSuchPropertyException $e) {
+       // Method/property was not found or not public
+   }
+   ```

src/Symfony/Component/PropertyAccess/CHANGELOG.md

@@ -0,0 +1,10 @@
+CHANGELOG
+=========
+
+2.3.0
+------
+
+ * [BC BREAK] changed PropertyAccessor to continue its search for a property or
+   method even if a non-public match was found. Before, a PropertyAccessDeniedException
+   was thrown in this case. Class PropertyAccessDeniedException was removed
+   now.

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -12,7 +12,6 @@
 namespace Symfony\Component\PropertyAccess;
 
 use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
-use Symfony\Component\PropertyAccess\Exception\PropertyAccessDeniedException;
 use Symfony\Component\PropertyAccess\Exception\UnexpectedTypeException;
 
 /**
@@ -180,9 +179,8 @@ private function &readIndex(&$array, $index)
      *
      * @return mixed The value of the read property
      *
-     * @throws NoSuchPropertyException       If the property does not exist
-     * @throws PropertyAccessDeniedException If the property cannot be accessed due to
-     *                                       access restrictions (private or protected)
+     * @throws NoSuchPropertyException If the property does not exist or is not
+     *                                 public.
      */
     private function &readProperty(&$object, $property)
     {
@@ -202,41 +200,38 @@ private function &readProperty(&$object, $property)
         $getter = 'get'.$camelProp;
         $isser = 'is'.$camelProp;
         $hasser = 'has'.$camelProp;
+        $classHasProperty = $reflClass->hasProperty($property);
 
-        if ($reflClass->hasMethod($getter)) {
-            if (!$reflClass->getMethod($getter)->isPublic()) {
-                throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $getter, $reflClass->name));
-            }
-
+        if ($reflClass->hasMethod($getter) && $reflClass->getMethod($getter)->isPublic()) {
             $result[self::VALUE] = $object->$getter();
-        } elseif ($reflClass->hasMethod($isser)) {
-            if (!$reflClass->getMethod($isser)->isPublic()) {
-                throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $isser, $reflClass->name));
-            }
-
+        } elseif ($reflClass->hasMethod($isser) && $reflClass->getMethod($isser)->isPublic()) {
             $result[self::VALUE] = $object->$isser();
-        } elseif ($reflClass->hasMethod($hasser)) {
-            if (!$reflClass->getMethod($hasser)->isPublic()) {
-                throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $hasser, $reflClass->name));
-            }
-
+        } elseif ($reflClass->hasMethod($hasser) && $reflClass->getMethod($hasser)->isPublic()) {
             $result[self::VALUE] = $object->$hasser();
-        } elseif ($reflClass->hasMethod('__get')) {
-            // needed to support magic method __get
+        } elseif ($reflClass->hasMethod('__get') && $reflClass->getMethod('__get')->isPublic()) {
             $result[self::VALUE] = $object->$property;
-        } elseif ($reflClass->hasProperty($property)) {
-            if (!$reflClass->getProperty($property)->isPublic()) {
-                throw new PropertyAccessDeniedException(sprintf('Property "%s" is not public in class "%s". Maybe you should create the method "%s()" or "%s()" or "%s()"?', $property, $reflClass->name, $getter, $isser, $hasser));
-            }
-
+        } elseif ($classHasProperty && $reflClass->getProperty($property)->isPublic()) {
             $result[self::VALUE] =& $object->$property;
             $result[self::IS_REF] = true;
-        } elseif (property_exists($object, $property)) {
-            // needed to support \stdClass instances
+        } elseif (!$classHasProperty && property_exists($object, $property)) {
+            // Needed to support \stdClass instances. We need to explicitly
+            // exclude $classHasProperty, otherwise if in the previous clause
+            // a *protected* property was found on the class, property_exists()
+            // returns true, consequently the following line will result in a
+            // fatal error.
             $result[self::VALUE] =& $object->$property;
             $result[self::IS_REF] = true;
         } else {
-            throw new NoSuchPropertyException(sprintf('Neither property "%s" nor method "%s()" nor method "%s()" exists in class "%s"', $property, $getter, $isser, $reflClass->name));
+            throw new NoSuchPropertyException(sprintf(
+                'Neither the property "%s" nor one of the methods "%s()", '.
+                '"%s()", "%s()" or "__get()" exist and have public access in '.
+                'class "%s".',
+                $property,
+                $getter,
+                $isser,
+                $hasser,
+                $reflClass->name
+            ));
         }
 
         // Objects are always passed around by reference
@@ -268,18 +263,17 @@ private function writeIndex(&$array, $index, $value)
     /**
      * Sets the value of the property at the given index in the path
      *
-     * @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|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
      *
-     * @throws NoSuchPropertyException       If the property does not exist
-     * @throws PropertyAccessDeniedException If the property cannot be accessed due to
-     *                                       access restrictions (private or protected)
+     * @throws NoSuchPropertyException If the property does not exist or is not
+     *                                 public.
      */
     private function writeProperty(&$object, $property, $singular, $value)
     {
-        $adderRemoverError = null;
+        $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));
@@ -330,38 +324,39 @@ private function writeProperty(&$object, $property, $singular, $value)
 
                 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)';
-                    $adderRemoverError .= 'guessed singulars: '.implode(', ', $singulars);
-                } else {
-                    $adderRemoverError .= 'passed singular: '.$singular;
-                }
+                // 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).'()", ';
             }
         }
 
         $setter = 'set'.$this->camelize($property);
+        $classHasProperty = $reflClass->hasProperty($property);
 
-        if ($reflClass->hasMethod($setter)) {
-            if (!$reflClass->getMethod($setter)->isPublic()) {
-                throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $setter, $reflClass->name));
-            }
-
+        if ($reflClass->hasMethod($setter) && $reflClass->getMethod($setter)->isPublic()) {
             $object->$setter($value);
-        } elseif ($reflClass->hasMethod('__set')) {
-            // needed to support magic method __set
+        } elseif ($reflClass->hasMethod('__set') && $reflClass->getMethod('__set')->isPublic()) {
             $object->$property = $value;
-        } elseif ($reflClass->hasProperty($property)) {
-            if (!$reflClass->getProperty($property)->isPublic()) {
-                throw new PropertyAccessDeniedException(sprintf('Property "%s" is not public in class "%s"%s. Maybe you should create the method "%s()"?', $property, $reflClass->name, $adderRemoverError, $setter));
-            }
-
+        } elseif ($classHasProperty && $reflClass->getProperty($property)->isPublic()) {
             $object->$property = $value;
-        } elseif (property_exists($object, $property)) {
-            // needed to support \stdClass instances
+        } elseif (!$classHasProperty && property_exists($object, $property)) {
+            // Needed to support \stdClass instances. We need to explicitly
+            // exclude $classHasProperty, otherwise if in the previous clause
+            // a *protected* property was found on the class, property_exists()
+            // returns true, consequently the following line will result in a
+            // fatal error.
             $object->$property = $value;
         } else {
-            throw new NoSuchPropertyException(sprintf('Neither element "%s" nor method "%s()" exists in class "%s"%s', $property, $setter, $reflClass->name, $adderRemoverError));
+            throw new NoSuchPropertyException(sprintf(
+                'Neither the property "%s" nor one of the methods %s"%s()" or '.
+                '"__set()" exist and have public access in class "%s".',
+                $property,
+                $guessedAdders,
+                $setter,
+                $reflClass->name
+            ));
         }
     }
 
@@ -402,7 +397,7 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula
 
             if ($addMethodFound xor $removeMethodFound) {
                 throw new NoSuchPropertyException(sprintf(
-                    'Found the public method "%s", but did not find a public "%s" on class %s',
+                    'Found the public method "%s()", but did not find a public "%s()" on class %s',
                     $addMethodFound ? $addMethod : $removeMethod,
                     $addMethodFound ? $removeMethod : $addMethod,
                     $reflClass->name

src/Symfony/Component/PropertyAccess/PropertyAccessorInterface.php

@@ -43,11 +43,9 @@ 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
-     * @throws Exception\PropertyAccessDeniedException If a property cannot be accessed due to
-     *                                                 access restrictions (private or protected)
-     * @throws Exception\UnexpectedTypeException       If a value within the path is neither object
-     *                                                 nor array
+     * @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);
 
@@ -77,8 +75,7 @@ public function setValue(&$objectOrArray, $propertyPath, $value);
      *
      * @return mixed The value at the end of the property path
      *
-     * @throws Exception\NoSuchPropertyException       If the property/getter does not exist
-     * @throws Exception\PropertyAccessDeniedException If the property/getter exists but is not public
+     * @throws Exception\NoSuchPropertyException If a property does not exist or is not public.
      */
     public function getValue($objectOrArray, $propertyPath);
 }

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

@@ -288,14 +288,10 @@ public function noAdderRemoverData()
         $car = $this->getMock(__CLASS__.'_CarNoAdderAndRemover');
         $propertyPath = 'axes';
         $expectedMessage = sprintf(
-            'Neither element "axes" nor method "setAxes()" exists in class '
-                .'"%s", 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)'
-                ,
-            get_class($car),
-            implode(', ', (array) $singulars = StringUtil::singularify('Axes'))
+            'Neither the property "axes" nor one of the methods "addAx()", '.
+            '"addAxe()", "addAxis()", "setAxes()" or "__set()" exist and have '.
+            'public access in class "%s".',
+            get_class($car)
         );
         $data[] = array($car, $propertyPath, $expectedMessage);
 
@@ -316,14 +312,10 @@ public function noAdderRemoverData()
         $car = $this->getMock(__CLASS__.'_CarNoAdderAndRemoverWithProperty');
         $propertyPath = 'axes';
         $expectedMessage = sprintf(
-            'Property "axes" is not public in class "%s", 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)'
-               .'. Maybe you should '
-                .'create the method "setAxes()"?',
-            get_class($car),
-            implode(', ', (array) $singulars = StringUtil::singularify('Axes'))
+            'Neither the property "axes" nor one of the methods "addAx()", '.
+            '"addAxe()", "addAxis()", "setAxes()" or "__set()" exist and have '.
+            'public access in class "%s".',
+            get_class($car)
         );
         $data[] = array($car, $propertyPath, $expectedMessage);
 

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

@@ -114,7 +114,7 @@ public function testGetValueReadsPropertyWithCustomPropertyPath()
     }
 
     /**
-     * @expectedException \Symfony\Component\PropertyAccess\Exception\PropertyAccessDeniedException
+     * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
      */
     public function testGetValueThrowsExceptionIfPropertyIsNotPublic()
     {
@@ -138,7 +138,7 @@ public function testGetValueCamelizesGetterNames()
     }
 
     /**
-     * @expectedException \Symfony\Component\PropertyAccess\Exception\PropertyAccessDeniedException
+     * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
      */
     public function testGetValueThrowsExceptionIfGetterIsNotPublic()
     {
@@ -180,7 +180,7 @@ public function testGetValueReadsMagicGetThatReturnsConstant()
     }
 
     /**
-     * @expectedException \Symfony\Component\PropertyAccess\Exception\PropertyAccessDeniedException
+     * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
      */
     public function testGetValueThrowsExceptionIfIsserIsNotPublic()
     {
@@ -295,7 +295,7 @@ public function testSetValueCamelizesSetterNames()
     }
 
     /**
-     * @expectedException \Symfony\Component\PropertyAccess\Exception\PropertyAccessDeniedException
+     * @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
      */
     public function testSetValueThrowsExceptionIfGetterIsNotPublic()
     {