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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX] Depreate heavy and conflicting Symfony/Twig/PHPUnit level sets #5477

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Jan 17, 2024

It's not best practise to use UP_TO_* sets in rector.php longer than is their single-run. They run dozens of rules with very complex logic, as it take to get from Symfony 2 to 7. It's like running a Tesla control and checking all elements available since the first horse ride :)

Also, the tools we build upon - php-parser and PHPStan - traverse tree from top to bottom. That means if Symfony 4.4 changes a class, and then Symfony 3.4 changed a method, the changes would crash with each other.

We published post about this, but we should do more to have a DX that is useful and practical:
https://getrector.com/blog/5-common-mistakes-in-rector-config-and-how-to-avoid-them

The best practice - and we use it this way since the first Symfony rule set - to run those just once and remove 馃憤


Instead use only the latest major sets, to keep up to date and get fast and reliable Rector run 馃殌

Thank you

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
@jackbentley
Copy link
Contributor

This "best practice" is silly. If I'm running rector in CI then I want it to prevent known deprecated code from being introduced.

Changing from UP_TO_SYMFONY_54 to SYMFONY_54 will only find deprecations in that single minor release. Any deprecations in prior minor versions won't be checked. This means any new code which is deprecated in any version prior to the current can unknowingly get introduced.

The only way to make sure this new already deprecated code gets fixed is to run periodic UP_TO_* - which is not good as it will now include more changes than necessary.

Not to mention you've removed not deprecated the UP_TO_* functionality... This is not what deprecated means. If it was deprecated it would still work.

I agree running for example Symfony 3.3 rules on Symfony 5.4 is silly, unnecessary and intensive. You're essentially running fixes for code which is impossible to have implemented. What we actually need is sets which include fixes for all deprecations in the current major versions (if the package follows semver that is). For example, SYMFONY_5 to include 51, 52, 53 and 54 sets.

@TomasVotruba
Copy link
Member Author

For example, SYMFONY_5 to include 51, 52, 53 and 54 sets.

I like this option 馃憤
How can we implement it to keep easy to maintain and setup in rector config?

@jackbentley
Copy link
Contributor

jackbentley commented Feb 1, 2024

If you're able to use glob patterns (or can enable it to be allowed) then you could set it up so the constant is pointed to all the sets in that version - for example, final public const SYMFONY_5 = __DIR__ . '/../../config/sets/symfony/symfony5*.php'; here https://github.com/rectorphp/rector-symfony/blob/main/src/Set/SymfonySetList.php

Worth mentioning an edge case might be someone running Symfony 6.1 when Symfony 6.2 is available with rectors. The rectors may add code which won't function. However, I'd expect in most cases people will be upgrading to the latest minor version at the same time as when they upgrade rector. Regardless such a feature is useful for running older versions like Symfony 5.4.

@samsonasik
Copy link
Member

RectorConfig->import() no longer support fnmatch, see

if (str_contains($filePath, '*')) {
throw new ShouldNotHappenException(
'Matching file paths by using glob-patterns is no longer supported. Use specific file path instead.'
);
}

so if that needed, the collection of explicit paths need to be registered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants