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

Fix incorrect result after using RemoveByType in PhpDocInfo #6301

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

carlos-granados
Copy link
Contributor

In the new InlineVarDocTagToAssertRector Rector rule we call the function RemoveByType() in the PhpDocInfo class like this:

                    $phpDocInfo->removeByType(VarTagValueNode::class, $variableName);

If we apply this to this example code:

            /**
             * @var Expression $stmt
             * @var FuncCall $expr
             */
            $expr = $stmt->expr;

We will call it with $variableName = "expr"

In the node visitor defined in line 266 of the PhpDocInfo class we would start by processing a node instance of PhpDocTagNode and its value would be an instance of VarTagValueNode so the first check would be true but the check in line 276 would be false because the variable name does not match the one we passed. Previously we would have returned null and the processing would have continued. We would then process the children of this node and one of them would be the VarTagValueNode. Since it matches the type of node that we want to remove it would be removed, but then we would be left with an invalid parent PhpDocTagNode node with value null and this can create issues if another rule tries to process this same node later on.
If the node value is a VarTagValueNode but the variable name does not match, the right solution is to stop processing the children of this node and this is what this PR implements

Hope this makes sense 😅

@carlos-granados
Copy link
Contributor Author

The Windows job failed but I think this is unrelated to this fix as it just timed out, can you maybe re-run it?

@samsonasik
Copy link
Member

Let's give it a try, thank you @carlos-granados

@samsonasik samsonasik merged commit 9620889 into rectorphp:main Sep 13, 2024
36 checks passed
@carlos-granados carlos-granados deleted the fix-remove-by-type branch September 13, 2024 20:11
@samsonasik
Copy link
Member

@carlos-granados btw, could you add test for it, you can place under tests/Issues for multi rules apply? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants