Upgrade to Nette 3.1 #5195
Upgrade to Nette 3.1 #5195
Conversation
|
@TomasVotruba Hi, I just finished with renaming things. I didn't see any other issues except those I marked as TODOs. Are there any rector rules to cover those or we have to write new rules? |
|
Good job with use of generic rules 👍 lot of case are covered now |
…etByTypeToConstructorInjectionRector in nette-31 set
|
@TomasVotruba I think it is ready for review. Code Analysis PHP 8.0 / PHPStan is failing but I believe it is not connected to this PR |
rules/nette/src/Rector/Assign/ArrayAccessAddRouteToAddRouteMethodCallRector.php
Outdated
Show resolved
Hide resolved
rules/nette/src/Rector/Assign/ArrayAccessAddRouteToAddRouteMethodCallRector.php
Outdated
Show resolved
Hide resolved
| if (! $this->isObjectType($assign->expr, 'Nette\Application\Routers\Route')) { | ||
| return false; | ||
| } | ||
|
|
||
| $arrayDimFetch = $assign->var; | ||
| if (! $arrayDimFetch->var instanceof Variable) { | ||
| return false; | ||
| } | ||
|
|
||
| return $this->isObjectType($arrayDimFetch->var, 'Nette\Application\Routers\RouteList'); |
There was a problem hiding this comment.
The class and method should be passed via configure() method, so this rule can be used as generic one.
$services->set(ArrayAccessAddRouteToAddRouteMethodCallRector::class)
->call('configure', [[
ArrayAccessAddRouteToAddRouteMethodCallRector::TYPE_TO_METHOD_CALL => [
'Nette\Application\Routers\Route' => 'addRoute'
]
]]);Is the second class 'Nette\Application\Routers\RouteList' really neccessary or can we remove it?
There was a problem hiding this comment.
I'm not sure now. I have to go deeper :) there are at least two classes with method addRoute() but I don't know if both can be used in createRouter method...
But let assume that you have router which just adds Routes to some array and then iterates over this array and adds it to RouteList
Something like this:
public function createRouter()
{
$routeArray = [];
$routeList = new RouteList();
$routeArray[] = new Route('<module>/<presenter>/<action>[/<id>]', 'Homepage:default');
foreach ($routeArray as $route) {
$routeList[] = $route;
}
return $routeList;
}
With ignoring RouteList, our code will call ->addRoute on $routeArray
There was a problem hiding this comment.
I see. Thanks for the code.
The right part must be checked for New_ node instance.
There was a problem hiding this comment.
This needs to be resolved. Rule must be configurable
There was a problem hiding this comment.
Only time will tell. This abstraction is obvious (class and method name), so better do it now when we have focus, then in 6 month create duplicated rule. Yes that happens :)
There was a problem hiding this comment.
OK I will try something. If I have 3+ config strings (listClass, itemClass, listMethod) I will need some ValueObject right?
There was a problem hiding this comment.
Thank you 👍
I will need some ValueObject right?
Yes, that would be way to go for 3+ values to check
There was a problem hiding this comment.
Thank you 👍
I will need some ValueObject right?
Yes, that would be way to go for 3+ values to check
There was a problem hiding this comment.
Generic rule is created, but probably some renamings should be done...
|
The PHPStan CI fail is unrelated, I'll look into it |
As I wrote, it is inspired by existing rule ArrayAccessSetControlToAddComponentMethodCallRector |
|
So, are we done? :) |
|
This is not resolved yet |
|
Code Analysis PHP 8.0 / PHPStan failed again |
|
@TomasVotruba I merged master into branch and build is green again. Generic rule is done. Is there anything you want to add / change in this PR? |
|
Looks very good, thanks for your detailed work 👍 |
rectorphp/rector-src@f2c8967 [DeadCode] Mirror comment on assign method call on RemoveUnusedVariableAssignRector (#5195)
rectorphp/rector-src@f2c8967 [DeadCode] Mirror comment on assign method call on RemoveUnusedVariableAssignRector (#5195)
rectorphp/rector-src@f2c8967 [DeadCode] Mirror comment on assign method call on RemoveUnusedVariableAssignRector (#5195)
Resolves #5142