feat: Add trailing comma in multiline to PER-CS 2.0

[michaelvickersuk]

As per https://www.php-fig.org/per/coding-style/#26-trailing-commas

Numerous PHP constructs allow a sequence of values to be separated by a comma, and the final item may have an optional comma ... If the list is split across multiple lines, then the last item MUST have a trailing comma.

Closes #7664

[coveralls]

coverage: 96.036% (+0.001%) from 96.035%
when pulling 620a199 on michaelvickersuk:trailing-comma-in-multiline-in-per-cs-v2
into 9782155 on PHP-CS-Fixer:master.

[michaelvickersuk]

I'll need to take some advice on how's best to pass the PHP 7.4 tests, will it be simply a case of removing the parameters and match elements or can we somehow still include them when PHP < 8.0?

[julienfalque]

As I understand, the PER rule covers any kind of multiline list that can have a comma after the last item. I think it would be nice to rework trailing_comma_in_multiline in the future so it can fix everything without the need to configure every possible kind of list, by default or with e.g. option ['elements' => '@all'].

What do you think @Wirone @keradus?

[keradus]

PER rule covers any kind of multiline list

I'm not sure. eg, it mentions function arguments in function declaration, but not function call params.

I'm +1 to have @all available.
I would be careful with @all by default, as it can easily create bc breaks in someone CS.

[julienfalque]

LGTM 👍

@keradus Do you want to check again before merging?

[keradus]

Do you want to check again before merging?

I don't mind, but as you poked me it got my attention anyway.

old concerns of mine are solved 👍🏻
i found 2 new.

[julienfalque]

Thanks @michaelvickersuk!

[raveren]

