Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Feat: Remove the most risky fixers from the next version #98

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
OndraM merged 1 commit into main from feature/remove-risky-fixers
May 14, 2024

Conversation

@OndraM
Copy link
Contributor

@OndraM OndraM commented May 13, 2024
edited
Loading

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).

PhpdocToReturnTypeFixer::class,
// Promote constructor properties
// @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/5956
RequireConstructorPropertyPromotionSniff::class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want this? We are already using it everywhere where we have PHP 8 🤔

Copy link
Contributor Author

@OndraM OndraM May 13, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Upgrade config format but with no much required code style changes. For this I will release version 4.0.
  2. 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 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, missed the description. Yea, it makes sense 👍

Copy link
Contributor

@D0L1K D0L1K May 13, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@OndraM OndraM May 13, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@D0L1K D0L1K May 13, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

// @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/6235
PropertyTypeHintSniff::class,

ParameterTypeHintSniff::class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing those two?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, they should be removed from this as well.

@OndraM OndraM force-pushed the feature/remove-risky-fixers branch 2 times, most recently from ccbe5ec to af5bce0 Compare May 13, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jirinovak jirinovak Awaiting requested review from jirinovak

@kdosiodjinud kdosiodjinud Awaiting requested review from kdosiodjinud

@MortalFlesh MortalFlesh Awaiting requested review from MortalFlesh

@hokypierce hokypierce Awaiting requested review from hokypierce

@MarketaSebkova MarketaSebkova Awaiting requested review from MarketaSebkova

1 more reviewer

@D0L1K D0L1K D0L1K left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

AltStyle によって変換されたページ (->オリジナル) /