Refactored #642 to forward the original class to PublicScopeSimulator when generating notice emulation code

Ocramius · Ocramius · commit 88751506be7f · 2020-12-30T12:55:10.000+01:00
Before this, the decision whether an interface was being proxied was delegated to the magic method generator,
which led to an explosion in conditionals in upstream code. Here we instead pass the whole `ReflectionClass`
for the parent symbol to the `PublicScopeSimulator`, therefore adding a single conditional to the source of
`PublicScopeSimulator`, leading to reduced complexity.
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGet.php
@@ -39,7 +39,7 @@ public function __construct(
                 null,
                 null,
                 'returnValue',
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicIsset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicIsset.php
@@ -39,7 +39,7 @@ public function __construct(
                 null,
                 null,
                 'returnValue',
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicSet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicSet.php
@@ -43,7 +43,7 @@ public function __construct(
                 'value',
                 null,
                 'returnValue',
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnset.php
@@ -39,7 +39,7 @@ public function __construct(
                 null,
                 null,
                 'returnValue',
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php
@@ -42,7 +42,7 @@ public function __construct(
             'value',
             $valueHolder,
             'returnValue',
-            $originalClass->isInterface() ? $originalClass->getName() : null
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php
@@ -42,7 +42,7 @@ public function __construct(
             'value',
             $valueHolder,
             'returnValue',
-            $originalClass->isInterface() ? $originalClass->getName() : null
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicSet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicSet.php
@@ -46,7 +46,7 @@ public function __construct(
             'value',
             $valueHolder,
             'returnValue',
-            $originalClass->isInterface() ? $originalClass->getName() : null
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php
@@ -42,7 +42,7 @@ public function __construct(
             'value',
             $valueHolder,
             'returnValue',
-            $originalClass->isInterface() ? $originalClass->getName() : null
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicGet.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicGet.php
@@ -57,7 +57,7 @@ public function __construct(
                 null,
                 $valueHolderProperty,
                 null,
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             )
         );
     }
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicIsset.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicIsset.php
@@ -46,7 +46,7 @@ public function __construct(
             null,
             $valueHolderProperty,
             null,
-            $originalClass->isInterface() ? $originalClass->getName() : null
+            $originalClass
         );
 
         $this->setBody(
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicSet.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicSet.php
@@ -53,7 +53,7 @@ public function __construct(
                 'value',
                 $valueHolderProperty,
                 null,
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             );
 
         $this->setBody(
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicUnset.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicUnset.php
@@ -49,7 +49,7 @@ public function __construct(
                 null,
                 $valueHolderProperty,
                 null,
-                $originalClass->isInterface() ? $originalClass->getName() : null
+                $originalClass
             );
 
         $this->setBody(
diff --git a/src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php b/src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php
@@ -6,10 +6,9 @@
 
 use InvalidArgumentException;
 use Laminas\Code\Generator\PropertyGenerator;
+use ReflectionClass;
 
 use function sprintf;
-use function str_replace;
-use function substr;
 use function var_export;
 
 /**
@@ -46,7 +45,7 @@ public static function getPublicAccessSimulationCode(
         ?string $valueParameter = null,
         ?PropertyGenerator $valueHolder = null,
         ?string $returnPropertyName = null,
-        ?string $interfaceName = null
+        ?ReflectionClass $originalClass = null
     ): string {
         $byRef  = self::getByRefReturnValue($operationType);
         $value  = $operationType === self::OPERATION_SET ? ', $value' : '';
@@ -56,13 +55,11 @@ public static function getPublicAccessSimulationCode(
             $target = '$this->' . $valueHolder->getName();
         }
 
-        if ($interfaceName !== null) {
-            return '$targetObject = ' . $target . ';' . "\n\n"
-                . self::getUndefinedPropertyNotice($operationType, $nameParameter, $interfaceName)
-                . self::getOperation($operationType, $nameParameter, $valueParameter);
-        }
+        $originalClassReflection = $originalClass === null
+            ? 'new \\ReflectionClass(get_parent_class($this))'
+            : 'new \\ReflectionClass(' . var_export($originalClass->getName(), true) . ')';
 
-        return '$realInstanceReflection = new \\ReflectionClass(get_parent_class($this));' . "\n\n"
+        return '$realInstanceReflection = ' . $originalClassReflection . ';' . "\n\n"
             . 'if (! $realInstanceReflection->hasProperty($' . $nameParameter . ')) {' . "\n"
             . '    $targetObject = ' . $target . ';' . "\n\n"
             . self::getUndefinedPropertyNotice($operationType, $nameParameter)
@@ -84,29 +81,23 @@ public static function getPublicAccessSimulationCode(
     /**
      * This will generate code that triggers a notice if access is attempted on a non-existing property
      */
-    private static function getUndefinedPropertyNotice(string $operationType, string $nameParameter, ?string $interfaceName = null): string
+    private static function getUndefinedPropertyNotice(string $operationType, string $nameParameter): string
     {
         if ($operationType !== self::OPERATION_GET) {
             return '';
         }
 
-        $code = '    $backtrace = debug_backtrace(false, 1);' . "\n"
+        return '    $backtrace = debug_backtrace(false, 1);' . "\n"
             . '    trigger_error(' . "\n"
             . '        sprintf(' . "\n"
             . '            \'Undefined property: %s::$%s in %s on line %s\',' . "\n"
-            . '            ' . ($interfaceName !== null ? var_export($interfaceName, true) : 'get_parent_class($this)') . ',' . "\n"
+            . '            $realInstanceReflection->getName(),' . "\n"
             . '            $' . $nameParameter . ',' . "\n"
             . '            $backtrace[0][\'file\'],' . "\n"
             . '            $backtrace[0][\'line\']' . "\n"
             . '        ),' . "\n"
             . '        \E_USER_NOTICE' . "\n"
             . '    );' . "\n";
-
-        if ($interfaceName !== null) {
-            $code = str_replace("\n    ", "\n", substr($code, 4));
-        }
-
-        return $code;
     }
 
     /**
diff --git a/tests/ProxyManagerTest/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGetTest.php b/tests/ProxyManagerTest/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGetTest.php
@@ -16,12 +16,10 @@
  * Tests for {@see \ProxyManager\ProxyGenerator\AccessInterceptorScopeLocalizer\MethodGenerator\MagicGet}
  *
  * @group Coverage
+ * @covers \ProxyManager\ProxyGenerator\AccessInterceptorScopeLocalizer\MethodGenerator\MagicGet
  */
 final class MagicGetTest extends TestCase
 {
-    /**
-     * @covers \ProxyManager\ProxyGenerator\AccessInterceptorScopeLocalizer\MethodGenerator\MagicGet::__construct
-     */
     public function testBodyStructure(): void
     {
         $reflection         = new ReflectionClass(EmptyClass::class);
@@ -42,9 +40,6 @@ public function testBodyStructure(): void
         self::assertStringMatchesFormat('%a$returnValue = & $accessor();%a', $magicGet->getBody());
     }
 
-    /**
-     * @covers \ProxyManager\ProxyGenerator\AccessInterceptorScopeLocalizer\MethodGenerator\MagicGet::__construct
-     */
     public function testBodyStructureWithInheritedMethod(): void
     {
         $reflection         = new ReflectionClass(ClassWithMagicMethods::class);
diff --git a/tests/ProxyManagerTest/ProxyGenerator/Util/PublicScopeSimulatorTest.php b/tests/ProxyManagerTest/ProxyGenerator/Util/PublicScopeSimulatorTest.php
@@ -4,6 +4,7 @@
 
 namespace ProxyManagerTest\ProxyGenerator\Util;
 
+use ArrayAccess;
 use InvalidArgumentException;
 use Laminas\Code\Generator\PropertyGenerator;
 use PHPUnit\Framework\TestCase;
@@ -29,7 +30,7 @@ public function testSimpleGet(): void
     trigger_error(
         sprintf(
             'Undefined property: %s::$%s in %s on line %s',
-            get_parent_class($this),
+            $realInstanceReflection->getName(),
             $foo,
             $backtrace[0]['file'],
             $backtrace[0]['line']
@@ -230,7 +231,7 @@ public function testWillReturnDirectlyWithNoReturnParam(): void
     trigger_error(
         sprintf(
             'Undefined property: %s::$%s in %s on line %s',
-            get_parent_class($this),
+            $realInstanceReflection->getName(),
             $foo,
             $backtrace[0]['file'],
             $backtrace[0]['line']
@@ -261,4 +262,23 @@ public function testWillReturnDirectlyWithNoReturnParam(): void
             )
         );
     }
+
+    /** @group #642 */
+    public function testWillNotAttemptToGetParentClassWhenReflectionClassIsGivenUpfront(): void
+    {
+        self::assertStringStartsWith(
+            <<<'PHP'
+$realInstanceReflection = new \ReflectionClass('ArrayAccess');
+PHP
+            ,
+            PublicScopeSimulator::getPublicAccessSimulationCode(
+                PublicScopeSimulator::OPERATION_GET,
+                'foo',
+                null,
+                null,
+                null,
+                new \ReflectionClass(ArrayAccess::class)
+            )
+        );
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ public function __construct(`
`39`	`39`	`null,`
`40`	`40`	`null,`
`41`	`41`	`'returnValue',`
`42`		`- $originalClass->isInterface() ? $originalClass->getName() : null`
	`42`	`+ $originalClass`
`43`	`43`	`);`
`44`	`44`	`}`
`45`	`45`
Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ public function __construct(`
`43`	`43`	`'value',`
`44`	`44`	`null,`
`45`	`45`	`'returnValue',`
`46`		`- $originalClass->isInterface() ? $originalClass->getName() : null`
	`46`	`+ $originalClass`
`47`	`47`	`);`
`48`	`48`	`}`
`49`	`49`
Original file line number	Diff line number	Diff line change
`@@ -57,7 +57,7 @@ public function __construct(`
`57`	`57`	`null,`
`58`	`58`	`$valueHolderProperty,`
`59`	`59`	`null,`
`60`		`- $originalClass->isInterface() ? $originalClass->getName() : null`
	`60`	`+ $originalClass`
`61`	`61`	`)`
`62`	`62`	`);`
`63`	`63`	`}`