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

avoid multiple reads/writes for inherited properties in hydrator class #85

Open
wants to merge 3 commits into
base: 4.2.x
Choose a base branch
from
+106 −2
Open

avoid multiple reads/writes for inherited properties in hydrator class #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9322fa1
avoid multiple reads/writes for inherited properties in hydrator class
pvonr Jun 11, 2019
d9e3829
avoid directaccess to public properies of ext-reflection
pvonr Jun 11, 2019
0fb7399
add unit test
pvonr Jun 12, 2019
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
4 changes: 2 additions & 2 deletions src/GeneratedHydrator/CodeGenerator/Visitor/HydratorMethodsVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ private function findAllInstanceProperties(?ReflectionClass $class = null) : arr
$this->findAllInstanceProperties($class->getParentClass() ?: null), // of course PHP is shit.
array_values(array_filter(
$class->getProperties(),
static function (ReflectionProperty $property) : bool {
return ! $property->isStatic();
static function (ReflectionProperty $property) use ($class) : bool {
return ! ($property->isStatic() || $property->getDeclaringClass()->getName() !== $class->getName());
}
))
));
Expand Down
104 changes: 104 additions & 0 deletions tests/GeneratedHydratorTest/CodeGenerator/Visitor/HydratorMethodsVisitorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
use CodeGenerationUtils\Inflector\Util\UniqueIdentifierGenerator;
use GeneratedHydrator\CodeGenerator\Visitor\HydratorMethodsVisitor;
use PhpParser\Node;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\NodeFinder;
use PhpParser\ParserFactory;
use PHPUnit\Framework\TestCase;
use ReflectionClass;
Expand Down Expand Up @@ -81,4 +83,106 @@ public function classAstProvider() : array
[$staticClassName, $parser->parse('<?php ' . $staticClassCode)[0], ['taz']],
];
}

/**
* @dataProvider propertyAstProvider
*/
public function testPropertyCodeGeneration(string $className, Class_ $classNode, array $properties) : void
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, this code is indeed complex and fragile. I applaud your efforts!

Still, I was wondering if we could write something like this:

$expected = <<<'PHP'
public function hydrate(...)
{
   // ...
}
PHP;

$actual = <<<'PHP'
public function hydrate(...)
{
   // ...
}
PHP;

$expectedAst = $parser->parse($expected);
$actualAst = $parser->parse($actual);

self::assertEquals(
    $expected->some->path->to->the->method->node,
    $actual->some->path->to->the->method->node
);

Is such a test feasible, in your opinion?

Also feasible: using the PHP-Parser pretty-printer to compare the AST via string rather than AST.

Copy link
Author

Choose a reason for hiding this comment

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

Was also thinking about using the pretty-printer and comparing strings :) though you would not like it ;)

Not sure whether assertEquals really works on a deeply nested array with different objects.. does it?

{
$visitor = new HydratorMethodsVisitor(new ReflectionClass($className));

/** @var Class_ $modifiedAst */
$modifiedNode = $visitor->leaveNode($classNode);

$constructors = $this->findConstructor($modifiedNode);
self::assertCount(1, $constructors);
$constructor = reset($constructors);

$hydratePropertyNames = $this->findAssignedPropertyNames(
$constructor,
'hydrateCallbacks',
'object',
function (Assign $assign) {
return $assign->var->name->name;
}
);
$extractPropertyNames = $this->findAssignedPropertyNames(
$constructor,
'extractCallbacks',
'values',
function (Assign $assign) {
return $assign->var->dim->value;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Not important, but \t is generally to be avoided. Can fix on merge from my end though.

Copy link
Author

Choose a reason for hiding this comment

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

ah sorry. didn't switch to spaces (working with tabs internally).

Copy link
Owner

Choose a reason for hiding this comment

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

No worries: that is entirely my preference, and I just forgot to add PHPCS to the repo ;-)

);

self::assertSameSize($properties, $hydratePropertyNames);
self::assertSameSize($properties, $extractPropertyNames);
self::assertCount(0, array_diff($properties, $hydratePropertyNames));
self::assertCount(0, array_diff($properties, $extractPropertyNames));
}

private function findConstructor(Class_ $class) : array {
return array_filter(
$class->stmts,
static function (Node $node): bool {
return $node instanceof ClassMethod && '__construct' === $node->name->name;
}
);
}

private function findAssignedPropertyNames(Node $node, string $callbackName, string $variableName, callable $mapper)
{
$finder = new NodeFinder();
$callbacks = $finder->find($node, function(Node $node) use ($callbackName) {
return $node instanceof Assign
&& $node->var->var->name instanceof Node\Identifier
&& $node->var->var->name->name === $callbackName;
});

$found = [];
foreach($callbacks as $callback) {
/** @noinspection SlowArrayOperationsInLoopInspection */
$found = array_merge($found, $finder->find($callback, function(Node $node) use ($variableName) {
return $node instanceof Assign
&& is_string($node->var->var->name)
&& $node->var->var->name === $variableName;
}));
}

return array_map($mapper, $found);
}

/**
* @return Node[]
*/
public function propertyAstProvider(): array
{
$parser = (new ParserFactory())->create(ParserFactory::ONLY_PHP7);

$className = UniqueIdentifierGenerator::getIdentifier('Foo');
$classCode = 'class '.$className.' { private $bar; private $baz; protected $tab; '
.'protected $tar; }';

eval($classCode);
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, interesting, I thought I made this library kinda side-effect free :|

Guess I'll have more work to do.


$subClassName = UniqueIdentifierGenerator::getIdentifier('Foo');
$subClassCode = 'class '.$subClassName.' extends '.$className.' { private $fuz; protected $buz; }';

eval($subClassCode);

$sub2ClassName = UniqueIdentifierGenerator::getIdentifier('Foo');
$sub2ClassCode = 'class '.$sub2ClassName.' extends '.$subClassName.' { protected $bis; }';

eval($sub2ClassCode);

return [
[$className, $parser->parse('<?php '.$classCode)[0], ['bar', 'baz', 'tab', 'tar']],
[$subClassName, $parser->parse('<?php '.$subClassCode)[0], ['bar', 'baz', 'tab', 'tar', 'fuz', 'buz']],
[
$sub2ClassName,
$parser->parse('<?php '.$sub2ClassCode)[0],
['bar', 'baz', 'tab', 'tar', 'fuz', 'buz', 'bis'],
],
];
}
}