[!!!][CLEANUP] ObjectAccess 85/39585/21
authorClaus Due <claus@namelesscoder.net>
Fri, 2 Sep 2016 09:58:35 +0000 (11:58 +0200)
committerMarkus Klein <markus.klein@typo3.org>
Fri, 7 Oct 2016 11:07:52 +0000 (13:07 +0200)
This patch aims to clean up and improve the
ObjectAccess class which currently does a
*lot* of unnecessary operations, but is intended
for use in many places especially with repeated
use. Therefore, any even small optimisation in
this class would be a benefit.

* Uses more native PHP methods where reasonable
* Uses fewer method calls where reasonable
* Gets rid of a variable passed by reference
* More cases return NULL rather than throw Exceptions
* Fastest decisions and access methods come first
* Reflection-based access isolated to edge cases and
  access with the "force" flag being TRUE.
* Sacrifices ability to read objects of types other
  than persisted objects contained in an ObjectStorage
  or subclass of ObjectStorage.
* Changes verdict from FALSE to TRUE when determining
  if a dynamically added property exists on an object
  (these are, by definition, publicly accessible).

Change-Id: Ib17051a43f61bb73a1bd5a8a6c710f54eec8f769
Resolves: #66995
Releases: master
Reviewed-on: https://review.typo3.org/39585
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Stefan Neufeind <typo3.neufeind@speedpartner.de>
Tested-by: Stefan Neufeind <typo3.neufeind@speedpartner.de>
Reviewed-by: Markus Klein <markus.klein@typo3.org>
Tested-by: Markus Klein <markus.klein@typo3.org>
typo3/sysext/core/Documentation/Changelog/master/Breaking-66995-ObjectAccessbehaviorschanged.rst [new file with mode: 0644]
typo3/sysext/extbase/Classes/Reflection/ObjectAccess.php
typo3/sysext/extbase/Tests/Unit/Reflection/ObjectAccessTest.php

diff --git a/typo3/sysext/core/Documentation/Changelog/master/Breaking-66995-ObjectAccessbehaviorschanged.rst b/typo3/sysext/core/Documentation/Changelog/master/Breaking-66995-ObjectAccessbehaviorschanged.rst
new file mode 100644 (file)
index 0000000..e3cdf25
--- /dev/null
@@ -0,0 +1,48 @@
+=================================================
+Breaking: #66995 - ObjectAccess behaviors changed
+=================================================
+
+Description
+===========
+
+The following changes are implemented in ObjectAccess:
+
+* Uses more native PHP methods where reasonable
+* Uses fewer method calls where reasonable
+* Gets rid of a variable passed by reference
+* More cases return ``null`` rather than throw Exceptions
+* Fastest decisions and access methods come first
+* Reflection-based access isolated to edge cases and
+  access with the "force direct access" flag enabled.
+* Sacrifices ability to read objects of types other
+  than persisted objects contained in an ObjectStorage
+  or subclass of ObjectStorage.
+* Changes verdict from ``false`` to ``true`` when
+  determining if a dynamically added property exists on
+  an object (these are by definition publicly accessible).
+
+
+Impact
+======
+
+* Performance improvement; optimising for most frequent case coming first, skipping expensive Reflection etc.
+* Reflection based access only happens when "force" flag is set to true in getProperty / getPropertyPath
+* Removes support for using objects of types other than persisted objects in any ObjectStorage implementation.
+* Changes behavior when accessing dynamically added properties on objects from previously false to now true (by definition they are public). This
+  improves compatibility with for example JSON sources decoded to stdClass.
+
+
+Affected Installations
+======================
+
+* Any code using ObjectStorage combined with objects that are not persistent objects (Extbase models)
+* Any code passing stdClass with dynamic properties to ``ObjectAccess::getProperty()`` and expecting ``null`` (real value will now be returned)
+
+
+Migration
+=========
+
+* If you have any ObjectStorage implementation containing other types than persisted objects, switch to any alternative (Iterator, ArrayAccess, etc.)
+* If you have Extbase code or Fluid templates using ObjectAccess to read stdClass instances with dynamically added properties and your code expects a
+  false verdict for such properties (highly unlikely!) inverse any conditions depending on the resolved value being null or empty to expect the actual
+  value/trueness of the value you address.
index 10fda6d..eb64bd9 100644 (file)
@@ -14,7 +14,7 @@ namespace TYPO3\CMS\Extbase\Reflection;
  * The TYPO3 project - inspiring people to share!
  */
 
