Feat: Remove the most risky fixers from the next version #98
Conversation
OndraM
requested review from
jirinovak,
D0L1K,
kdosiodjinud,
MortalFlesh,
hokypierce and
MarketaSebkova
| PhpdocToReturnTypeFixer::class, | ||
| // Promote constructor properties | ||
| // @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/5956 | ||
| RequireConstructorPropertyPromotionSniff::class, |
There was a problem hiding this comment.
Why don't we want this? We are already using it everywhere where we have PHP 8 🤔
There was a problem hiding this comment.
See PR description - we want this, but I think for some teams may be preferred to have the ability to easily split the upgrade into two phases:
- Upgrade config format but with no much required code style changes. For this I will release version 4.0.
- Upgrade code to comply to all new shiny fixers. For this I will release version 4.1, immediately after 4.0.
The most agile teams will upgrade directly to 4.1 and get all those fixers. The conservative teams or teams with bigger codebase will have option to upgrade to 4.0 first, and once they are cleared, they can continue with upgrade to 4.1, which may be more difficult, because the version will contain tens of new fixers, see #95 .
There was a problem hiding this comment.
Oh sorry, missed the description. Yea, it makes sense 👍
There was a problem hiding this comment.
But wait, I've just realised - those checks are already present in 3.3.x under ecs-8.0.php. So teams using PHP 8.0+ with those rules are already using them and if we remove them, it will create only more mess in the codebase. And if not, they can start with using them as step 1 and afterwards just switch to 4.0.0 and edit configs as step 2.
I just don't think removing of rules which are already used in some projects is a good idea tbh
There was a problem hiding this comment.
If we remove these checks, nothing will happen. The rules will just not be checked, but no change in code will be triggered. So I don't see a risk of creating any mess in the codebase.
Also, if you already use the 8.0 checks, you can simply upgrade to 4.1, right?
These rules were only "optional", they were not part of the default setup, because you have to manually require the php-8.0.php file on top of the ecs.php. Because of this, I see them as "newly added default checks", because they are now "forced". So I am trying to be a bit conservative in this... Also those checks are risky, they can change the code behavior, so I want people to be really cautions when they will be adding them.
There was a problem hiding this comment.
Yea I know, what I've meant by that is that the code, which is not valid in 3.3 with 8.0 rules used and which also will not be valid in 4.1, will be valid in 4.0. So that can create a situation where some code will be formated by those rules from the previous version and some not when 4.0 is used, which will result in more fixes with 4.1.
However, as far as 4.1 will be released with 4.0 at the same time I'm ok with it :)
|
|
||
| // switch -> match | ||
| // @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/5894 | ||
|
|
||
| // Require \Stringable interface in classes implementing __toString() method | ||
| // > it may probably be a phpstan rule, more than cs rule - since it needs a class hierarchy to solve this | ||
| // @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/6235 | ||
| PropertyTypeHintSniff::class, | ||
|
|
||
| ParameterTypeHintSniff::class, |
There was a problem hiding this comment.
Why are we removing those two?
There was a problem hiding this comment.
And if we rly don't want them, maybe we should also remove
Line 521 in af892d7
There was a problem hiding this comment.
True, they should be removed from this as well.
OndraM
force-pushed
the
feature/remove-risky-fixers
branch
2 times, most recently
from
ccbe5ec
to
af5bce0
Compare
…dded in the next release)
OndraM
force-pushed
the
feature/remove-risky-fixers
branch
from
af5bce0
to
26ec573
Compare
Those fixers will not be included in next release (4.0), but will be readded as part of #95 targeted to version 4.1.
This is to provide easier upgrade path for the products which would like to make more conservative two-phases upgrade (first to 4.0 mainly to change the config, then to 4.1 to fix more code issues).