Closed nikophil closed 8 months ago
I am for shipping it in the repo 👍🏻
Hey @OskarStark
I'd be okay to ship it in this repo, but I'm wondering how it should be split from the main library: for now I've totally decoupled it in order not to mess with dependencies: we need rector/rector
and phpstan/phpstan-doctrine
(both of them require phpstan/phpstan
).
It raises some issues:
bamarni/composer-bin-plugin
in order to separate "tools" dependencies (symfony doc, phpstan, psalm) from our main dependencies. But rector (and phpstan) will need to be at least in require-dev
section (in order to run the tests), which kinda remove the benefit from bamarni/composer-bin-plugin
.rector/rector
and phpstan/phpstan-doctrine
in the main require
section in composer.json
, we'd need to ask users to install by themselves rector. Moreover my rules need rector ^0.18.Faker does not require rector, but just ship the rules: https://github.com/FakerPHP/Faker/blob/2.0/rector-migrate.php
thecodingmachine/safe
does the same:
https://github.com/thecodingmachine/safe/blob/master/rector-migrate.php
So I would just ship the "rule set" which can then be included like:
<?php
declare(strict_types=1);
use Rector\Caching\ValueObject\Storage\FileCacheStorage;
use Rector\CodingStyle\Rector\FuncCall\ArraySpreadInsteadOfArrayMergeRector;
use Rector\Config\RectorConfig;
use Rector\Core\ValueObject\PhpVersion;
use Rector\Doctrine\Set\DoctrineSetList;
use Rector\Php80\Rector\Class_\AnnotationToAttributeRector;
use Rector\Php83\Rector\ClassMethod\AddOverrideAttributeToOverriddenMethodsRector;
use Rector\PHPUnit\CodeQuality\Rector\Class_\PreferPHPUnitThisCallRector;
use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\ReplaceTestAnnotationWithPrefixedFunctionRector;
use Rector\PHPUnit\PHPUnit100\Rector\Class_\StaticDataProviderClassMethodRector;
use Rector\PHPUnit\Rector\Class_\PreferPHPUnitSelfCallRector;
use Rector\PHPUnit\Set\PHPUnitLevelSetList;
use Rector\PHPUnit\Set\PHPUnitSetList;
use Rector\Set\ValueObject\LevelSetList;
use Rector\Set\ValueObject\SetList;
use Rector\Symfony\Set\SymfonyLevelSetList;
use Rector\Symfony\Set\SymfonySetList;
use Rector\Symfony\Set\TwigLevelSetList;
use function Safe\getcwd;
return static function (RectorConfig $rectorConfig): void {
$rectorConfig->parallel();
$rectorConfig->paths([
__DIR__.'/.php-cs-fixer.dist.php',
__DIR__.'/composer-unused.php',
__DIR__.'/rector.php',
__DIR__.'/phparkitect.php',
__DIR__.'/src',
__DIR__.'/migrations',
__DIR__.'/tests',
]);
$rectorConfig->skip([
__DIR__.'/src/KnowledgeTools/InstanceConfig/Instances',
]);
$rectorConfig->cacheClass(FileCacheStorage::class);
$rectorConfig->cacheDirectory('./var/cache/rector');
$rectorConfig->phpVersion(PhpVersion::PHP_83);
$rectorConfig->importNames();
$rectorConfig->importShortClasses(false);
$rectorConfig->phpstanConfigs([
getcwd().'/phpstan.neon.dist',
'vendor/phpstan/phpstan-doctrine/extension.neon',
'vendor/phpstan/phpstan-phpunit/extension.neon',
'vendor/phpstan/phpstan-symfony/extension.neon',
'vendor/phpstan/phpstan-webmozart-assert/extension.neon',
]);
$rectorConfig->sets([
SetList::PHP_83,
LevelSetList::UP_TO_PHP_83,
PHPUnitLevelSetList::UP_TO_PHPUNIT_90,
PHPUnitSetList::PHPUNIT_91,
PHPUnitSetList::PHPUNIT_CODE_QUALITY,
SymfonyLevelSetList::UP_TO_SYMFONY_63,
SymfonySetList::SYMFONY_CODE_QUALITY,
DoctrineSetList::DOCTRINE_CODE_QUALITY,
TwigLevelSetList::UP_TO_TWIG_240,
]);
$rectorConfig->import(__DIR__.'/vendor/fakerphp/faker/rector-migrate.php');
$rectorConfig->import(__DIR__.'/vendor/thecodingmachine/safe/rector-migrate.php');
};
cool, this sounds good, I think we can go in this direction, but there is a difference: they don't ship custom rules. So they don't need to test it... Currently I've added all the rules and their tests in a separate /utils
directory, with its specific composer.json
file and vendor
directory. But I'm not really sure this split is needed.
Another thing : our rules need rector ^0.18 and phpstan-doctrine, I think I'll throw an exception if those requirements are not respected.
cool, this sounds good, I think we can go in this direction, but there is a difference: they don't ship custom rules. So they don't need to test it... Currently I've added all the rules and their tests in a separate /utils directory, with its specific composer.json file and vendor directory. But I'm not really sure this split is needed.
That's another story, I am just talking about the end user, and how this can be consumed.
Another thing : our rules need rector ^0.18 and phpstan-doctrine, I think I'll throw an exception if those requirements are not respected.
Makes sense, or there is another way, that I am not aware of. @TomasVotruba can a rule define a rector version as minimum?
I just added a RuleRequirementChecker
using \Composer\InstalledVersions
This is great, thanks for this Christmas present @nikophil! I'm not concerned about all the extra code, we can remove it all in 2.x, right?
I'm not concerned about all the extra code, we can remove it all in 2.x, right?
I think we could do this, we'll have to advertise about the lastest 1.x
version which will be part of the migration process
Hi!
here is my present for christmas :gift: :santa:
I really felt this migration path was too much pain, so I decided to try to work on rector rules!
When my project is updated with the last
1.x-bc
version (actually, the one which hasObjectFactory
, still not merged), I have these results:BEFORE using rector:
AFTER using rector (and polishing some stuff):
(of course in both cases, tests are green.)
Actually the only deprecation left is:
There is no way with Rector to make it 100% accurate (or I haven't found how 😅)
The rector rules decide automatically if it must use
ObjectFactory
orPersistentProxyObjectFactory
based on if the target class is final and if it is persisted. Based on that, it automatically removes->object()
calls. Actually, I've introduced in my code a methodunproxify()
, and the only thing I have to do to make the tests pass is to remove by hand these calls, impossible to automatize this.I think the way I've incorporated to Foundry is a bit clumsy, I followed their guide and applied the structure they suggest, I'll have to work on this. I don't even know if we should ship this in this repo, but I really think we should ship it somehow, I'm pretty happy with the result :grin:
There are some things that are still needed:
merry christmas!