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

Add currentStmt property on AbstractRector to allow pull Scope from it on deep Expr #4437

Merged
merged 4 commits into from
Jul 19, 2023

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Jul 8, 2023

Currently, on out of control deep Expr, like this:

$$param = $$bar = $zzz = self::decodeValue($result->getItem()->getTextContent());

we currrently direct pull Scope from File:

        // too deep Expr, eg: $$param = $$bar = self::decodeValue($result->getItem()->getTextContent());
        if ($node instanceof Expr && $node->getAttribute(AttributeKey::EXPRESSION_DEPTH) >= 2) {
            return $this->scopeFactory->createFromFile($filePath);
        }

Instead, I think we can pull Scope from nearest current Stmt, which can be pulled on AbstractRector when verify current valid Node, check nearest Stmt before match:

    private function isMatchingNodeType(Node $node): bool
    {
        $nodeClass = $node::class;
        foreach ($this->getNodeTypes() as $nodeType) {
            if (! is_a($nodeClass, $nodeType, true)) {
+                if ($node instanceof Stmt) {
+                    $this->currentStmt = $node;
+                }

                continue;
            }

            return true;
        }

        return false;
    }

so we can pull Scope from there.

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik
Copy link
Member Author

Rebased 👍

@samsonasik
Copy link
Member Author

I think if it need to be applied, it should be a "nodeHolder" property, as when passed is a ClassMethod, the holder is for example: a Class_, when Assign, the holder can be an Expression stmt.

That's we may can know if the holder is no longer belong to the node on RectifiedAnalyzer on multi rules apply so can be skippable instead of continue.

@TomasVotruba
Copy link
Member

Let's ship it 👍

@TomasVotruba TomasVotruba merged commit ceab6d7 into main Jul 19, 2023
43 checks passed
@TomasVotruba TomasVotruba deleted the add-current-stmt branch July 19, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants