[WIP] Fix #346 - magic methods hints need to be reflected in generated child classes #350
Conversation
| $this->setReturnsReference($originalMethod->returnsReference()); | ||
| $this->setReturnType(((string) $originalMethod->getReturnType()) ?: null); | ||
| $this->addOrReplaceParameterNames($originalMethod, ...$parameters); | ||
| } else { |
There was a problem hiding this comment.
Don't like this else. Can probably just call the parent constructor with the given parameters, then replace
| } | ||
| } | ||
|
|
||
| private function addOrReplaceParameterNames( |
There was a problem hiding this comment.
This method is way too complex: need to split into multiple private methods explaining exactly what is going on at each step.
| ) : void { | ||
| $originalParameters = array_values(array_map( | ||
| function (\ReflectionParameter $parameter) use ($originalMethod) : ParameterGenerator { | ||
| return ParameterGenerator::fromReflection(new ParameterReflection( |
There was a problem hiding this comment.
Loads of coupling due to Zend\Code's shitty reflection API :-(
| $parentAccess | ||
|
|
||
| $symbols = [ | ||
| '%init' => '$this->' . $initializerProperty->getName() |
There was a problem hiding this comment.
I should hunt down all the sprintf() and replace them with this syntax: much easier to follow.
| @@ -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; | |||
There was a problem hiding this comment.
This decision path is duplicated everywhere, and can surely be simplified.
| * | ||
| * @return string | ||
| * | ||
| * @throws \InvalidArgumentException | ||
| */ | ||
| public static function getPublicAccessSimulationCode( | ||
| ?\ReflectionMethod $originalMethod, |
There was a problem hiding this comment.
Also note: since we are passing down reflection details, we may as well get rid of a lot of runtime boilerplate, to be replaced with string constants.
| case static::OPERATION_UNSET: | ||
| return 'unset($targetObject->$' . $nameParameter . ');'; | ||
| return 'unset($targetObject->$' . $nameParameter . ');' |
There was a problem hiding this comment.
return unset($foo) is not a valid expression, so I simply add a fake return block afterwards.
| @@ -119,6 +121,8 @@ public function getTestedClasses() : array | |||
| return [ | |||
| [BaseClass::class], | |||
| [ClassWithMagicMethods::class], | |||
| [ClassWithHintedMagicMethods::class], | |||
| //[InterfaceWithHintedMagicMethods::class], | |||
There was a problem hiding this comment.
This test doesn't verify interface proxying - need to move to appropriate functional tests instead.
…gic methods, as hints should be preserved
…n blocks into account (varies depending on `void` or non-void return hint)
…int of a previously existing class, if given, and replaces names only
…ode generator API
Ocramius
force-pushed
the
fix/#346-magic-methods-hints-reproducibility
branch
from
7dbb2ef
to
acbc96f
Compare
…cAccessSimulationCode()`, as the parameter count changed
|
2 years, any progress? Two more assets that make test suite crash: Majkl578@06d9f51 Note that with 7.2+ requirement you could simply ignore parameters, but you still need return types and void. |
|
No progress, but I could restart it. can you PR the assets to |
This is just a patch that gets us where we want, but the introduced complexity is too much, therefore will need to find a better way to implement it, without having codegen talking to every reflection layer.