Migration of route methods from Slim 2 to Slim 4

[brotkrueml]

We want to migrate our route methods (essentially from Slim 2 to Slim 4). We thought we should try it with rector for the first time ;-)

The main aspects are:

• The method signature changes: previously the route parameters are given as method arguments. Now we have a PSR-7 request and PSR-7 response along with an args array argument which holds the route parameters. Also the return type is set to a PSR-7 response.
• The given method arguments are now in $args array variables and must be adjusted in the method body itself.
• Instead of calling a class method a new instance of a Responder class is returned.

Previous code:

/**
 * @Vendor\Common\Annotations\Annotation\Route("../archives/{archive}/documents/{docid}/file", name="downloadfile", method="GET")
 */
public function downloadFile($archive, $docid)
{
    $this->archiveTableName = $archive;
    $this->prepareDownloadEnvironment($docid);

    // do something
 
    $this->setBinaryResult($file->getData());
}

Target code:

public function downloadFile(ServerRequestInterface $request, ResponseInterface $response, array $args): ResponseInterface
{
    $this->archiveTableName = $args['archive'];

    $this->prepareDownloadEnvironment($args['docid']);

    // do something

    return new BinaryResponder($file);
}

Our rector by now (which sets the method arguments and the return type):

public function getNodeTypes(): array
{
    return [Node\Stmt\ClassMethod::class];
}

public function refactor(Node $node)
{
    // TODO: Get only methods with Route annotation

    // Substitute method arguments
    $subNodes = [
       'attrGroups' => $node->attrGroups,
       'flags' => $node->flags,
       'byRef' => $node->byRef,
       'name' => $node->name,
       'params' => [
            new Node\Param(new Node\Expr\Variable('request'), null, ServerRequestInterface::class),
            new Node\Param(new Node\Expr\Variable('response'), null, ResponseInterface::class),
            new Node\Param(new Node\Expr\Variable('args'), null, 'array'),
        ],
        'returnType' => ResponseInterface::class,
        'stmts' => $node->getStmts(),
    ];

    $node = new Node\Stmt\ClassMethod(
        $node->name->name,
        $subNodes,
        $node->getAttributes()
    );

    // TODO: Adjust previous method arguments to use $args array
    // TODO: Substitute method call with a new instantiated Responder returned

    return $node;
}

Now our questions:

• Is the substitution of the method arguments the right way, by now it works.
• Should we use one rector rule for each substitution? For the method signature and the substitution with $args array this should be done in one rule, we think.

[TomasVotruba]

Hi, thanks for question with real PHP examples. I think this is a great use case to save yourself big amount of manual work 👍

Rector tried to follow unix philosophy - build simple rules that handle 1 job right, then connect them together solid realible machine.

From top of my head, I see there 3 different rules:

• replace params
• replace used variables in method body
• replace setBinaryResult() with return new BinaryResponder

Of course you can fit those to one big God Rector that handles everything and have just 1 file :) but in my experience, this crashes programmers brains with cognitive overload, as they juggle too many concepts at once and get confused with ever simple operation like variable rename.

It's better to just modify the node, instead of replacing it.

$node = new Node\Stmt\ClassMethod(new Identifier('newName'));

vs.

$node->name = new Identifier('newName');

Why? Each node hold so called "attributes", that contain metadata about spaces, parent nodes, resolved names and types. These metadata are important to static type resolving and printing node in code in original format as much as possible. Creating a new node will remove all these attributes and can break stuff.

It's also best practise to change as small node as possible. E.g. if you need to rename 1 method, you should not do it on Class_ node, but rather ClassMethod.

Is the substitution of the method arguments the right way, by now it works.

Last thing... if it works, it's good enough :) if this rule is create & apply & remove, there is no need to write it with best practises in mind. Just run it, refactor and throw it away :). But if you plan to help you the Slim community and release this is a migration set, best practise will make these rule more reliable and easy to fix.

I think you're on the right path. Good luck 👍

[brotkrueml]

Thanks for your explanations :-) This is very welcome and we will work in it.