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

ci: Migrate to PHP-CS-Fixer #230

Merged
merged 9 commits into from
Mar 30, 2024
Merged

ci: Migrate to PHP-CS-Fixer #230

merged 9 commits into from
Mar 30, 2024

Conversation

smnandre
Copy link
Contributor

#229

✅ get rid of PHP-CS
✅ install PHP-CS-Fixer instead
✅ make sure PHP-CS-Fixer runs in GitHub Actions
✅ keep code changes minimal (avoid reformatting as much as possible) - this means ensuring that the PHP-CS-Fixer configuration matches our PHP-CS configuration closely)

✅ PSR-12 standard
✅ NO yoda condition

In the following steps we will see for these ones, but i guess we already have a significative change in hands with this small PR :)

  • preventing backtick syntax
  • enforcing long open tag (<?php)
  • enforcing usage of primary functions / preventing usage of function aliases (e.g. count() vs sizeof())

Have a good day :)

@meyfa
Copy link
Owner

meyfa commented Mar 30, 2024

Amazing, great stuff!

It seems rector.php is causing an error, but the error isn't shown in the logs - do you know what this is about?

@smnandre
Copy link
Contributor Author

I need to take a look i had none locally (but in 8.3)

@smnandre
Copy link
Contributor Author

That's it

    ->withPhpSets(php74: true)

Named arguments are not supported before 8.0. Let's exclude this file for now :)

Copy link
Owner

@meyfa meyfa left a comment

Choose a reason for hiding this comment

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

LGTM

@smnandre
Copy link
Contributor Author

That's still red :( Did not have time to find a fix

@meyfa
Copy link
Owner

meyfa commented Mar 30, 2024

I think we can get rid of the parameter in rector.php and it will determine it based on composer.json. That way, PHP 8 syntax is gone and CI can pass.

@smnandre
Copy link
Contributor Author

I think we can get rid of the parameter in rector.php and it will determine it based on composer.json. That way, PHP 8 syntax is gone and CI can pass.

That'd be perfect, nice!

@meyfa meyfa merged commit cd666e3 into meyfa:main Mar 30, 2024
7 checks passed
@smnandre
Copy link
Contributor Author

oh sorry... @meyfa ! Iade the first step on PR... next time every commuit 😅

@meyfa
Copy link
Owner

meyfa commented Mar 31, 2024

Don't worry about it. This repo is configured to always squash commits on merge.

Thanks for another great contribution!

meyfa added a commit that referenced this pull request Mar 31, 2024
meyfa added a commit that referenced this pull request Mar 31, 2024
@smnandre
Copy link
Contributor Author

Happy to help :)

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.

2 participants