Skip to content
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.

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

Upgrade to Nette 3.1 #5195

Merged
merged 17 commits into from
Jan 20, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions config/set/nette-31.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use Rector\Composer\Rector\ChangePackageVersionRector;
use Rector\Composer\Rector\RemovePackageRector;
use Rector\Composer\ValueObject\PackageAndVersion;
use Rector\Nette\Rector\Assign\ArrayAccessAddRouteToAddRouteMethodCallRector;
use Rector\Nette\Rector\MethodCall\ContextGetByTypeToConstructorInjectionRector;
use Rector\Renaming\Rector\MethodCall\RenameMethodRector;
use Rector\Renaming\Rector\Name\RenameClassRector;
use Rector\Renaming\Rector\StaticCall\RenameStaticMethodRector;
Expand All @@ -19,7 +21,7 @@
$services->set(RenameClassRector::class)->call('configure', [[
RenameClassRector::OLD_TO_NEW_CLASSES => [
// https://github.com/nette/application/compare/v3.0.7...v3.1.0
'Nette\Application\IRouter' => 'Nette\Routing\Router', // TODO not sure about this, it is not simple rename, Nette\Routing\Router already exists in nette/routing
'Nette\Application\IRouter' => 'Nette\Routing\Router',
'Nette\Application\IResponse' => 'Nette\Application\Response',
'Nette\Application\UI\IRenderable' => 'Nette\Application\UI\Renderable',
'Nette\Application\UI\ISignalReceiver' => 'Nette\Application\UI\SignalReceiver',
Expand Down Expand Up @@ -90,9 +92,9 @@
]),
]]);

// TODO change $router[] = new Router() to $router->addRoute() because of deprecated flags
$services->set(ArrayAccessAddRouteToAddRouteMethodCallRector::class);

// TODO Presenter->getContext() and Presenter->context is deprecated
$services->set(ContextGetByTypeToConstructorInjectionRector::class);

$services->set(ChangePackageVersionRector::class)
->call('configure', [[
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?php

declare(strict_types=1);

namespace Rector\Nette\Rector\Assign;

use PhpParser\Node;
use PhpParser\Node\Expr\ArrayDimFetch;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\MethodCall;
use PhpParser\Node\Expr\Variable;
use Rector\Core\Rector\AbstractRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

/**
* @see \Rector\Nette\Tests\Rector\Assign\ArrayAccessAddRouteToAddRouteMethodCallRector\ArrayAccessAddRouteToAddRouteMethodCallRectorTest
*/
final class ArrayAccessAddRouteToAddRouteMethodCallRector extends AbstractRector
lulco marked this conversation as resolved.
Show resolved Hide resolved
{
public function getRuleDefinition(): RuleDefinition
{
return new RuleDefinition(
'Change magic array access add Route, to explicit $routeList->addRoute(...) method',
[
new CodeSample(
lulco marked this conversation as resolved.
Show resolved Hide resolved
<<<'CODE_SAMPLE'
use Nette\Application\Routers\Route;
use Nette\Application\Routers\RouteList;

class RouterFactory
{
public static function createRouter()
{
$routeList = new RouteList();
$routeList[] = new Route('<module>/<presenter>/<action>[/<id>]', 'Homepage:default');
return $routeList;
}
}
CODE_SAMPLE
,
<<<'CODE_SAMPLE'
use Nette\Application\Routers\RouteList;

class RouterFactory
{
public static function createRouter()
{
$routeList = new RouteList();
$routeList->addRoute('<module>/<presenter>/<action>[/<id>]', 'Homepage:default');
return $routeList;
}
}
CODE_SAMPLE
),

]);
}

/**
* @return string[]
*/
public function getNodeTypes(): array
{
return [Assign::class];
}

/**
* @param Assign $node
*/
public function refactor(Node $node): ?Node
{
if (! $this->isAssignOfRouteToRouteListDimFetch($node)) {
return null;
}

/** @var ArrayDimFetch $arrayDimFetch */
$arrayDimFetch = $node->var;

/** @var Node\Expr\New_ $newRoute */
$newRoute = $node->expr;

return new MethodCall($arrayDimFetch->var, 'addRoute', $newRoute->args);
}

private function isAssignOfRouteToRouteListDimFetch(Assign $assign): bool
{
if (! $assign->var instanceof ArrayDimFetch) {
return false;
}

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the code.
The right part must be checked for New_ node instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check added

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be resolved. Rule must be configurable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I will try something. If I have 3+ config strings (listClass, itemClass, listMethod) I will need some ValueObject right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

I will need some ValueObject right?

Yes, that would be way to go for 3+ values to check

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

I will need some ValueObject right?

Yes, that would be way to go for 3+ values to check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic rule is created, but probably some renamings should be done...

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

declare(strict_types=1);

namespace Rector\Nette\Tests\Rector\Assign\ArrayAccessAddRouteToAddRouteMethodCallRector;

use Iterator;
use Rector\Nette\Rector\Assign\ArrayAccessAddRouteToAddRouteMethodCallRector;
use Rector\Testing\PHPUnit\AbstractRectorTestCase;
use Symplify\SmartFileSystem\SmartFileInfo;

final class ArrayAccessAddRouteToAddRouteMethodCallRectorTest extends AbstractRectorTestCase
{
/**
* @dataProvider provideData()
*/
public function test(SmartFileInfo $fileInfo): void
{
$this->doTestFileInfo($fileInfo);
}

public function provideData(): Iterator
{
return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
}

protected function getRectorClass(): string
{
return ArrayAccessAddRouteToAddRouteMethodCallRector::class;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

namespace Rector\Nette\Tests\Rector\Assign\ArrayAccessAddRouteToAddRouteMethodCallRector\Fixture;

use Nette\Application\Routers\Route;
use Nette\Application\Routers\RouteList;

class RouterFactory
{
public static function createRouter()
{
$routeList = new RouteList();
$routeList[] = new Route('<module>/<presenter>/<action>[/<id>]', 'Homepage:default');
return $routeList;
}
}

?>
-----
<?php

namespace Rector\Nette\Tests\Rector\Assign\ArrayAccessAddRouteToAddRouteMethodCallRector\Fixture;

use Nette\Application\Routers\Route;
use Nette\Application\Routers\RouteList;

class RouterFactory
{
public static function createRouter()
{
$routeList = new RouteList();
$routeList->addRoute('<module>/<presenter>/<action>[/<id>]', 'Homepage:default');
return $routeList;
}
}

?>