bug #29355 [PropertyAccess] calculate cache keys for property setters…

… depending on the value (xabbuh)

This PR was merged into the 3.4 branch.

Discussion
----------

[PropertyAccess] calculate cache keys for property setters depending on the value

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29340
| License       | MIT
| Doc PR        |

Commits
-------

fa23437 calculate cache keys for property setters depending on the value

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -687,7 +687,8 @@ private function writeCollection($zval, $property, $collection, $addMethod, $rem
      */
     private function getWriteAccessInfo($class, $property, $value)
     {
-        $key = str_replace('\\', '.', $class).'..'.$property;
+        $useAdderAndRemover = \is_array($value) || $value instanceof \Traversable;
+        $key = str_replace('\\', '.', $class).'..'.$property.'..'.(int) $useAdderAndRemover;
 
         if (isset($this->writePropertyCache[$key])) {
             return $this->writePropertyCache[$key];
@@ -707,6 +708,16 @@ private function getWriteAccessInfo($class, $property, $value)
         $camelized = $this->camelize($property);
         $singulars = (array) Inflector::singularize($camelized);
 
+        if ($useAdderAndRemover) {
+            $methods = $this->findAdderAndRemover($reflClass, $singulars);
+
+            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($access[self::ACCESS_TYPE])) {
             $setter = 'set'.$camelized;
             $getsetter = lcfirst($camelized); // jQuery style, e.g. read: last(), write: last($item)
@@ -728,22 +739,16 @@ private function getWriteAccessInfo($class, $property, $value)
                 $access[self::ACCESS_TYPE] = self::ACCESS_TYPE_MAGIC;
                 $access[self::ACCESS_NAME] = $setter;
             } elseif (null !== $methods = $this->findAdderAndRemover($reflClass, $singulars)) {
-                if (\is_array($value) || $value instanceof \Traversable) {
-                    $access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
-                    $access[self::ACCESS_ADDER] = $methods[0];
-                    $access[self::ACCESS_REMOVER] = $methods[1];
-                } else {
-                    $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)
-                    );
-                }
+                $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(
@@ -783,6 +788,18 @@ private function isPropertyWritable($object, $property)
 
         $access = $this->getWriteAccessInfo(\get_class($object), $property, array());
 
+        $isWritable = self::ACCESS_TYPE_METHOD === $access[self::ACCESS_TYPE]
+            || self::ACCESS_TYPE_PROPERTY === $access[self::ACCESS_TYPE]
+            || self::ACCESS_TYPE_ADDER_AND_REMOVER === $access[self::ACCESS_TYPE]
+            || (!$access[self::ACCESS_HAS_PROPERTY] && property_exists($object, $property))
+            || self::ACCESS_TYPE_MAGIC === $access[self::ACCESS_TYPE];
+
+        if ($isWritable) {
+            return true;
+        }
+
+        $access = $this->getWriteAccessInfo(\get_class($object), $property, '');
+
         return self::ACCESS_TYPE_METHOD === $access[self::ACCESS_TYPE]
             || self::ACCESS_TYPE_PROPERTY === $access[self::ACCESS_TYPE]
             || self::ACCESS_TYPE_ADDER_AND_REMOVER === $access[self::ACCESS_TYPE]

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

@@ -719,7 +719,27 @@ public function testWriteToPluralPropertyWhileSingularOneExists()
         $this->propertyAccessor->isWritable($object, 'emails'); //cache access info
         $this->propertyAccessor->setValue($object, 'emails', array('test@email.com'));
 
-        self::assertEquals(array('test@email.com'), $object->getEmails());
-        self::assertNull($object->getEmail());
+        $this->assertEquals(array('test@email.com'), $object->getEmails());
+        $this->assertNull($object->getEmail());
+    }
+
+    public function testAdderAndRemoverArePreferredOverSetter()
+    {
+        $object = new TestPluralAdderRemoverAndSetter();
+
+        $this->propertyAccessor->isWritable($object, 'emails'); //cache access info
+        $this->propertyAccessor->setValue($object, 'emails', array('test@email.com'));
+
+        $this->assertEquals(array('test@email.com'), $object->getEmails());
+    }
+
+    public function testAdderAndRemoverArePreferredOverSetterForSameSingularAndPlural()
+    {
+        $object = new TestPluralAdderRemoverAndSetterSameSingularAndPlural();
+
+        $this->propertyAccessor->isWritable($object, 'aircraft'); //cache access info
+        $this->propertyAccessor->setValue($object, 'aircraft', array('aeroplane'));
+
+        $this->assertEquals(array('aeroplane'), $object->getAircraft());
     }
 }

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

@@ -0,0 +1,37 @@
+<?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;
+
+class TestPluralAdderRemoverAndSetter
+{
+    private $emails = array();
+
+    public function getEmails()
+    {
+        return $this->emails;
+    }
+
+    public function setEmails(array $emails)
+    {
+        $this->emails = array('foo@email.com');
+    }
+
+    public function addEmail($email)
+    {
+        $this->emails[] = $email;
+    }
+
+    public function removeEmail($email)
+    {
+        $this->emails = array_diff($this->emails, array($email));
+    }
+}

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

@@ -0,0 +1,28 @@
+<?php
+
+namespace Symfony\Component\PropertyAccess\Tests;
+
+class TestPluralAdderRemoverAndSetterSameSingularAndPlural
+{
+    private $aircraft = array();
+
+    public function getAircraft()
+    {
+        return $this->aircraft;
+    }
+
+    public function setAircraft(array $aircraft)
+    {
+        $this->aircraft = array('plane');
+    }
+
+    public function addAircraft($aircraft)
+    {
+        $this->aircraft[] = $aircraft;
+    }
+
+    public function removeAircraft($aircraft)
+    {
+        $this->aircraft = array_diff($this->aircraft, array($aircraft));
+    }
+}