-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fix EntityManagerRector based on project_analysis results #274
Conversation
Fixes 220 error while running analysis. Search for In https://git.drupalcode.org/project/project_analysis/-/commits/11-55227?ref_type=tags |
Marked draft, needs better solution. |
Copied a functional test locally, which shows the difference. Will push tonight. |
Ok, only error i made was mostly the removal of the depth counter. Also made phpstan a little more happy. Ready for review. Should solve lots of errors in project anlysis |
0c9acfb
to
bb3284c
Compare
bb3284c
to
5a2340f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes, I think we can tidy up some of the checks.
// Should the expression be the class we are looking for and the name is the one we are looking for, we can return early. | ||
if ($node instanceof $class && $this->getName($node->name) === $name) { | ||
if ($node instanceof Node && $node instanceof $class && $this->getName($node->name) === $name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expr
is nullable? It's not supposed to be
$node = $node->var; | ||
++$depth; | ||
if ($node instanceof $class && $this->getName($node->name)) { | ||
|
||
if ($node instanceof Node && $node instanceof $class && isset($node->name) && $this->getName($node->name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, feels weird that we have to instanceof
something which shouldn't be nullable and is \PhpParser\Node\Expr
And we can call $this->getName($node)
, that's what the name resolver is for.
while (isset($node->var) && !($node->var instanceof $class && $this->getName($node->var->name) !== $name)) { | ||
while (isset($node->var) && !($node->var instanceof $class && isset($node->var->name) && $this->getName($node->var->name) !== $name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work if we did $this->getName($node->var)
? See \Rector\NodeNameResolver\NodeNameResolver\VariableNameResolver::resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this, it makes the functional tests fail :(
|
||
public function provideConfigFilePath(): string | ||
{ | ||
// must be implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has?
use Rector\Config\RectorConfig; | ||
|
||
return static function (RectorConfig $rectorConfig): void { | ||
// $rectorConfig->rule(EntityViewRector::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this?
I tried to use getName properly, but couldnt get unit and functional tests to work :( (small tip: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, too much work to make it fancy :)
Description
Explain the technical implementation of the work done.
To Test
Drupal.org issue
Provide a link to the issue from https://www.drupal.org/project/rector/issues. If no issue exists, please create one and link to this PR.