Introduce PHP-CS-Fixer

[Wirone]

I did not apply suggested changes because I want to get feedback if you like these changes or not. We can tweak configuration in order to keep more of current conventions (like class instantiation without parentheses).

[stof]

This breaks testing on PHP 7.2 because of incompatible dependencies. This cannot be accepted as is.

Also, it would be great to start with a php-cs-fixer that is a bit closer to our existing setup. For instance, I'm quite sure that the current codebase is mostly using concatenation without spaces (i.e. the @Symfony rule for them), and not having the explicit visibility in phpspec specs is following the official phpspec examples. This would make it easier to review the changes (we can have a dedicated migration after that if we decide to change the coding standards).

[Wirone]

Yeah, I'll try to figure out how to do it properly, keeping support for PHP 7.2+ 👍.

[Wirone]

@stof in terms of Fixer configuration, there are rules that cause diff regardless of the config, because the codebase is not consistent:

• new_with_parentheses, there are both new Foo; and new Foo();
  • 21 files changed with added explicit ()
  • 34 files changed with removed ()
• concat_space, there are both 'a' . 'b' and 'a'.'b'
  • 10 files changed from 1 space to none
  • 18 files changed from none space to 1
• braces_position.control_structures_opening_brace, there are if statements with { in both same and and new line
  • 17 files changed with { moved from new line to same line
  • 71 files changed with next_line_unless_newline_at_signature_end strategy ({ moved to new line)
• control_structure_continuation_position.position, there are else in the next line currently, but if I set next_line strategy, then catches land in new line..
  • 3 files changed with elseif moved to same line as }
  • 14 files changed with next_line strategy

So basically each option have tradeoff and it needs to be decided what to do with each. I've now configured Fixer to use options that causes as little changes as possible (66 files changed instead of 118 as before). Let's see if it's acceptable 🙂.

[Wirone]

I was thinking about adding --no-scripts many times today and in the end I forgot 😆.

[Wirone]

@stof it should be OK now (not counting CS) 🙂. Waiting for workflow approval is a frustrating experience for first-time contributors 😅.

[stof]

@Wirone I understand that and I generally change that setting to require it only for new github accounts instead of the github default value. But I don't have admin rights on the repo /cc @ciaranmcnulty

[Wirone]

FYI: PHPStan errors are fixed in #612, so maybe we should finish that one first.

[Wirone]

@stof rebased and conflicts resolved, ready for review and deciding about the Fixer's ruleset 🙂.

[Wirone]

@stof I can't reach @ciaranmcnulty (no response for my messages), do you have enough power here so we can agree on final ruleset and just apply fixes, or we need to wait for Ciaran's decision? I would like to continue to contribute, but having sane QA automation 😉.

[Wirone]

@stof @ciaranmcnulty friendly reminder 😉.

[ciaranmcnulty]

@stof You now have repo admin rights

[ciaranmcnulty]

@Wirone can you add a commit accepting the changes so we can see if there's anything crazy, easily?

[Wirone]

@ciaranmcnulty Of course, here are the applied fixes 🙂.

[Wirone]

@stof I've rebased the branch, re-applied coding standards for new code (amended previous commit), and added simple change to the Fixer config (disabled trailing_comma_in_multiline as it tries to apply changes not compatible with PHP 7.x (info in the last commit).

[stof]

Instead of disabling trailing_comma_in_multiline entirely, you should keep the rule enabled by only for array.

[Wirone]

@stof I wanted to do it initially, but as I wrote in the commit's message, it's against current coding standard in the project. But sure, I can do it if you want - it will add 15 commas across the code base. Do you want me to do it?

[stof]

@Wirone I'm quite sure the current state of the code is a mixed state (due to not having tooling enforcing it) rather than a consistent state of not using trailing comma in multiline array.