Add ability to add new nodes from enterNode()

[theofidry]

See #507 (comment)

---

Original post:

Is it a feature you would be willing to consider? Right now it seems this is possible only in leaveNode() and if the parent is an array.

Maybe this could be done by introducing a NullNode class that can wrap several nodes.

[nikic]

Can you please provide an example of what this means? I don't understand what "add" would do if the parent is not an array.

[theofidry]

Yes sorry. For example with the following example, I could replace:

const X = 'foo', Y = 'bar';

by:

const X = 'foo';
const Y = 'bar';

To do so you could image the following node visitor:

final class ReplaceConstStmts extends NodeVisitorAbstract
{
    /**
     * {@inheritdoc}
     */
    public function enterNode(Node $node): Node
    {
        if (false === ($node instanceof Node\Stmt\Const_)) {
            return $node;
        }

        return new NullNode(
            array_map(
                function (Node\Const_ $constant) use ($node): Node\Stmt\Const_ {
                    return new Node\Stmt\Const_([$constant], $node->getAttributes());
                },
                $node->consts
            )
        );
    }
}

And when printing the array of nodes, NullNode would actually not be rendered but only its wrapped nodes

The example above is one kind of transformation that makes much easier for PHP-Scoper since sometimes I need to transform const into define(). I already do a similar transformation for the grouped use statements although I can do that one in the traverser directly.

More generally though, I think it would make it easier to manipulate the AST that way.

[nikic]

@theofidry Isn't this the same as using the array return value in leaveNode()? Is the feature request here to also allow this in enterNode(), or does NullNode provide some different/additional functionality?

[theofidry]

Isn't this the same as using the array return value in leaveNode()?

It differs in several way:

• It can be used in enterNode() as well as you noted
• The parent does not require to be an array

I would be fine with returning an array as well instead. I suggested NullNode because it looks easier to deal with

[Kolesar]

I do not know if this is the right topic, but do not want open one more.

I cannot find how to add a new node in any case here.

I want to add one new use, but I do not know how :(

I guess it is something here:

public function enterNode(Node $node)
{
   if ($node instanceof Node\Stmt\Use_) {
   }
}

[TomasVotruba]

Do you still need help with this?

I now know how to replace, remove, add new nodes on many places in @rectorphp

[theofidry]

@TomasVotruba I do, however I really have no time to look into this right now and unlikely to have any before new year.

I'm also still not sure wether or not @nikic is cool with it.

Nonetheless, if you have a couple of examples they would be welcome as they may help to move forward with the discussion regarding the API and to decide if worth having here in the library or not

[TomasVotruba]

@theofidry Sure 👍 I'm pretty busy this month myself, crazy December :)

I'm also still not sure wether or not @nikic is cool with it.

I don't understand. php-parser already support adding of any node you could think of.

Nonetheless, if you have a couple of examples they would be welcome as they may help to move forward with the discussion regarding the API and to decide if worth having here in the library or not

It's hard to tell in general. I can comment specific code though. If you want something in php-scope, just create issue there with current code and desired one. And ping me there.

[theofidry]

I don't understand. php-parser already support adding of any node you could think of.

I'm confused. Do you have examples? Because my two first comments are exactly this

[TomasVotruba]

Like this? https://github.com/rectorphp/rector/blob/master/docs/AllRectorsOverview.md#delegateexceptionargumentsrector

[danog]

A temporary workaround could be returning the following node:

new Node\Stmt\If_(
    new Node\Expr\ConstFetch(new Node\Name('true')),
    ['stmts' => $arrayOfStmts]
);

A bit ugly, but it does the job.

[theofidry]

@TomasVotruba it is a good solution to delegate the adding/removal of nodes the way you did. But overall I find the solution quite complex compared to adding a NullNode which would be only here to nest nodes/statements and not be rendered at all as explained here

[TomasVotruba]

Why not just return array of nodes?

[theofidry]

I just thought a null object would be easier to deal with semantically. But that's a suggestion like another, so if it's easier to deal with an array of nodes instead of a null object it's fine by me.

@nikic any opinion about this? I may try to do a PR but I would rather make sure we're on the same page first

[nikic]

I still don't understand what this is about. The ability to return arrays already exists and the example in #507 (comment) would already work with it for leaveNode. Is this about extending support for enterNode?