sulu / skeleton

Project template for starting your new project based on the Sulu content management system
https://sulu.io
MIT License
261 stars 81 forks source link

Upgrade rector to 0.18 version #227

Closed alexander-schranz closed 1 year ago

alexander-schranz commented 1 year ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets fixes #issuenum
Related issues/PRs #issuenum
License MIT
Documentation PR sulu/sulu-docs#prnum

What's in this PR?

Upgrade rector to 0.18 version.

Why?

Newer version to make upgrades better.

ratza commented 1 year ago

I think the missing/non-magically loaded PHPStan extension configuration should be added to the Rector configuration, as well, using the feature you implemented in Rector (thanks, btw!).

In my case, this solves the failures that are also reported by the CI pipeline above:

    $rectorConfig->phpstanConfigs([
        __DIR__ . '/phpstan.neon',
        'vendor/phpstan/phpstan-doctrine/extension.neon',
        'vendor/phpstan/phpstan-symfony/extension.neon',
    ]);
alexander-schranz commented 1 year ago

@ratza thx for the hint. I'm still not sure what extension we should load or not 🤔 Do you have any experience yet with it?

ratza commented 1 year ago

@alexander-schranz not really used to PHPStan tbh (in the Psalm camp before the Sulu skeleton), so I'm yet to discover the use of those extensions.

The way I understand it, we'd (possibly) need them as soon as we enable any of the Symfony/Doctrine specific Rector rules, since they'd help Rector fetch more type information from the DIC and Doctrine's mapping data. Unless it affects performance of Rector, feels like better type inference wouldn't hurt, right?

One thing is certain though, if you want to keep this config in phpstan.neon:

    doctrine:
        objectManagerLoader: tests/phpstan/object-manager.php
    symfony:
        containerXmlPath: %currentWorkingDirectory%/var/cache/website/dev/App_KernelDevDebugContainer.xml
        consoleApplicationLoader: tests/phpstan/console-application.php

and load that same PHPStan config in Rector config (as opposed to separate .neon file), we need to load the extension config as well, otherwise the phpstan.neon config is considered invalid when running Rector.

BTW, I suspect there's currently a slight difference between the lists of extensions loaded by PHPStan vs Rector. For example Rector is likely not loading phpstan/phpstan-webmozart-assert and phpstan/phpstan-phpunit.

alexander-schranz commented 1 year ago

For example Rector is likely not loading phpstan/phpstan-webmozart-assert and phpstan/phpstan-phpunit.

Yeah I understand rector point of not longer loading all extensions as it is challenging to keep rector uptodate with phpstan core container changes. But if I understand rector correctly it will not longer be possible to understand maybe all types now in all cases but not sure 🤔 .

Will need to check if e.g. a rename method rector would work on some code like this:

function (mixed $object) {
    Assert::isInstanceOf($object, \Our\Class::class);

    $object->methodToRename();
}