feat: introduce rector rules 🎉

[nikophil]

Hi!

here is my present for christmas 🎁 🎅

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 has ObjectFactory, still not merged), I have these results:

BEFORE using rector:

PhpUnit: Remaining direct deprecation notices (19782)
PhpStan: Found 85 errors

AFTER using rector (and polishing some stuff):

PhpUnit: Remaining direct deprecation notices (8)
PhpStan: Found 0 errors

(of course in both cases, tests are green.)

Actually the only deprecation left is:

Calling instance method "Zenstruck\Foundry\Object\Instantiator::withoutConstructor()"

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 or PersistentProxyObjectFactory 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 method unproxify(), 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 😁

There are some things that are still needed:

• create a rector "set" with the list of the rules. ✅
• And add an "how to" in the upgrade docs
• add a way to read Symfony's config to be able to detect doctrine's mapping which have not been create with attributes ✅
• I don't think I'll bother to create a rector for the bundle's configuration: it's not so boring to do by hand in userland

merry christmas!

[OskarStark]

I am for shipping it in the repo 👍🏻

[nikophil]

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:

• currently in Foundry, we're using 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.
• if we ship this directly with Foundry, since we won't add 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.

[OskarStark]

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');
};

[nikophil]

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.

[OskarStark]

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?

[nikophil]

rectorphp/rector#8379

[nikophil]

actually it is more complex from user's POV to leverage phpstan's configuration than to override the service definition

see https://github.com/zenstruck/foundry/pull/544/files#diff-57df83b36e5e3d69ad38354b8047370ad4841d60ce901ee2d5020a3a28784a02

[nikophil]

rectorphp/rector#8377

[nikophil]

this rector currently only transforms ModelFactory into ObjectFactory or PersistentProxyObjectFactory

I'm wondering how it would be useful to allow to transform intro PersistentObjectFactory

[nikophil]

I just added a RuleRequirementChecker using \Composer\InstalledVersions

[kbond]

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?

[nikophil]

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