Skip to content

Commit 4b9ecae

Browse files
OcramiusOcramius
committed
#632 #645 #646 refactored PublicScopeSimulator to reject $valueParameter when not generating __set code
While changing this may seem like a BC Break, pre-existing usages of `PublicScopeSimulator` when combining `$operationType = 'set'` and `$valueParameter !== null` was most likely a bug that did not surface in consumer code yet. This change highlights the bug, and leads to generating cleaner code for `__set`, as well as correcting `public function __unset() : void` codegen, which previously generated invalid code that contained `return <expression>;` statements (crashing since PHP 7.3.0).
1 parent 76cb45d commit 4b9ecae

File tree

6 files changed

+97
-27
lines changed

6 files changed

+97
-27
lines changed

‎src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public function __construct(
3939
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
4040
PublicScopeSimulator::OPERATION_GET,
4141
'name',
42-
'value',
42+
null,
4343
$valueHolder,
4444
'returnValue',
4545
$originalClass

‎src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public function __construct(
3939
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
4040
PublicScopeSimulator::OPERATION_ISSET,
4141
'name',
42-
'value',
42+
null,
4343
$valueHolder,
4444
'returnValue',
4545
$originalClass

‎src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public function __construct(
3939
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
4040
PublicScopeSimulator::OPERATION_UNSET,
4141
'name',
42-
'value',
42+
null,
4343
$valueHolder,
4444
'returnValue',
4545
$originalClass

‎src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php‎

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
use Laminas\Code\Generator\PropertyGenerator;
99
use ReflectionClass;
1010

11+
use function array_filter;
12+
use function array_map;
13+
use function implode;
1114
use function sprintf;
1215
use function var_export;
1316

@@ -30,7 +33,8 @@ class PublicScopeSimulator
3033
*
3134
* @param string $operationType operation to execute: one of 'get', 'set', 'isset' or 'unset'
3235
* @param string $nameParameter name of the `name` parameter of the magic method
33-
* @param string|null $valueParameter name of the `value` parameter of the magic method
36+
* @param string|null $valueParameter name of the `value` parameter of the magic method, only to be
37+
* used with $operationType 'set'
3438
* @param PropertyGenerator $valueHolder name of the property containing the target object from which
3539
* to read the property. `$this` if none provided
3640
* @param string|null $returnPropertyName name of the property to which we want to assign the result of
@@ -50,7 +54,6 @@ public static function getPublicAccessSimulationCode(
5054
?ReflectionClass $originalClass = null
5155
): string {
5256
$byRef = self::getByRefReturnValue($operationType);
53-
$value = $operationType === self::OPERATION_SET ? ', $' . ($valueParameter ?? 'value') : '';
5457
$target = '$this';
5558

5659
if ($valueHolder) {
@@ -61,7 +64,13 @@ public static function getPublicAccessSimulationCode(
6164
? 'new \\ReflectionClass(get_parent_class($this))'
6265
: 'new \\ReflectionClass(' . var_export($originalClass->getName(), true) . ')';
6366

64-
$returnPropertyName ??= ($operationType === self::OPERATION_UNSET ? 'unset' : $returnPropertyName);
67+
$accessorEvaluation = $returnPropertyName
68+
? '$' . $returnPropertyName . ' = ' . $byRef . '$accessor();'
69+
: '$returnValue = ' . $byRef . '$accessor();' . "\n\n" . 'return $returnValue;';
70+
71+
if ($operationType === self::OPERATION_UNSET) {
72+
$accessorEvaluation = '$accessor();';
73+
}
6574

6675
return '$realInstanceReflection = ' . $originalClassReflection . ';' . "\n\n"
6776
. 'if (! $realInstanceReflection->hasProperty($' . $nameParameter . ')) {' . "\n"
@@ -70,19 +79,22 @@ public static function getPublicAccessSimulationCode(
7079
. ' ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
7180
. '}' . "\n\n"
7281
. '$targetObject = ' . self::getTargetObject($valueHolder) . ";\n"
73-
. '$accessor = function ' . $byRef . '() use ($targetObject, $' . $nameParameter . $value . ') {' . "\n"
82+
. '$accessor = function ' . $byRef . '() use ('
83+
. implode(', ', array_map(
84+
static fn (string $parameterName): string => '$' . $parameterName,
85+
array_filter(['targetObject', $nameParameter, $valueParameter])
86+
))
87+
. ') {' . "\n"
7488
. ' ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
7589
. "};\n"
76-
. self::getScopeReBind()
77-
. (
78-
$returnPropertyName
79-
? '$' . $returnPropertyName . ' = ' . $byRef . '$accessor();'
80-
: '$returnValue = ' . $byRef . '$accessor();' . "\n\n" . 'return $returnValue;'
81-
);
90+
. self::generateScopeReBind()
91+
. $accessorEvaluation;
8292
}
8393

8494
/**
8595
* This will generate code that triggers a notice if access is attempted on a non-existing property
96+
*
97+
* @psalm-param $operationType self::OPERATION_*
8698
*/
8799
private static function getUndefinedPropertyNotice(string $operationType, string $nameParameter): string
88100
{
@@ -109,6 +121,8 @@ private static function getUndefinedPropertyNotice(string $operationType, string
109121
* Note: if the object is a wrapper, the wrapped instance is accessed directly. If the object
110122
* is a ghost or the proxy has no wrapper, then an instance of the parent class is created via
111123
* on-the-fly unserialization
124+
*
125+
* @psalm-param $operationType self::OPERATION_*
112126
*/
113127
private static function getByRefReturnValue(string $operationType): string
114128
{
@@ -129,19 +143,35 @@ private static function getTargetObject(?PropertyGenerator $valueHolder = null):
129143

130144
/**
131145
* @throws InvalidArgumentException
146+
*
147+
* @psalm-param $operationType self::OPERATION_*
132148
*/
133149
private static function getOperation(string $operationType, string $nameParameter, ?string $valueParameter): string
134150
{
151+
if ($valueParameter !== null && $operationType !== self::OPERATION_SET) {
152+
throw new InvalidArgumentException(
153+
'Parameter $valueParameter should be provided (only) when $operationType === "' . self::OPERATION_SET . '"'
154+
. self::class
155+
. '::OPERATION_SET'
156+
);
157+
}
158+
135159
switch ($operationType) {
136160
case self::OPERATION_GET:
137161
return 'return $targetObject->$' . $nameParameter . ';';
138162

139163
case self::OPERATION_SET:
140164
if ($valueParameter === null) {
141-
throw new InvalidArgumentException('Parameter $valueParameter not provided');
165+
throw new InvalidArgumentException(
166+
'Parameter $valueParameter should be provided (only) when $operationType === "' . self::OPERATION_SET . '"'
167+
. self::class
168+
. '::OPERATION_SET'
169+
);
142170
}
143171

144-
return '$targetObject->$' . $nameParameter . ' = $' . $valueParameter . '; return $targetObject->$' . $nameParameter . ';';
172+
return '$targetObject->$' . $nameParameter . ' = $' . $valueParameter . ';'
173+
. "\n\n"
174+
. ' return $targetObject->$' . $nameParameter . ';';
145175

146176
case self::OPERATION_ISSET:
147177
return 'return isset($targetObject->$' . $nameParameter . ');';
@@ -158,11 +188,13 @@ private static function getOperation(string $operationType, string $nameParamete
158188
/**
159189
* Generates code to bind operations to the parent scope
160190
*/
161-
private static function getScopeReBind(): string
191+
private static function generateScopeReBind(): string
162192
{
163-
return '$backtrace = debug_backtrace(true, 2);' . "\n"
164-
. '$scopeObject = isset($backtrace[1][\'object\'])'
165-
. ' ? $backtrace[1][\'object\'] : new \ProxyManager\Stub\EmptyClassStub();' . "\n"
166-
. '$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));' . "\n";
193+
return <<<'PHP'
194+
$backtrace = debug_backtrace(true, 2);
195+
$scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
196+
$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));
197+
198+
PHP;
167199
}
168200
}

‎tests/ProxyManagerTest/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnsetTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public function testBodyStructure(): void
3939

4040
self::assertSame('__unset', $magicGet->getName());
4141
self::assertCount(1, $magicGet->getParameters());
42-
self::assertStringMatchesFormat('%a$returnValue = $accessor();%a', $magicGet->getBody());
42+
self::assertStringMatchesFormat('%a$accessor();%a', $magicGet->getBody());
4343
}
4444

4545
/**

‎tests/ProxyManagerTest/ProxyGenerator/Util/PublicScopeSimulatorTest.php‎

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
use Laminas\Code\Generator\PropertyGenerator;
1010
use PHPUnit\Framework\TestCase;
1111
use ProxyManager\ProxyGenerator\Util\PublicScopeSimulator;
12-
use ProxyManagerTestAsset\ClassWithMixedProperties;
1312
use ReflectionClass;
1413

1514
use function sprintf;
@@ -66,6 +65,20 @@ public function testSimpleGet(): void
6665
);
6766
}
6867

68+
public function testValueParameterNameIgnoredForSetOperation(): void
69+
{
70+
$this->expectException(InvalidArgumentException::class);
71+
$this->expectExceptionMessage('Parameter $valueParameter should be provided (only) when $operationType === "set"');
72+
73+
PublicScopeSimulator::getPublicAccessSimulationCode(
74+
PublicScopeSimulator::OPERATION_GET,
75+
'foo',
76+
'givenValue',
77+
null,
78+
'bar'
79+
);
80+
}
81+
6982
public function testSimpleSet(): void
7083
{
7184
$expected = <<<'PHP'
@@ -74,12 +87,16 @@ public function testSimpleSet(): void
7487
if (! $realInstanceReflection->hasProperty($foo)) {
7588
$targetObject = $this;
7689
77-
$targetObject->$foo = $baz; return $targetObject->$foo;
90+
$targetObject->$foo = $baz;
91+
92+
return $targetObject->$foo;
7893
}
7994
8095
$targetObject = $realInstanceReflection->newInstanceWithoutConstructor();
8196
$accessor = function & () use ($targetObject, $foo, $baz) {
82-
$targetObject->$foo = $baz; return $targetObject->$foo;
97+
$targetObject->$foo = $baz;
98+
99+
return $targetObject->$foo;
83100
};
84101
$backtrace = debug_backtrace(true, 2);
85102
$scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
@@ -154,7 +171,7 @@ public function testSimpleUnset(): void
154171
$backtrace = debug_backtrace(true, 2);
155172
$scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
156173
$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));
157-
$bar = $accessor();
174+
$accessor();
158175
PHP;
159176

160177
self::assertSame(
@@ -178,6 +195,23 @@ public function testUnsetCodeWillNotProduceReturnValueStatements(): void
178195
{
179196
self::assertStringNotContainsString(
180197
'return ',
198+
PublicScopeSimulator::getPublicAccessSimulationCode(
199+
PublicScopeSimulator::OPERATION_UNSET,
200+
'foo'
201+
),
202+
'Generated return statements do not contain value expressions (invalid since PHP 7.3+)'
203+
);
204+
}
205+
206+
/**
207+
* @group #632
208+
* @group #645
209+
* @group #646
210+
*/
211+
public function testUnsetCodeWillNotAssignAccessorEvaluationToVariable(): void
212+
{
213+
self::assertStringEndsWith(
214+
"\n" . '$accessor();',
181215
PublicScopeSimulator::getPublicAccessSimulationCode(
182216
PublicScopeSimulator::OPERATION_UNSET,
183217
'foo'
@@ -206,12 +240,16 @@ public function testDelegatesToValueHolderWhenAvailable(): void
206240
if (! $realInstanceReflection->hasProperty($foo)) {
207241
$targetObject = $this->valueHolder;
208242
209-
$targetObject->$foo = $baz; return $targetObject->$foo;
243+
$targetObject->$foo = $baz;
244+
245+
return $targetObject->$foo;
210246
}
211247
212248
$targetObject = $this->valueHolder;
213249
$accessor = function & () use ($targetObject, $foo, $baz) {
214-
$targetObject->$foo = $baz; return $targetObject->$foo;
250+
$targetObject->$foo = $baz;
251+
252+
return $targetObject->$foo;
215253
};
216254
$backtrace = debug_backtrace(true, 2);
217255
$scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();

0 commit comments

Comments
 (0)