Merge pull request #651 from Ocramius/fix/#642-remove-fatal-error-when-accessing-dynamic-property-of-proxied-interface

Ocramius · web-flow · commit 098e306e6796 · 2020-12-30T13:12:34.000+01:00
#642 fix fatal error when accessing a dynamic property of a proxied interface (refactored)
diff --git a/infection.json.dist b/infection.json.dist
@@ -10,6 +10,6 @@
             "branch": "master"
         }
     },
-    "minMsi": 84,
+    "minMsi": 84.3,
     "minCoveredMsi": 86
 }
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGet.php
@@ -38,7 +38,8 @@ public function __construct(
                 'name',
                 null,
                 null,
-                'returnValue'
+                'returnValue',
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicIsset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicIsset.php
@@ -38,7 +38,8 @@ public function __construct(
                 'name',
                 null,
                 null,
-                'returnValue'
+                'returnValue',
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicSet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicSet.php
@@ -42,7 +42,8 @@ public function __construct(
                 'name',
                 'value',
                 null,
-                'returnValue'
+                'returnValue',
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnset.php
@@ -38,7 +38,8 @@ public function __construct(
                 'name',
                 null,
                 null,
-                'returnValue'
+                'returnValue',
+                $originalClass
             );
         }
 
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php
@@ -41,7 +41,8 @@ public function __construct(
             'name',
             'value',
             $valueHolder,
-            'returnValue'
+            'returnValue',
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php
@@ -41,7 +41,8 @@ public function __construct(
             'name',
             'value',
             $valueHolder,
-            'returnValue'
+            'returnValue',
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicSet.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicSet.php
@@ -45,7 +45,8 @@ public function __construct(
             'name',
             'value',
             $valueHolder,
-            'returnValue'
+            'returnValue',
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php b/src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php
@@ -41,7 +41,8 @@ public function __construct(
             'name',
             'value',
             $valueHolder,
-            'returnValue'
+            'returnValue',
+            $originalClass
         );
 
         if (! $publicProperties->isEmpty()) {
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicGet.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicGet.php
@@ -55,7 +55,9 @@ public function __construct(
                 PublicScopeSimulator::OPERATION_GET,
                 'name',
                 null,
-                $valueHolderProperty
+                $valueHolderProperty,
+                null,
+                $originalClass
             )
         );
     }
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicIsset.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicIsset.php
@@ -44,7 +44,9 @@ public function __construct(
             PublicScopeSimulator::OPERATION_ISSET,
             'name',
             null,
-            $valueHolderProperty
+            $valueHolderProperty,
+            null,
+            $originalClass
         );
 
         $this->setBody(
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicSet.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicSet.php
@@ -51,7 +51,9 @@ public function __construct(
                 PublicScopeSimulator::OPERATION_SET,
                 'name',
                 'value',
-                $valueHolderProperty
+                $valueHolderProperty,
+                null,
+                $originalClass
             );
 
         $this->setBody(
diff --git a/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicUnset.php b/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicUnset.php
@@ -47,7 +47,9 @@ public function __construct(
                 PublicScopeSimulator::OPERATION_UNSET,
                 'name',
                 null,
-                $valueHolderProperty
+                $valueHolderProperty,
+                null,
+                $originalClass
             );
 
         $this->setBody(
diff --git a/src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php b/src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php
@@ -6,8 +6,10 @@
 
 use InvalidArgumentException;
 use Laminas\Code\Generator\PropertyGenerator;
+use ReflectionClass;
 
 use function sprintf;
+use function var_export;
 
 /**
  * Generates code necessary to simulate a fatal error in case of unauthorized
@@ -33,6 +35,7 @@ class PublicScopeSimulator
      *                                              to read the property. `$this` if none provided
      * @param string|null       $returnPropertyName name of the property to which we want to assign the result of
      *                                              the operation. Return directly if none provided
+     * @param string|null       $interfaceName      name of the proxified interface if any
      *
      * @throws InvalidArgumentException
      */
@@ -41,7 +44,8 @@ public static function getPublicAccessSimulationCode(
         string $nameParameter,
         ?string $valueParameter = null,
         ?PropertyGenerator $valueHolder = null,
-        ?string $returnPropertyName = null
+        ?string $returnPropertyName = null,
+        ?ReflectionClass $originalClass = null
     ): string {
         $byRef  = self::getByRefReturnValue($operationType);
         $value  = $operationType === self::OPERATION_SET ? ', $value' : '';
@@ -51,7 +55,11 @@ public static function getPublicAccessSimulationCode(
             $target = '$this->' . $valueHolder->getName();
         }
 
-        return '$realInstanceReflection = new \\ReflectionClass(get_parent_class($this));' . "\n\n"
+        $originalClassReflection = $originalClass === null
+            ? 'new \\ReflectionClass(get_parent_class($this))'
+            : 'new \\ReflectionClass(' . var_export($originalClass->getName(), true) . ')';
+
+        return '$realInstanceReflection = ' . $originalClassReflection . ';' . "\n\n"
             . 'if (! $realInstanceReflection->hasProperty($' . $nameParameter . ')) {' . "\n"
             . '    $targetObject = ' . $target . ';' . "\n\n"
             . self::getUndefinedPropertyNotice($operationType, $nameParameter)
@@ -83,7 +91,7 @@ private static function getUndefinedPropertyNotice(string $operationType, string
             . '    trigger_error(' . "\n"
             . '        sprintf(' . "\n"
             . '            \'Undefined property: %s::$%s in %s on line %s\',' . "\n"
-            . '            get_parent_class($this),' . "\n"
+            . '            $realInstanceReflection->getName(),' . "\n"
             . '            $' . $nameParameter . ',' . "\n"
             . '            $backtrace[0][\'file\'],' . "\n"
             . '            $backtrace[0][\'line\']' . "\n"
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,10 +4,12 @@
 
 namespace ProxyManagerTest\ProxyGenerator\Util;
 
+use ArrayAccess;
 use InvalidArgumentException;
 use Laminas\Code\Generator\PropertyGenerator;
 use PHPUnit\Framework\TestCase;
 use ProxyManager\ProxyGenerator\Util\PublicScopeSimulator;
+use ReflectionClass;
 
 /**
  * Tests for {@see \ProxyManager\ProxyGenerator\Util\PublicScopeSimulator}
@@ -29,7 +31,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 +232,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 +263,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)
+            )
+        );
+    }
 }
diff --git a/tests/language-feature-scripts/lazy-loading-value-holder-denies-interface-property-read.phpt b/tests/language-feature-scripts/lazy-loading-value-holder-denies-interface-property-read.phpt
@@ -0,0 +1,27 @@
+--TEST--
+Verifies that lazy loading value holder proxies for interfaces disallow public property read attempts
+--FILE--
+<?php
+
+require_once __DIR__ . '/init.php';
+
+interface MyInterface
+{
+}
+
+$factory = new \ProxyManager\Factory\LazyLoadingValueHolderFactory($configuration);
+
+$proxy = $factory
+    ->createProxy(MyInterface::class, function (& $wrapped, $proxy, $method, array $parameters, & $initializer) : bool {
+        $initializer = null;
+        $wrapped     = new class implements MyInterface {
+        };
+
+        return true;
+    });
+
+echo $proxy->someDynamicProperty;
+
+?>
+--EXPECTF--
+Notice: Undefined property: MyInterface::$someDynamicProperty %s

Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,6 @@`
`10`	`10`	`"branch": "master"`
`11`	`11`	`}`
`12`	`12`	`},`
`13`		`- "minMsi": 84,`
	`13`	`+ "minMsi": 84.3,`
`14`	`14`	`"minCoveredMsi": 86`
`15`	`15`	`}`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,8 @@ public function __construct(`
`38`	`38`	`'name',`
`39`	`39`	`null,`
`40`	`40`	`null,`
`41`		`- 'returnValue'`
	`41`	`+ 'returnValue',`
	`42`	`+ $originalClass`
`42`	`43`	`);`
`43`	`44`	`}`
`44`	`45`
Original file line number	Diff line number	Diff line change
`@@ -42,7 +42,8 @@ public function __construct(`
`42`	`42`	`'name',`
`43`	`43`	`'value',`
`44`	`44`	`null,`
`45`		`- 'returnValue'`
	`45`	`+ 'returnValue',`
	`46`	`+ $originalClass`
`46`	`47`	`);`
`47`	`48`	`}`
`48`	`49`
Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,9 @@ public function __construct(`
`55`	`55`	`PublicScopeSimulator::OPERATION_GET,`
`56`	`56`	`'name',`
`57`	`57`	`null,`
`58`		`- $valueHolderProperty`
	`58`	`+ $valueHolderProperty,`
	`59`	`+ null,`
	`60`	`+ $originalClass`
`59`	`61`	`)`
`60`	`62`	`);`
`61`	`63`	`}`