8000 RouteCollectionBuilderToRoutingConfiguratorRector should not rename `confifgureRoutes` to `configureRouting` · Issue #500 · rectorphp/rector-symfony · GitHub
[go: up one dir, main page]
Skip to content

RouteCollectionBuilderToRoutingConfiguratorRector should not rename confifgureRoutes to configureRouting #500

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.

Sign up for GitHub

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

Jump to bottom
Closed
rimas-kudelis opened this issue Aug 1, 2023 · 9 comments · Fixed by #540
Closed

RouteCollectionBuilderToRoutingConfiguratorRector should not rename confifgureRoutes to configureRouting #500

rimas-kudelis opened this issue Aug 1, 2023 · 9 comments · Fixed by #540

Comments

@rimas-kudelis
Copy link
rimas-kudelis commented Aug 1, 2023 •
edited
Loading

The following fixer:

rector-symfony/rules/Symfony51/Rector/ClassMethod/RouteCollectionBuilderToRoutingConfiguratorRector.php

Lines 78 to 132 in a0af12a

public function refactor(Node $node): ?Node
{
if (! $this->isName($node, 'configureRoutes')) {
return null;
}
$firstParam = $node->params[0];
if ($firstParam->type === null) {
return null;
}
if (! $this->isName($firstParam->type, 'Symfony\Component\Routing\RouteCollectionBuilder')) {
return null;
}
$firstParam->type = new FullyQualified('Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator');
$node->name = new Identifier('configureRouting');
$node->returnType = new Identifier('void');
$this->traverseNodesWithCallable((array) $node->stmts, function (Node $node): ?MethodCall {
if (! $node instanceof MethodCall) {
return null;
}
if (! $this->isName($node->name, 'add')) {
return null;
}
// avoid nesting chain iteration infinity loop
$shouldSkip = (bool) $node->getAttribute(AttributeKey::DO_NOT_CHANGE);
if ($shouldSkip) {
return null;
}
$node->setAttribute(AttributeKey::DO_NOT_CHANGE, true);
$pathValue = $node->getArgs()[0]
->value;
$controllerValue = $node->getArgs()[1]
->value;
$nameValue = $node->getArgs()[2]
->value ?? null;
if (! $nameValue instanceof Expr) {
throw new NotImplementedYetException();
}
$node->args = [new Arg($nameValue), new Arg($pathValue)];
return new MethodCall($node, 'controller', [new Arg($controllerValue)]);
});
return $node;
}

renames the kernel's configureRoutes() method to configureRouting(). However, it looks like Symfony didn't rename the method they are looking for, so after the fixer fixes the code, this method is no longer being used.
< 8000 /div>
@rimas-kudelis
Copy link
Author

Also, the fixer doesn't fix any other usages of the RouteCollectionBuilder class. For example, if the configureRoutes() method passes its $routes argument to another method, that other method remains unfixed and still requires an instance of RouteCollectionBuilder and not a RoutingConfigurator.

Here's an example of such kernel: https://github.com/BitBagCommerce/OpenMarketplace/blob/master/src/Kernel.php.

@TomasVotruba
Copy link
Member

Hi, could you send a faling test fixture for RouteCollectionBuilderToRoutingConfiguratorRector? That way we'll have it covered and can fix the behavior

@TomasVotruba
Copy link
Member
TomasVotruba commented Oct 24, 2023 •
edited
Loading

I'm looking into this and this change comes only in Symfony 5.1:
symfony/symfony#32937

It should not be used in previous version and could have changed in future versions. If that happened and Rector missed this change, it will require a new rule in particular Rector version

@TomasVotruba TomasVotruba mentioned this issue Oct 24, 2023
Merged
@TomasVotruba
Copy link
Member
TomasVotruba commented Oct 24, 2023 •
edited
Loading

To reopen, please send a failing before/after test fixture to RouteCollectionBuilderToRoutingConfiguratorRectorTest so we know exactly what is being changed :)

Thanks 🙏

@rimas-kudelis
Copy link
Author

I'm looking into this and this change comes only in Symfony 5.1: symfony/symfony#32937

It should not be used in previous version and could have changed in future versions. If that happened and Rector missed this change, it will require a new rule in particular Rector version

Maybe this PR was later reversed? Because even when looking at this file in 5.1 branch, I don't see any references to configureRouting(), but I definitely see some to configureRoutes(). Or am I confusing something?

@rimas-kudelis
Copy link
Author
rimas-kudelis commented Oct 25, 2023 •
edited
Loading

In fact, if you look at the history of this file, you'll find that the change you referenced (Nov. 25, 2019) was overwritten a couple weeks later (Dec 13, 2019) and the method name was changed back.

I'm not sure how you decide to write these fixers, but I suppose it would be safer to work on them only when the relevant target branch tags a stable release, and to look at the relevant UPGRADE-x.y.md file first.

@TomasVotruba
Copy link
Member

I see. It's possible this rule was created in that window. Thanks for investigation 👍

Could you send a PR to remove this rule then?

@rimas-kudelis
Copy link
Author

Please take care of this if you can. 🙏 I'm not sure when I would get to this myself.

@TomasVotruba
Copy link
Member
TomasVotruba commented Oct 25, 2023 •
edited
Loading

No worries, I'll check it when I find some time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle only Kernel in RouteCollectionBuilderToRoutingConfiguratorRector rectorphp/rector-symfony
2 participants
@TomasVotruba @rimas-kudelis
0