Rector does not instantly upgrade PHP code

[sylbru]

Sorry to be this person, but I find it misleading and dangerous to advertise Rector as a way to "instantly upgrade PHP." It’s not that simple, sadly.

I’m in the process of upgrading to PHP 8.0. The rule set contains both rules for breaking changes (renamed functions…) and for new features (match…). When people need to upgrade PHP though, it means they need, at first, to fix breaking changes. New features should not, in my opinion, be put on the same level.

But what’s serious here is that running Rector with the PHP 8.0 ruleset will potentially leave quite a bit of incompatible code. Things like nested ternaries, unparenthesized expressions with +/- and .… And I saw no warning about that on the website or anywhere. Some of these are probably impossible to fix using the current approach (it looks like parens are not part of the AST). But others are just missing (removed mbregex aliases…).

Perhaps even more importantly, some rules can introduce bugs where there wouldn’t have been any. An example is ChangeSwitchToMatchRector, which doesn’t seem to care about the fact that match does a strict comparison where switch doesn’t. I think this should be mentioned somewhere, so that people don’t assume everything’s gonna be fine.

So yeah, just thought I should mention that I found some of these claims on the website to be a little dishonest.
That said, I have to say that Rector is already proving super helpful in my upgrade (PHP and libraries). I hope this message won’t come across as too agressive. Thanks for your work!

[TomasVotruba]

Hi, thanks for sharing your view. And for your patience, even though it seems it's very stresful to write these lines.

There are some saint defaults, that can lead to edge-case lake you describe. They're here to avoid choice paralysis. I agree with you, it require some knowledge of PHP to be aware what Rector rule exactly does.

Rector is here to help with upgrades that you'd do manually otherwise. That's where it can help the most as a tool. It works in limits of PHP language itself. The developer still needs to decide, test, validate and check. Same applied for coding standard tools, that can break the PHP code as well. It's a an extension tool for developer.

A tool is as good as the code-base itself - tests, PHPStan level 8 and coding standard helps. I wrote about relation to other tools and repository setup here. It might clear some questions about the scope Rector works in:
https://tomasvotruba.com/blog/2019/12/16/8-steps-you-can-make-before-huge-upgrade-to-make-it-faster-cheaper-and-more-stable

I personally use ChangeSwitchToMatchRector only in a standalone PR, for reasons you've described. The same process I'd do manually.

Still, I hope you'll give Rector a second a try and make use of it mainly in CI long-term usage 🙂

[sylbru]

Thanks for your answer. I understand your stance, even though I would have drawn the line somewhere else, trying to aim for safe defaults. But even this would not have been possible because a lot of these breaking changes are impossible to fix automatically. It’s just the way the whole thing is advertised ("upgrade to PHP8 in a single CLI command") that bothers me a little bit.

But yeah, tests and static analysis and coding standards are of huge help here. Maybe it should be written somewhere as a disclaimer.

I’m more than giving it a second try, I’m using it successfully to check for particular code usages while upgrading to PHP 8/Slim 4/CycleORM 2! 🙂 However I’m not sure in which situations I would use it in CI, I would need to have only strictly safe rules there.