This is an another (see #6933) BC break in a minor version.

My config:

return (new PhpCsFixer\Config())->setRules([
    '@PER-CS'                           => true,
    'not_operator_with_successor_space' => true,
    'no_break_comment'                  => false,
])
    ->setFinder($finder)
    ->setCacheFile(__DIR__ . '/storage/.php-cs-fixer.cache')
;

Now I have 5% of my codebase "breaking" thanks to this PR.

[kubawerlos]

This is an another (see #6933) BC break in a minor version.

@raveren what the h. do you mean? How adding new fixer is BC break?

[raveren]

What I mean is, people set up CI pipelines to analyze and deploy their code. (P.S. why else do people use php-cs-fixer for?)

Someone in the team does an innocent composer update and CI pipeline is broken because 5% of the codebase is now not passing the cs-fixer step.

I meant to reference issue #6658 in my original comment, it's the second time in less than two years that this scenario happens.

[kubawerlos]

Someone in the team does an innocent composer update and CI pipeline is broken

Sound to me like the problem is not on PHP CS Fixer side 😉. Maybe you need to have constraint like ~3.52.0.

Also, similarly to #6658, the same reasoning @keradus made there, according to https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/feature-or-bug.rst#feature this is no BC break, but more of a feature, and I agree with it.

You are using PER and E stands for evolving, and in this case it simply evolved.

[Wirone]

@raveren You need to decide whether you follow the PER-CS standard or not. In fact, there is #4502 and #7247 that, when implemented, will additionally change the scope of that rulesets. You have 3 options:

• lock Fixer version to keep changes under control
• accept that these rulesets are evolving and may be modified according to the specs that are not covered fully now
• prepare your own ruleset, with 100% explicit config of rules with options (rules are under BC promise, rulesets not)

Thank you for understanding and in the future, please consider interacting without accusations.

[raveren]

Maybe you need to have constraint like ~3.52.0.

The readme states this project is using SemVer

so it's wiser to take in nice feedback from your patient users, accumulate that knowledge and hopefully it will someday fruit into a professional BC policy for the project

[raveren]

Replying to @Wirone: in that case maybe it would be nice to notify your users on the website (https://cs.symfony.com/doc/ruleSets/PER-CS.html) that the rulesets can and will change under their noses, it's a VERY important consideration when building a CI pipeline.

[raveren]

I think this info is hugely important:

rules are under BC promise, rulesets not

I can't find that stated anywhere, and that already constitutes a constructive and valid BC policy.

But users need to know that to prevent such unpleasant surprises in the future.

[Wirone]

replying to @Wirone in that case maybe it would be nice to notify your users on the website (cs.symfony.com/doc/ruleSets/PER-CS.html) that the rulesets can and will change under their noses, and thus are not advised to be used in CI pipelines?

Why would it be not advised? If you follow pre-defined ruleset, you agree to its rules. It's not a problem of the tool, it's the problem of your CI pipeline and development process. There is nothing like "someone in the team does an innocent composer update" - if there was Composer update and Fixer's version was bumped, developer who did the update should apply CS rules, so nothing fails in CI. If applied changes are not wanted, then your own config should be improved to continue with what you had before (override rulesets' config with custom config for specific rules).

I can't find that stated anywhere, and that already constitutes a constructive and valid BC policy.
But users need to know that to prevent such unpleasant surprises in the future.

I don't remember now if we have such explicit information about rulesets, but I always thought it's obvious. Imagine bumping major version after every ruleset modification, or adding more and more clones of existing rulesets 🤷. You want stable ruleset? Prepare your own.

[raveren]

I always thought it's obvious.

well I can vouch to you that it is not :)

P.S. It's made especially difficult since php-cs-fixer can't be made compatible with phpstorm's code formatter, and to fix each new rule by cs-fixer I need to come up with new and inventive ways to format edge cases that work for both :) espacially in dust filled legacy corners of the application.

Here's an example: the only well looking formatting for this case accepted by both:

                $line =
                    [
                        'key'     => $value,
                        'key2'    => $value2,
                    ]
                    + $line;

Not your fault, just giving feedback, on what real world is like for your users.

[Wirone]

P.S. It's made especially difficult since php-cs-fixer can't be made compatible with phpstorm's code formatter, and to fix each new rule by cs-fixer I need to come up with new and inventive ways to format edge cases that work for both :)

It's not true. You can set up external formatter and apply Fixer changes in real-time. I use it for developing Fixer:

It either applies the rules automatically, or underlines code that violates the standard and provides controls to apply fixes:

It's possible to make it consistent with CI and fully automated (with actions executed on commit).

[raveren]

Ah, yeah it's true.

However php-cs-fixer has no "killer" features above the PHPStorm one, and there are important things that cs-fixer does not support (e.g.: import cleanup, notably max line length, etc). Plus: the inbuilt formatter is simpler to set up and use, which is especially relevant when we're talking about a rotating team of developers on different platforms: some might not even have PHP running locally.

[kubawerlos]

import cleanup

can you elaborate how import cleanup is not supported?

max line length

that's by design, because it can never be fully supported

[raveren]

Import cleanup is sorting and removing the unused use statements, it's a minor thing, but then again each individual rule is.

[Wirone]

Import cleanup is sorting and removing the unused use statements, it's a minor thing, but then again each individual rule is.

You mean this? 🤷‍♂️

Plus: the inbuilt formatter is simpler to set up and use

How's sharing parts of .idea config and/or formatter config that can be imported to IDE easier than shared config for CS Fixer? I've been using Fixer or ECS for years now, automated locally and in central CI and never had problems with conflicts between PHPStorm's built-in formatter and the external tooling. I can't be sure, but maybe you have invalid setup that leads to these kind of problems?

which is especially relevant when we're talking about a rotating team of developers on different platforms: some might not even have PHP running locally.

Set up Docker stack and configure remote PHP interpreter within shared .idea config, so everyone can run everything on the same runtime - problem solved. Anyway, what's the point in working with the PHP code and doing changes, when you don't have PHP runtime and can't run CS fixes and tests?

[raveren]

Hey, thanks for trying, but this is not a conversation about options for my tech stack, I am simply giving you a real world situation as feedback from your users so you have a better idea.

Besides, no hard feelings, but the number one reason I wouldn't base so much of a business on this project is because it broke its SemVer (and my trust) twice and the support response both times was "no we didn't! 👺".

[Wirone]

Besides, no hard feelings, but the number one reason I wouldn't base so much of a business on this project is because it broke its SemVer (and my trust) twice and the support response both times was "no we didn't! 👺".

Modifying built-in rulesets is not a SemVer violation, as the engine (public API) does not change and each specific rule also works the same. The only thing that changes is the scope of the ruleset, which you implicitly agree on by opting-in when you base your setup on such ruleset. It's like complaining that PHPStan reports new issues when you have bleeding edge enabled. Sorry if it was not clear, we'll try to add such information in the docs. However, stating that Fixer violates SemVer after all the effort we put into keeping backward compatibility is just rude.

[julienfalque]

just giving feedback, on what real world is like for your users

I mean, yeah, none of the maintainers here has a "real world" job where they could use the thing they build and see how it actually works 🤷

Also, even a little bugfix can break a CI. This is true for any package you might rely on but especially for us. Like e.g. a bug that prevents a rule from fixing code in an edge-case that happens to exist in your codebase. Fixing that bug will make your CI fail too, this does't mean we should make a major release each time we fix a bug. So please, pin the versions of your tools, check what happens when you do upgrade them, and be nice when reporting actual issues. Thanks.