Fix "Only variable references should be returned by reference"

Author: nicolas-grekas

src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php

@@ -48,7 +48,7 @@ public static function getPublicAccessSimulationCode(
         ?ReflectionClass $originalClass = null
     ): string {
         $byRef  = self::getByRefReturnValue($operationType);
-        $value  = $operationType === self::OPERATION_SET ? ', $value' : '';
+        $value  = $operationType === self::OPERATION_SET ? ', $' . ($valueParameter ?? 'value') : '';
         $target = '$this';
 
         if ($valueHolder) {
@@ -67,7 +67,7 @@ public static function getPublicAccessSimulationCode(
             . "    return;\n"
             . '}' . "\n\n"
             . '$targetObject = ' . self::getTargetObject($valueHolder) . ";\n"
-            . '$accessor = function ' . $byRef . '() use ($targetObject, $name' . $value . ') {' . "\n"
+            . '$accessor = function ' . $byRef . '() use ($targetObject, $' . $nameParameter . $value . ') {' . "\n"
             . '    ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
             . "};\n"
             . self::getScopeReBind()
@@ -138,7 +138,7 @@ private static function getOperation(string $operationType, string $nameParamete
                     throw new InvalidArgumentException('Parameter $valueParameter not provided');
                 }
 
-                return 'return $targetObject->$' . $nameParameter . ' = $' . $valueParameter . ';';
+                return '$targetObject->$' . $nameParameter . ' = $' . $valueParameter . '; return $targetObject->$' . $nameParameter . ';';
 
             case self::OPERATION_ISSET:
                 return 'return isset($targetObject->$' . $nameParameter . ');';

tests/ProxyManagerTest/ProxyGenerator/Util/PublicScopeSimulatorTest.php

@@ -9,8 +9,11 @@
 use Laminas\Code\Generator\PropertyGenerator;
 use PHPUnit\Framework\TestCase;
 use ProxyManager\ProxyGenerator\Util\PublicScopeSimulator;
+use ProxyManagerTestAsset\ClassWithMixedProperties;
 use ReflectionClass;
 
+use function sprintf;
+
 /**
  * Tests for {@see \ProxyManager\ProxyGenerator\Util\PublicScopeSimulator}
  *
@@ -43,7 +46,7 @@ public function testSimpleGet(): void
 }
 
 $targetObject = $realInstanceReflection->newInstanceWithoutConstructor();
-$accessor = function & () use ($targetObject, $name) {
+$accessor = function & () use ($targetObject, $foo) {
     return $targetObject->$foo;
 };
 $backtrace = debug_backtrace(true, 2);
@@ -72,13 +75,13 @@ public function testSimpleSet(): void
 if (! $realInstanceReflection->hasProperty($foo)) {
     $targetObject = $this;
 
-    return $targetObject->$foo = $baz;
+    $targetObject->$foo = $baz; return $targetObject->$foo;
     return;
 }
 
 $targetObject = $realInstanceReflection->newInstanceWithoutConstructor();
-$accessor = function & () use ($targetObject, $name, $value) {
-    return $targetObject->$foo = $baz;
+$accessor = function & () use ($targetObject, $foo, $baz) {
+    $targetObject->$foo = $baz; return $targetObject->$foo;
 };
 $backtrace = debug_backtrace(true, 2);
 $scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
@@ -111,7 +114,7 @@ public function testSimpleIsset(): void
 }
 
 $targetObject = $realInstanceReflection->newInstanceWithoutConstructor();
-$accessor = function () use ($targetObject, $name) {
+$accessor = function () use ($targetObject, $foo) {
     return isset($targetObject->$foo);
 };
 $backtrace = debug_backtrace(true, 2);
@@ -145,7 +148,7 @@ public function testSimpleUnset(): void
 }
 
 $targetObject = $realInstanceReflection->newInstanceWithoutConstructor();
-$accessor = function () use ($targetObject, $name) {
+$accessor = function () use ($targetObject, $foo) {
     unset($targetObject->$foo);
 };
 $backtrace = debug_backtrace(true, 2);
@@ -187,13 +190,13 @@ public function testDelegatesToValueHolderWhenAvailable(): void
 if (! $realInstanceReflection->hasProperty($foo)) {
     $targetObject = $this->valueHolder;
 
-    return $targetObject->$foo = $baz;
+    $targetObject->$foo = $baz; return $targetObject->$foo;
     return;
 }
 
 $targetObject = $this->valueHolder;
-$accessor = function & () use ($targetObject, $name, $value) {
-    return $targetObject->$foo = $baz;
+$accessor = function & () use ($targetObject, $foo, $baz) {
+    $targetObject->$foo = $baz; return $targetObject->$foo;
 };
 $backtrace = debug_backtrace(true, 2);
 $scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
@@ -244,7 +247,7 @@ public function testWillReturnDirectlyWithNoReturnParam(): void
 }
 
 $targetObject = $realInstanceReflection->newInstanceWithoutConstructor();
-$accessor = function & () use ($targetObject, $name) {
+$accessor = function & () use ($targetObject, $foo) {
     return $targetObject->$foo;
 };
 $backtrace = debug_backtrace(true, 2);
@@ -282,4 +285,35 @@ public function testWillNotAttemptToGetParentClassWhenReflectionClassIsGivenUpfr
             )
         );
     }
+
+    /**
+     * @group #632
+     * @group #645
+     * @group #646
+     */
+    public function testFunctional(): void
+    {
+        /** @psalm-var ClassWithMixedProperties $sut */
+        $sut = eval(
+            sprintf(
+                'return new class() extends %s
+                {
+                    public function doGet($prop) { %s }
+                    public function doSet($prop, $val) { %s }
+                    public function doIsset($prop) { %s }
+                    public function doUnset($prop) { %s }
+                };',
+                ClassWithMixedProperties::class,
+                PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_GET, 'prop'),
+                PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_SET, 'prop', 'val'),
+                PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_ISSET, 'prop'),
+                PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_UNSET, 'prop')
+            )
+        );
+
+        $this->assertSame('publicProperty0', $sut->doGet('publicProperty0'));
+        $this->assertSame('bar', $sut->doSet('publicProperty0', 'bar'));
+        $this->assertTrue($sut->doIsset('publicProperty0'));
+        $this->assertNull($sut->doUnset('publicProperty0'));
+    }
 }