-use TYPO3\CMS\Core\Utility\MathUtility;
+use TYPO3\CMS\Extbase\Persistence\ObjectStorage;
 
 /**
  * Provides methods to call appropriate getter/setter on an object given the
@@ -47,7 +47,6 @@ class ObjectAccess
      * @param string $propertyName name of the property to retrieve
      * @param bool $forceDirectAccess directly access property using reflection(!)
      *
-     * @throws Exception\PropertyNotAccessibleException
      * @throws \InvalidArgumentException in case $subject was not an object or $propertyName was not a string
      * @return mixed Value of the property
      */
@@ -59,12 +58,7 @@ class ObjectAccess
         if (!is_string($propertyName) && (!is_array($subject) && !$subject instanceof \ArrayAccess)) {
             throw new \InvalidArgumentException('Given property name is not of type string.', 1231178303);
         }
-        $propertyExists = false;
-        $propertyValue = self::getPropertyInternal($subject, $propertyName, $forceDirectAccess, $propertyExists);
-        if ($propertyExists === true) {
-            return $propertyValue;
-        }
-        throw new Exception\PropertyNotAccessibleException('The property "' . $propertyName . '" on the subject was not accessible.', 1263391473);
+        return self::getPropertyInternal($subject, $propertyName, $forceDirectAccess);
     }
 
     /**
@@ -77,66 +71,56 @@ class ObjectAccess
      * @param mixed $subject Object or array to get the property from
      * @param string $propertyName name of the property to retrieve
      * @param bool $forceDirectAccess directly access property using reflection(!)
-     * @param bool &$propertyExists (by reference) will be set to TRUE if the specified property exists and is gettable
      *
      * @throws Exception\PropertyNotAccessibleException
      * @return mixed Value of the property
      * @internal
      */
