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.

Sign up for GitHub

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

[WIP] Fix #346 - magic methods hints need to be reflected in generated child classes #350

Closed
wants to merge 7 commits into from
Closed

[WIP] Fix #346 - magic methods hints need to be reflected in generated child classes #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ffa5901
#346 Test asset: class with heavily hinted magic methods
Ocramius Nov 21, 2016
2e3bac1
#346 Test asset: interface with heavily hinted magic methods
Ocramius Nov 21, 2016
d8e3e01
#346 Test: code generation should succeed also with heavily hinted ma…
Ocramius Nov 21, 2016
ce5aa9b
#346 modified `PublicScopeSimulator` to take code generation of retur…
Ocramius Nov 22, 2016
9063ae4
#346 `MagicMethodGenerator` now preserves parameter and return type h…
Ocramius Nov 22, 2016
acbc96f
#346 Adapting currently existing code to the `PublicScopeSimulator` c…
Ocramius Nov 22, 2016
900b70b
[BC BREAK] correcting static calls to `PublicScopeSimulator::getPubli…
Ocramius May 4, 2017
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
55 changes: 44 additions & 11 deletions src/ProxyManager/Generator/MagicMethodGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
namespace ProxyManager\Generator;

use ReflectionClass;
use Zend\Code\Generator\ParameterGenerator;
use Zend\Code\Reflection\ParameterReflection;

/**
* Method generator for magic methods
Expand All @@ -31,24 +33,55 @@
class MagicMethodGenerator extends MethodGenerator
{
/**
* @param ReflectionClass $originalClass
* @param string $name
* @param array $parameters
* @param ReflectionClass $originalClass
* @param string $name
* @param ParameterGenerator[] $parameters
*
* @throws \Zend\Code\Generator\Exception\InvalidArgumentException
*/
public function __construct(ReflectionClass $originalClass, string $name, array $parameters = [])
{
parent::__construct(
$name,
$parameters,
static::FLAG_PUBLIC,
null,
$originalClass->hasMethod($name) ? '{@inheritDoc}' : null
);
parent::__construct($name, [], static::FLAG_PUBLIC);

$this->setReturnsReference(strtolower($name) === '__get');

if ($originalClass->hasMethod($name)) {
$this->setReturnsReference($originalClass->getMethod($name)->returnsReference());
$originalMethod = $originalClass->getMethod($name);

$this->setDocBlock('{@inheritDoc}');
$this->setReturnsReference($originalMethod->returnsReference());
$this->setReturnType(((string) $originalMethod->getReturnType()) ?: null);
$this->addOrReplaceParameterNames($originalMethod, ...$parameters);
} else {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't like this else. Can probably just call the parent constructor with the given parameters, then replace

$this->setParameters($parameters);
}
}

private function addOrReplaceParameterNames(
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is way too complex: need to split into multiple private methods explaining exactly what is going on at each step.

\ReflectionMethod $originalMethod,
ParameterGenerator ...$newParameters
) : void {
$originalParameters = array_values(array_map(
function (\ReflectionParameter $parameter) use ($originalMethod) : ParameterGenerator {
return ParameterGenerator::fromReflection(new ParameterReflection(
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loads of coupling due to Zend\Code's shitty reflection API :-(

[$originalMethod->getDeclaringClass()->getName(), $originalMethod->getName()],
$parameter->getPosition()
));
},
$originalMethod->getParameters()
));

$parametersCount = max(count($originalParameters), count($newParameters));
$parameters = [];

for ($idx = 0; $idx < $parametersCount; $idx += 1) {
$parameter = $originalParameters[$idx] ?? $newParameters[$idx];

$parameter->setName(($newParameters[$idx] ?? $originalParameters[$idx])->getName());

$parameters[] = $parameter;
}

$this->setParameters($parameters);
}
}
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public function __construct(

if (! $parent) {
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_GET,
'name',
null,
Expand Down
1 change: 1 addition & 0 deletions ...roxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicIsset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public function __construct(

if (! $parent) {
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_ISSET,
'name',
null,
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public function __construct(

if (! $parent) {
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_SET,
'name',
'value',
Expand Down
1 change: 1 addition & 0 deletions ...roxyManager/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public function __construct(

if (! $parent) {
$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_UNSET,
'name',
null,
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public function __construct(
$this->setDocBlock(($parent ? "{@inheritDoc}\n" : '') . '@param string $name');

$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_GET,
'name',
'value',
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public function __construct(
$this->setDocBlock(($parent ? "{@inheritDoc}\n" : '') . '@param string $name');

$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_ISSET,
'name',
'value',
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public function __construct(
$this->setDocBlock(($parent ? "{@inheritDoc}\n" : '') . '@param string $name');

$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_SET,
'name',
'value',
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public function __construct(
$this->setDocBlock(($parent ? "{@inheritDoc}\n" : '') . '@param string $name');

$callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_UNSET,
'name',
'value',
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/LazyLoadingGhost/MethodGenerator/MagicGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ public function __construct(

if (! $override) {
$parentAccess = PublicScopeSimulator::getPublicAccessSimulationCode(
null,
PublicScopeSimulator::OPERATION_GET,
'name'
);
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/LazyLoadingGhost/MethodGenerator/MagicIsset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ public function __construct(

if (! $override) {
$parentAccess = PublicScopeSimulator::getPublicAccessSimulationCode(
null,
PublicScopeSimulator::OPERATION_ISSET,
'name'
);
Expand Down
1 change: 1 addition & 0 deletions src/ProxyManager/ProxyGenerator/LazyLoadingGhost/MethodGenerator/MagicSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ public function __construct(

if (! $override) {
$parentAccess = PublicScopeSimulator::getPublicAccessSimulationCode(
null,
PublicScopeSimulator::OPERATION_SET,
'name',
'value'
Expand Down
75 changes: 43 additions & 32 deletions src/ProxyManager/ProxyGenerator/LazyLoadingGhost/MethodGenerator/MagicUnset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
namespace ProxyManager\ProxyGenerator\LazyLoadingGhost\MethodGenerator;

use ProxyManager\Generator\MagicMethodGenerator;
use ProxyManager\Generator\Util\ProxiedMethodReturnExpression;
use Zend\Code\Generator\ParameterGenerator;
use ProxyManager\ProxyGenerator\LazyLoadingGhost\PropertyGenerator\PrivatePropertiesMap;
use ProxyManager\ProxyGenerator\LazyLoadingGhost\PropertyGenerator\ProtectedPropertiesMap;
Expand All @@ -42,67 +43,67 @@ class MagicUnset extends MagicMethodGenerator
* @var string
*/
private $callParentTemplate = <<<'PHP'
%s
%init

if (isset(self::$%s[$name])) {
if (isset(self::$%publicProperties[$name])) {
unset($this->$name);

return;
%voidReturn1
}

if (isset(self::$%s[$name])) {
if (isset(self::$%protectedProperties1[$name])) {
// check protected property access via compatible class
$callers = debug_backtrace(\DEBUG_BACKTRACE_PROVIDE_OBJECT, 2);
$caller = isset($callers[1]) ? $callers[1] : [];
$object = isset($caller['object']) ? $caller['object'] : '';
$expectedType = self::$%s[$name];
$expectedType = self::$%protectedProperties2[$name];

if ($object instanceof $expectedType) {
unset($this->$name);

return;
%voidReturn2
}

$class = isset($caller['class']) ? $caller['class'] : '';

if ($class === $expectedType || is_subclass_of($class, $expectedType) || $class === 'ReflectionProperty') {
unset($this->$name);

return;
%voidReturn3
}
} elseif (isset(self::$%s[$name])) {
} elseif (isset(self::$%privateProperties1[$name])) {
// check private property access via same class
$callers = debug_backtrace(\DEBUG_BACKTRACE_PROVIDE_OBJECT, 2);
$caller = isset($callers[1]) ? $callers[1] : [];
$class = isset($caller['class']) ? $caller['class'] : '';

static $accessorCache = [];

if (isset(self::$%s[$name][$class])) {
if (isset(self::$%privateProperties2[$name][$class])) {
$cacheKey = $class . '#' . $name;
$accessor = isset($accessorCache[$cacheKey])
? $accessorCache[$cacheKey]
: $accessorCache[$cacheKey] = \Closure::bind(function ($instance) use ($name) {
unset($instance->$name);
}, null, $class);

return $accessor($this);
%accessorReturn1
}

if ('ReflectionProperty' === $class) {
$tmpClass = key(self::$%s[$name]);
$tmpClass = key(self::$%privateProperties3[$name]);
$cacheKey = $tmpClass . '#' . $name;
$accessor = isset($accessorCache[$cacheKey])
? $accessorCache[$cacheKey]
: $accessorCache[$cacheKey] = \Closure::bind(function ($instance) use ($name) {
unset($instance->$name);
}, null, $tmpClass);

return $accessor($this);
%accessorReturn2
}
}

%s
%parentAccess
PHP;

/**
Expand All @@ -126,30 +127,40 @@ public function __construct(
) {
parent::__construct($originalClass, '__unset', [new ParameterGenerator('name')]);

$override = $originalClass->hasMethod('__unset');
$existingMethod = $originalClass->hasMethod('__unset') ? $originalClass->getMethod('__unset') : null;

$this->setDocBlock(($override ? "{@inheritDoc}\n" : '') . '@param string $name');
$this->setDocBlock(($existingMethod ? "{@inheritDoc}\n" : '') . '@param string $name');

$parentAccess = 'return parent::__unset($name);';

if (! $override) {
$parentAccess = PublicScopeSimulator::getPublicAccessSimulationCode(
$parentAccess = $existingMethod
? ProxiedMethodReturnExpression::generate('parent::__unset($name)', $existingMethod)
: PublicScopeSimulator::getPublicAccessSimulationCode(
$existingMethod,
PublicScopeSimulator::OPERATION_UNSET,
'name'
);
}

$this->setBody(sprintf(
$this->callParentTemplate,
'$this->' . $initializerProperty->getName() . ' && $this->' . $callInitializer->getName()
. '(\'__unset\', array(\'name\' => $name));',
$publicProperties->getName(),
$protectedProperties->getName(),
$protectedProperties->getName(),
$privateProperties->getName(),
$privateProperties->getName(),
$privateProperties->getName(),
$parentAccess

$symbols = [
'%init' => '$this->' . $initializerProperty->getName()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should hunt down all the sprintf() and replace them with this syntax: much easier to follow.

. ' && $this->' . $callInitializer->getName()
. '(\'__unset\', array(\'name\' => $name));',
'%publicProperties' => $publicProperties->getName(),
'%protectedProperties1' => $protectedProperties->getName(),
'%protectedProperties2' => $protectedProperties->getName(),
'%privateProperties1' => $privateProperties->getName(),
'%privateProperties2' => $privateProperties->getName(),
'%privateProperties3' => $privateProperties->getName(),
'%parentAccess' => $parentAccess,
'%voidReturn1' => ProxiedMethodReturnExpression::generate('true', $existingMethod),
'%voidReturn2' => ProxiedMethodReturnExpression::generate('true', $existingMethod),
'%voidReturn3' => ProxiedMethodReturnExpression::generate('true', $existingMethod),
'%accessorReturn1' => ProxiedMethodReturnExpression::generate('$accessor($this)', $existingMethod),
'%accessorReturn2' => ProxiedMethodReturnExpression::generate('$accessor($this)', $existingMethod),
];

$this->setBody(str_replace(
array_keys($symbols),
array_values($symbols),
$this->callParentTemplate
));
}
}
3 changes: 2 additions & 1 deletion src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function __construct(
) {
parent::__construct($originalClass, '__get', [new ParameterGenerator('name')]);

$hasParent = $originalClass->hasMethod('__get');
$hasParent = $originalClass->hasMethod('__get') ? $originalClass->getMethod('__get') : null;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This decision path is duplicated everywhere, and can surely be simplified.


$this->setDocBlock(($hasParent ? "{@inheritDoc}\n" : '') . '@param string $name');

Expand All @@ -78,6 +78,7 @@ public function __construct(
$initializer,
$valueHolder,
$callParent . PublicScopeSimulator::getPublicAccessSimulationCode(
$hasParent,
PublicScopeSimulator::OPERATION_GET,
'name',
null,
Expand Down
4 changes: 3 additions & 1 deletion src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicIsset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ public function __construct(
$initializer = $initializerProperty->getName();
$valueHolder = $valueHolderProperty->getName();
$callParent = '';
$parent = $originalClass->hasMethod('__isset') ? $originalClass->getMethod('__isset') : null;

$this->setDocBlock(($originalClass->hasMethod('__isset') ? "{@inheritDoc}\n" : '') . '@param string $name');
$this->setDocBlock(($parent ? "{@inheritDoc}\n" : '') . '@param string $name');

if (! $publicProperties->isEmpty()) {
$callParent = 'if (isset(self::$' . $publicProperties->getName() . "[\$name])) {\n"
Expand All @@ -67,6 +68,7 @@ public function __construct(
}

$callParent .= PublicScopeSimulator::getPublicAccessSimulationCode(
$parent,
PublicScopeSimulator::OPERATION_ISSET,
'name',
null,
Expand Down
3 changes: 2 additions & 1 deletion src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public function __construct(
[new ParameterGenerator('name'), new ParameterGenerator('value')]
);

$hasParent = $originalClass->hasMethod('__set');
$hasParent = $originalClass->hasMethod('__set') ? $originalClass->getMethod('__set') : null;
$initializer = $initializerProperty->getName();
$valueHolder = $valueHolderProperty->getName();
$callParent = '';
Expand All @@ -74,6 +74,7 @@ public function __construct(
$callParent .= $hasParent
? 'return $this->' . $valueHolder . '->__set($name, $value);'
: PublicScopeSimulator::getPublicAccessSimulationCode(
$hasParent,
PublicScopeSimulator::OPERATION_SET,
'name',
'value',
Expand Down
3 changes: 2 additions & 1 deletion src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicUnset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function __construct(
) {
parent::__construct($originalClass, '__unset', [new ParameterGenerator('name')]);

$hasParent = $originalClass->hasMethod('__unset');
$hasParent = $originalClass->hasMethod('__unset') ? $originalClass->getMethod('__unset') : null;
$initializer = $initializerProperty->getName();
$valueHolder = $valueHolderProperty->getName();
$callParent = '';
Expand All @@ -70,6 +70,7 @@ public function __construct(
$callParent .= $hasParent
? 'return $this->' . $valueHolder . '->__unset($name);'
: PublicScopeSimulator::getPublicAccessSimulationCode(
$hasParent,
PublicScopeSimulator::OPERATION_UNSET,
'name',
null,
Expand Down
Loading