Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge release 2.10.1 into 2.11.x #654

Merged
merged 16 commits into from
Dec 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d1ccf94
Fix proxying an interface and using a dynamic property on the proxy
nicolas-grekas Jun 25, 2018
076b9e6
Refactored #642 test to reduce it to the actual bug
Ocramius Dec 30, 2020
8875150
Refactored #642 to forward the original class to `PublicScopeSimulato…
Ocramius Dec 30, 2020
a23c464
Since #642 refactoring increased test coverage and mutation test cove…
Ocramius Dec 30, 2020
6a7251c
#642 imported symbol used in test (CS)
Ocramius Dec 30, 2020
098e306
Merge pull request #651 from Ocramius/fix/#642-remove-fatal-error-whe…
Ocramius Dec 30, 2020
7b57b2d
Fix "Only variable references should be returned by reference"
nicolas-grekas Dec 22, 2020
04ab867
#632 #645 #646 ensured that `unset()` code generated by `PublicScopeS…
Ocramius Dec 30, 2020
687bd41
#632 #645 #646 moved `PublicScopeSimulator` functional test to `tests…
Ocramius Dec 30, 2020
31196e5
#632 #645 #646 more precise type signature for `PublicScopeSimulator`…
Ocramius Dec 30, 2020
76cb45d
#632 #645 #646 applied automated CS fixes via phpcbf (mostly docblock…
Ocramius Dec 30, 2020
4b9ecae
#632 #645 #646 refactored `PublicScopeSimulator` to reject `$valuePar…
Ocramius Dec 30, 2020
0590fc9
#632 #645 #646 since tests improved again, raising mutation score thr…
Ocramius Dec 30, 2020
133fb21
Merge pull request #652 from Ocramius/fix/#646-#632-#645-only-variabl…
Ocramius Dec 30, 2020
4c93721
Fix lazy-loading-value-holder-internal-php-classes.phpt
nicolas-grekas Dec 28, 2020
8e85357
Merge branch 'fix/#649-correct-compress-parameter-used-in-internal-cl…
Ocramius Dec 30, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion infection.json.dist
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@
"branch": "master"
}
},
"minMsi": 84,
"minMsi": 84.5,
"minCoveredMsi": 86
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ public function __construct(
'name',
null,
null,
'returnValue'
'returnValue',
$originalClass
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ public function __construct(
'name',
null,
null,
'returnValue'
'returnValue',
$originalClass
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ public function __construct(
'name',
'value',
null,
'returnValue'
'returnValue',
$originalClass
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ public function __construct(
'name',
null,
null,
'returnValue'
'returnValue',
$originalClass
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ public function __construct(
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
PublicScopeSimulator::OPERATION_GET,
'name',
'value',
null,
$valueHolder,
'returnValue'
'returnValue',
$originalClass
);

if (! $publicProperties->isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ public function __construct(
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
PublicScopeSimulator::OPERATION_ISSET,
'name',
'value',
null,
$valueHolder,
'returnValue'
'returnValue',
$originalClass
);

if (! $publicProperties->isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ public function __construct(
'name',
'value',
$valueHolder,
'returnValue'
'returnValue',
$originalClass
);

if (! $publicProperties->isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ public function __construct(
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
PublicScopeSimulator::OPERATION_UNSET,
'name',
'value',
null,
$valueHolder,
'returnValue'
'returnValue',
$originalClass
);

if (! $publicProperties->isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ public function __construct(
PublicScopeSimulator::OPERATION_GET,
'name',
null,
$valueHolderProperty
$valueHolderProperty,
null,
$originalClass
)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ public function __construct(
PublicScopeSimulator::OPERATION_ISSET,
'name',
null,
$valueHolderProperty
$valueHolderProperty,
null,
$originalClass
);

$this->setBody(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ public function __construct(
PublicScopeSimulator::OPERATION_SET,
'name',
'value',
$valueHolderProperty
$valueHolderProperty,
null,
$originalClass
);

$this->setBody(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ public function __construct(
PublicScopeSimulator::OPERATION_UNSET,
'name',
null,
$valueHolderProperty
$valueHolderProperty,
null,
$originalClass
);

$this->setBody(
Expand Down
87 changes: 66 additions & 21 deletions src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@

use InvalidArgumentException;
use Laminas\Code\Generator\PropertyGenerator;
use ReflectionClass;

use function array_filter;
use function array_map;
use function implode;
use function sprintf;
use function var_export;

/**
* Generates code necessary to simulate a fatal error in case of unauthorized
Expand All @@ -28,50 +33,68 @@ class PublicScopeSimulator
*
* @param string $operationType operation to execute: one of 'get', 'set', 'isset' or 'unset'
* @param string $nameParameter name of the `name` parameter of the magic method
* @param string|null $valueParameter name of the `value` parameter of the magic method
* @param string|null $valueParameter name of the `value` parameter of the magic method, only to be
* used with $operationType 'set'
* @param PropertyGenerator $valueHolder name of the property containing the target object from which
* 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
*
* @psalm-param $operationType self::OPERATION_*
*/
public static function getPublicAccessSimulationCode(
string $operationType,
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' : '';
$target = '$this';

if ($valueHolder) {
$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) . ')';

$accessorEvaluation = $returnPropertyName
? '$' . $returnPropertyName . ' = ' . $byRef . '$accessor();'
: '$returnValue = ' . $byRef . '$accessor();' . "\n\n" . 'return $returnValue;';

if ($operationType === self::OPERATION_UNSET) {
$accessorEvaluation = '$accessor();';
}

return '$realInstanceReflection = ' . $originalClassReflection . ';' . "\n\n"
. 'if (! $realInstanceReflection->hasProperty($' . $nameParameter . ')) {' . "\n"
. ' $targetObject = ' . $target . ';' . "\n\n"
. self::getUndefinedPropertyNotice($operationType, $nameParameter)
. ' ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
. " return;\n"
. '}' . "\n\n"
. '$targetObject = ' . self::getTargetObject($valueHolder) . ";\n"
. '$accessor = function ' . $byRef . '() use ($targetObject, $name' . $value . ') {' . "\n"
. '$accessor = function ' . $byRef . '() use ('
. implode(', ', array_map(
static fn (string $parameterName): string => '$' . $parameterName,
array_filter(['targetObject', $nameParameter, $valueParameter])
))
. ') {' . "\n"
. ' ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
. "};\n"
. self::getScopeReBind()
. (
$returnPropertyName
? '$' . $returnPropertyName . ' = ' . $byRef . '$accessor();'
: '$returnValue = ' . $byRef . '$accessor();' . "\n\n" . 'return $returnValue;'
);
. self::generateScopeReBind()
. $accessorEvaluation;
}

/**
* This will generate code that triggers a notice if access is attempted on a non-existing property
*
* @psalm-param $operationType self::OPERATION_*
*/
private static function getUndefinedPropertyNotice(string $operationType, string $nameParameter): string
{
Expand All @@ -83,7 +106,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"
Expand All @@ -98,6 +121,8 @@ private static function getUndefinedPropertyNotice(string $operationType, string
* Note: if the object is a wrapper, the wrapped instance is accessed directly. If the object
* is a ghost or the proxy has no wrapper, then an instance of the parent class is created via
* on-the-fly unserialization
*
* @psalm-param $operationType self::OPERATION_*
*/
private static function getByRefReturnValue(string $operationType): string
{
Expand All @@ -118,25 +143,43 @@ private static function getTargetObject(?PropertyGenerator $valueHolder = null):

/**
* @throws InvalidArgumentException
*
* @psalm-param $operationType self::OPERATION_*
*/
private static function getOperation(string $operationType, string $nameParameter, ?string $valueParameter): string
{
if ($valueParameter !== null && $operationType !== self::OPERATION_SET) {
throw new InvalidArgumentException(
'Parameter $valueParameter should be provided (only) when $operationType === "' . self::OPERATION_SET . '"'
. self::class
. '::OPERATION_SET'
);
}

switch ($operationType) {
case self::OPERATION_GET:
return 'return $targetObject->$' . $nameParameter . ';';

case self::OPERATION_SET:
if ($valueParameter === null) {
throw new InvalidArgumentException('Parameter $valueParameter not provided');
throw new InvalidArgumentException(
'Parameter $valueParameter should be provided (only) when $operationType === "' . self::OPERATION_SET . '"'
. self::class
. '::OPERATION_SET'
);
}

return 'return $targetObject->$' . $nameParameter . ' = $' . $valueParameter . ';';
return '$targetObject->$' . $nameParameter . ' = $' . $valueParameter . ';'
. "\n\n"
. ' return $targetObject->$' . $nameParameter . ';';

case self::OPERATION_ISSET:
return 'return isset($targetObject->$' . $nameParameter . ');';

case self::OPERATION_UNSET:
return 'unset($targetObject->$' . $nameParameter . ');';
return 'unset($targetObject->$' . $nameParameter . ');'
. "\n\n"
. ' return;';
}

throw new InvalidArgumentException(sprintf('Invalid operation "%s" provided', $operationType));
Expand All @@ -145,11 +188,13 @@ private static function getOperation(string $operationType, string $nameParamete
/**
* Generates code to bind operations to the parent scope
*/
private static function getScopeReBind(): string
private static function generateScopeReBind(): string
{
return '$backtrace = debug_backtrace(true, 2);' . "\n"
. '$scopeObject = isset($backtrace[1][\'object\'])'
. ' ? $backtrace[1][\'object\'] : new \ProxyManager\Stub\EmptyClassStub();' . "\n"
. '$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));' . "\n";
return <<<'PHP'
$backtrace = debug_backtrace(true, 2);
$scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));

PHP;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

declare(strict_types=1);

namespace ProxyManagerTest\ProxyGenerator\Functional;

use PHPUnit\Framework\TestCase;
use ProxyManager\ProxyGenerator\Util\PublicScopeSimulator;
use ProxyManagerTestAsset\ClassWithMixedProperties;

use function sprintf;

/**
* @covers \ProxyManager\ProxyGenerator\Util\PublicScopeSimulator
* @coversNothing
*/
final class PublicScopeSimulatorFunctionalTest extends TestCase
{
/**
* @group #632
* @group #645
* @group #646
*/
public function testAccessingUndefinedPropertiesDoesNotLeadToInvalidByRefAccess(): void
{
/** @psalm-var ClassWithMixedProperties $sut */
$sut = eval(sprintf(
<<<'PHP'
return new class() extends %s {
public function doGet($prop) : string { %s }
public function doSet($prop, $val) : string { %s }
public function doIsset($prop) : bool { %s }
public function doUnset($prop) : void { %s }
};
PHP
,
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')
));

self::assertSame('publicProperty0', $sut->doGet('publicProperty0'));
self::assertSame('bar', $sut->doSet('publicProperty0', 'bar'));
self::assertTrue($sut->doIsset('publicProperty0'));
self::assertNull($sut->doUnset('publicProperty0'));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Loading