-    public static function getPropertyInternal($subject, $propertyName, $forceDirectAccess, &$propertyExists)
+    public static function getPropertyInternal($subject, $propertyName, $forceDirectAccess = false)
     {
+        // type check and conversion of iterator to numerically indexed array
         if ($subject === null || is_scalar($subject)) {
             return null;
+        } elseif (!$forceDirectAccess && ($subject instanceof \SplObjectStorage || $subject instanceof ObjectStorage)) {
+            $subject = iterator_to_array($subject, false);
         }
-        $propertyExists = true;
-        if (is_array($subject)) {
-            if (array_key_exists($propertyName, $subject)) {
+
+        // value get based on data type of $subject (possibly converted above)
+        if ($subject instanceof \ArrayAccess || is_array($subject)) {
+            // isset() is safe; array_key_exists would only be needed to determine
+            // if the value is NULL - and that's already what we return as fallback.
+            if (isset($subject[$propertyName])) {
                 return $subject[$propertyName];
             }
-            $propertyExists = false;
-            return null;
-        }
-        if ($forceDirectAccess === true) {
-            if (property_exists(get_class($subject), $propertyName)) {
-                $propertyReflection = new PropertyReflection(get_class($subject), $propertyName);
-                return $propertyReflection->getValue($subject);
-            } elseif (property_exists($subject, $propertyName)) {
+        } elseif (is_object($subject)) {
+            if ($forceDirectAccess) {
+                if (property_exists($subject, $propertyName)) {
+                    $propertyReflection = new PropertyReflection($subject, $propertyName);
+                    return $propertyReflection->getValue($subject);
+                } else {
+                    throw new Exception\PropertyNotAccessibleException('The property "' . $propertyName . '" on the subject does not exist.', 1302855001);
+                }
+            }
+            $upperCasePropertyName = ucfirst($propertyName);
+            $getterMethodName = 'get' . $upperCasePropertyName;
+            if (is_callable([$subject, $getterMethodName])) {
+                return $subject->{$getterMethodName}();
+            }
+            $getterMethodName = 'is' . $upperCasePropertyName;
+            if (is_callable([$subject, $getterMethodName])) {
+                return $subject->{$getterMethodName}();
+            }
+            $getterMethodName = 'has' . $upperCasePropertyName;
+            if (is_callable([$subject, $getterMethodName])) {
+                return $subject->{$getterMethodName}();
+            }
+            if (property_exists($subject, $propertyName)) {
                 return $subject->{$propertyName};
             } else {
                 throw new Exception\PropertyNotAccessibleException('The property "' . $propertyName . '" on the subject does not exist.', 1302855001);
             }
         }
-        if ($subject instanceof \SplObjectStorage || $subject instanceof \TYPO3\CMS\Extbase\Persistence\ObjectStorage) {
-            if (MathUtility::canBeInterpretedAsInteger($propertyName)) {
-                $index = 0;
-                foreach ($subject as $value) {
-                    if ($index === (int)$propertyName) {
-                        return $value;
-                    }
-                    $index++;
-                }
-                $propertyExists = false;
-                return null;
-            }
-        } elseif ($subject instanceof \ArrayAccess && isset($subject[$propertyName])) {
-            return $subject[$propertyName];
-        }
-        $getterMethodName = 'get' . ucfirst($propertyName);
-        if (is_callable([$subject, $getterMethodName])) {
-            return $subject->{$getterMethodName}();
-        }
-        $getterMethodName = 'is' . ucfirst($propertyName);
-        if (is_callable([$subject, $getterMethodName])) {
-            return $subject->{$getterMethodName}();
-        }
-        $getterMethodName = 'has' . ucfirst($propertyName);
-        if (is_callable([$subject, $getterMethodName])) {
-            return $subject->{$getterMethodName}();
-        }
-        if (is_object($subject) && array_key_exists($propertyName, get_object_vars($subject))) {
-            return $subject->{$propertyName};
-        }
-        $propertyExists = false;
+
         return null;
     }
 
@@ -156,12 +140,12 @@ class ObjectAccess
     public static function getPropertyPath($subject, $propertyPath)
     {
         $propertyPathSegments = explode('.', $propertyPath);
-        foreach ($propertyPathSegments as $pathSegment) {
-            $propertyExists = false;
-            $subject = self::getPropertyInternal($subject, $pathSegment, false, $propertyExists);
-            if (!$propertyExists || $subject === null) {
-                return $subject;
+        try {
+            foreach ($propertyPathSegments as $pathSegment) {
+                $subject = self::getPropertyInternal($subject, $pathSegment);
             }
+        } catch (Exception\PropertyNotAccessibleException $error) {
+            return null;
         }
         return $subject;
     }
@@ -187,7 +171,7 @@ class ObjectAccess
      */
     public static function setProperty(&$subject, $propertyName, $propertyValue, $forceDirectAccess = false)
     {
-        if (is_array($subject)) {
+        if (is_array($subject) || ($subject instanceof \ArrayAccess && !$forceDirectAccess)) {
             $subject[$propertyName] = $propertyValue;
             return true;
         }
@@ -197,24 +181,31 @@ class ObjectAccess
         if (!is_string($propertyName)) {
             throw new \InvalidArgumentException('Given property name is not of type string.', 1231178878);
         }
-        if ($forceDirectAccess === true) {
-            if (property_exists(get_class($subject), $propertyName)) {
-                $propertyReflection = new PropertyReflection(get_class($subject), $propertyName);
+        $result = true;
+        if ($forceDirectAccess) {
+            if (property_exists($subject, $propertyName)) {
+                $propertyReflection = new PropertyReflection($subject, $propertyName);
                 $propertyReflection->setAccessible(true);
                 $propertyReflection->setValue($subject, $propertyValue);
             } else {
                 $subject->{$propertyName} = $propertyValue;
             }
-        } elseif (is_callable([$subject, $setterMethodName = self::buildSetterMethodName($propertyName)])) {
+            return $result;
+        }
+        $setterMethodName = self::buildSetterMethodName($propertyName);
+        if (is_callable([$subject, $setterMethodName])) {
             $subject->{$setterMethodName}($propertyValue);
-        } elseif ($subject instanceof \ArrayAccess) {
-            $subject[$propertyName] = $propertyValue;
-        } elseif (array_key_exists($propertyName, get_object_vars($subject))) {
-            $subject->{$propertyName} = $propertyValue;
+        } elseif (property_exists($subject, $propertyName)) {
+            $reflection = new PropertyReflection($subject, $propertyName);
+            if ($reflection->isPublic()) {
+                $subject->{$propertyName} = $propertyValue;
+            } else {
+                $result = false;
+            }
         } else {
-            return false;
+            $result = false;
         }
-        return true;
+        return $result;
     }
 
     /**
@@ -235,7 +226,7 @@ class ObjectAccess
             throw new \InvalidArgumentException('$object must be an object, ' . gettype($object) . ' given.', 1237301369);
         }
         if ($object instanceof \stdClass) {
-            $declaredPropertyNames = array_keys(get_object_vars($object));
+            $declaredPropertyNames = array_keys((array)$object);
         } else {
             $declaredPropertyNames = array_keys(get_class_vars(get_class($object)));
         }
@@ -275,7 +266,7 @@ class ObjectAccess
             throw new \InvalidArgumentException('$object must be an object, ' . gettype($object) . ' given.', 1264022994);
         }
         if ($object instanceof \stdClass) {
-            $declaredPropertyNames = array_keys(get_object_vars($object));
+            $declaredPropertyNames = array_keys((array)$object);
         } else {
             $declaredPropertyNames = array_keys(get_class_vars(get_class($object)));
         }
@@ -325,11 +316,9 @@ class ObjectAccess
         if (!is_object($object)) {
             throw new \InvalidArgumentException('$object must be an object, ' . gettype($object) . ' given.', 1259828921);
         }
-        if ($object instanceof \ArrayAccess && isset($object[$propertyName]) === true) {
+        if ($object instanceof \ArrayAccess && isset($object[$propertyName])) {
             return true;
-        } elseif ($object instanceof \stdClass && array_search($propertyName, array_keys(get_object_vars($object))) !== false) {
-            return true;
-        } elseif ($object instanceof \ArrayAccess && isset($object[$propertyName]) === true) {
+        } elseif ($object instanceof \stdClass && isset($object->$propertyName)) {
             return true;
         }
         if (is_callable([$object, 'get' . ucfirst($propertyName)])) {
@@ -341,7 +330,11 @@ class ObjectAccess
         if (is_callable([$object, 'is' . ucfirst($propertyName)])) {
             return true;
         }
-        return array_search($propertyName, array_keys(get_class_vars(get_class($object)))) !== false;
+        if (property_exists($object, $propertyName)) {
+            $propertyReflection = new PropertyReflection($object, $propertyName);
+            return $propertyReflection->isPublic();
+        }
+        return false;
     }
 
     /**
@@ -361,11 +354,7 @@ class ObjectAccess
         }
         $properties = [];
         foreach (self::getGettablePropertyNames($object) as $propertyName) {
-            $propertyExists = false;
-            $propertyValue = self::getPropertyInternal($object, $propertyName, false, $propertyExists);
-            if ($propertyExists === true) {
-                $properties[$propertyName] = $propertyValue;
-            }
+            $properties[$propertyName] = self::getPropertyInternal($object, $propertyName);
         }
         return $properties;
     }
index 20f049b..875ee74 100644 (file)
@@ -70,31 +70,30 @@ class ObjectAccessTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     /**
      * @test
      */
-    public function getPropertyReturnsPropertyNotAccessibleExceptionForNotExistingPropertyIfForceDirectAccessIsTrue()
+    public function getPropertyThrowsPropertyNotAccessibleExceptionForNotExistingPropertyIfForceDirectAccessIsTrue()
     {
         $this->expectException(PropertyNotAccessibleException::class);
         $this->expectExceptionCode(1302855001);
-        $property = \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty($this->dummyObject, 'notExistingProperty', true);
+        \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty($this->dummyObject, 'notExistingProperty', true);
     }
 
     /**
      * @test
      */
-    public function getPropertyReturnsThrowsExceptionIfPropertyDoesNotExist()
+    public function getPropertyThrowsExceptionIfPropertyDoesNotExist()
     {
         $this->expectException(PropertyNotAccessibleException::class);
-        $this->expectExceptionCode(1263391473);
+        $this->expectExceptionCode(1302855001);
         \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty($this->dummyObject, 'notExistingProperty');
     }
 
     /**
      * @test
      */
-    public function getPropertyReturnsThrowsExceptionIfArrayKeyDoesNotExist()
+    public function getPropertyReturnsNullIfArrayKeyDoesNotExist()
     {
-        $this->expectException(PropertyNotAccessibleException::class);
-        $this->expectExceptionCode(1263391473);
-        \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty([], 'notExistingProperty');
+        $result = \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty([], 'notExistingProperty');
+        $this->assertNull($result);
     }
 
     /**
@@ -199,9 +198,10 @@ class ObjectAccessTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     public function getPropertyCanAccessPropertiesOfAnObjectStorageObject()
     {
         $objectStorage = new \TYPO3\CMS\Extbase\Persistence\ObjectStorage();
-        $objectStorage->key = 'value';
-        $actual = \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty($objectStorage, 'key');
-        $this->assertEquals('value', $actual, 'getProperty does not work with ObjectStorage property.');
+        $object = new \stdClass();
+        $objectStorage->attach($object);
+        $actual = \TYPO3\CMS\Extbase\Reflection\ObjectAccess::getProperty($objectStorage, 0);
+        $this->assertSame($object, $actual, 'getProperty does not work with ObjectStorage property.');
     }
 
     /**
@@ -375,17 +375,29 @@ class ObjectAccessTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     }
 
     /**
+     * @dataProvider propertyGettableTestValues
      * @test
+     * @param string $property
+     * @param bool $expected
      */
-    public function isPropertyGettableTellsIfAPropertyCanBeRetrieved()
+    public function isPropertyGettableTellsIfAPropertyCanBeRetrieved($property, $expected)
     {
-        $this->assertTrue(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'publicProperty'));
-        $this->assertTrue(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'property'));
-        $this->assertTrue(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'booleanProperty'));
-        $this->assertFalse(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'privateProperty'));
-        $this->assertFalse(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'writeOnlyMagicProperty'));
-        $this->assertFalse(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'shouldNotBePickedUp'));
-        $this->assertTrue(\TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, 'anotherBooleanProperty'));
+        $this->assertEquals($expected, \TYPO3\CMS\Extbase\Reflection\ObjectAccess::isPropertyGettable($this->dummyObject, $property));
+    }
+
+    /**
+     * @return array
+     */
+    public function propertyGettableTestValues()
+    {
+        return [
+            ['publicProperty', true],
+            ['property', true],
+            ['booleanProperty', true],
+            ['anotherBooleanProperty', true],
+            ['privateProperty', false],
+            ['writeOnlyMagicProperty', false]
+        ];
     }
 
     /**