Rector: [Q] String interpolation to concat

[olivernybroe]

Question

Hi, so we are creating a bunch of custom rectors, and one of them makes the following translation

- $this->method()
+ method()

We are doing that with a replaceNode call.
However this results in errors when the code is running on the following

"{$this->method()}/some string"

As it will translate it to

- "{$this->method()}/some string"
+ "{method()}/some string"

Which is not valid for running the method.

How do we handle this? Is there some method we should run, or do we have to add in a custom scenario for when the MethodCall is inside a string interpolation and then convert it to valid PHP?

[TomasVotruba]

Hi,

could you share the full Rector rule + test case?

What is the parent node?

[olivernybroe]

@TomasVotruba Sure, however there are other stuff happening there
https://github.com/pestphp/drift/blob/master/src/PHPUnit/HelperMethodRector.php#L98-L112

I linked to the specific method in question.

[TomasVotruba]

I see.

First, you should use traverseNodesWithCallable() to replace nested nodes:

$this->callableNodeTraverser->traverseNodesWithCallable((array) $classMethod->stmts, function (Node $node) {
    if (! $node instanceof MethodCall) {
        return null;
    }

    // match node


    $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);

    // probably your case to skip
    // https://github.com/rectorphp/rector/blob/master/docs/nodes_overview.md#phpparsernodescalarencapsed

    if ($parentNode instanceof Encapsed) {
       return null;
    }


    // change it
    // or return null
    return null;
});

Could you link the test fixture for your Rector rule, where is this covered? That would be much easier for me to read.

[olivernybroe]

@TomasVotruba Alright, so to simplify it, I have made a simple example that shows the error.

My rector class is

class BasicRector extends AbstractRector
{

    public function getNodeTypes(): array
    {
        return [MethodCall::class];
    }

    public function refactor(Node $node): ?Node
    {
        return $this->createFuncCall($this->getName($node->name));
    }

    public function getDefinition(): RectorDefinition
    {
        return new RectorDefinition("example");
    }
}

And this is the fixture

<?php
class TestClass
{
    public function example()
    {
        return "{$this->method()} works";
    }
}
?>
-----
<?php
class TestClass
{
    public function example()
    {
        return method() . " works";
    }
}
?>

[TomasVotruba]

I'm not sure you've tried the suggestion in my previous comment. Any feedback on that?

[olivernybroe]

@TomasVotruba Alright, so I tried you previous comment out and it seems to work. However as we are still not able to handle Encapsed element, we just ignore them now.

Is there are helper method to handle them or do we have to write the logic for these cases and other edge-cases ourself?

Btw. what is the advantage of using traverseNodesWithCallable instead of replaceNode?

[TomasVotruba]

However as we are still not able to handle Encapsed element, we just ignore them now.

If you make a failing test case in your code and point me to failing Github Action, I might be able to help with pull-request there.

Btw. what is the advantage of using traverseNodesWithCallable instead of replaceNode?

It's a convention working with nodes and more tree-like approach. I haven't seen used/seen replaceNode() for a long time, have to check it :smile:

[olivernybroe]

@TomasVotruba Ah that's good to know. There are soo many nice features that is is hard to figure out what is the prefered to use.

I'll try and add one it, thanks!

[TomasVotruba]

Ah that's good to know

I think that method should be private, it's my fault. I'll update it so there is 1 way to do it.
I'm sorry for the confusion.

[olivernybroe]

@TomasVotruba What about the removeNode and addNodeAfterNode, are they okay to use?

I get it. So many methods, hard to keep track of them all 👍

[TomasVotruba]

Yes, they should be unique. The return $node is used in 99 % when the node is changed.

Added or removed nodes have to be handled outside the tree travserse, that's why it has this extra methods.

If you find any ambiguous or unclear methods in Rector, please let me know. The code can be always more clear.

[olivernybroe]

Thanks, I'll open issues when I stumble upon any unclear methods like that.

This kinda leads me to a follow up question, but that question is basically issue #3517, which I created recently.

[TomasVotruba]

This should solve the replaceNode() dualism :)
https://github.com/rectorphp/rector/pull/3518

[TomasVotruba]

Closing as answered.