Skip to content
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

Introduce PHP-CS-Fixer #613

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Wirone
Copy link
Contributor

@Wirone Wirone commented Dec 1, 2023

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
Copy link
Member

stof commented Dec 1, 2023

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
Copy link
Contributor Author

Wirone commented Dec 1, 2023

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

@Wirone
Copy link
Contributor Author

Wirone commented Dec 1, 2023

@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
Copy link
Contributor Author

Wirone commented Dec 1, 2023

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

@Wirone
Copy link
Contributor Author

Wirone commented Dec 1, 2023

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

@stof
Copy link
Member

stof commented Dec 1, 2023

@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
Copy link
Contributor Author

Wirone commented Dec 1, 2023

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

@Wirone
Copy link
Contributor Author

Wirone commented Dec 4, 2023

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

composer.json Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@Wirone Wirone requested a review from stof December 4, 2023 15:48
@Wirone

This comment was marked as outdated.

@Wirone

This comment was marked as outdated.

@ciaranmcnulty
Copy link
Member

@stof You now have repo admin rights

@ciaranmcnulty
Copy link
Member

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

Shim version was used in order to minimise impact of dev dependencies. Config files may look like there are missing classes, but it works.
This is required because Prophecy supports PHP 7.2+ while Fixer requires PHP >=7.4, so it's failing in the CI.
…tests in the CI

Fixer is needed only in dedicated job, which runs on PHP 8.2 so dependencies can be resolved. For running tests it;s not required at all, so even if it's not compatible only with 7.2 and 7.3 we can remove it for any PHP version in the tests' matrix.
@Wirone
Copy link
Contributor Author

Wirone commented Mar 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants