-
Notifications
You must be signed in to change notification settings - Fork 354
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
Switch to GH Actions #670
Switch to GH Actions #670
Conversation
689f9cd
to
0818ffe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to include #655, maybe better merge it first?
@guilliamxavier thanks, did not see that one. |
OK I think for me this is ready to merge now. As a next step we could increase the phpstan level but I wanted to keep it low for now so this is easy to cherry-pick into the 5.x.x branch, so we get at least some basic static analysis going there, and can do the level 8 stuff only on master branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fairly sane, although worth noting that I'm not overly familiar with deploying GH actions, so if there are problems in this, it's entirely possible I may have missed them.
composer.json
Outdated
@@ -32,7 +32,7 @@ | |||
"icecave/parity": "1.0.0" | |||
}, | |||
"require-dev": { | |||
"friendsofphp/php-cs-fixer": "~2.2.20||~2.15.1", | |||
"friendsofphp/php-cs-fixer": "~2.2.20 || ^2.15.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was pinned due to an issue with newer minor versions of php-cs-fixer rewriting code in a way that actually caused logic changes, without warning. I would prefer to continue with pinning specific versions if possible, as this happened on multiple occasions with different versions of php-cs-fixer.
Is there are strong reason that requires loosening this constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason was allowing it to install fine on PHP 8 in development, because 2.15 requires php 5/7. But I'm happy to pin it to 2.19 instead to avoid surprises in the future.
Thanks for the review, merging this then, which should hopefully help getting future PRs checked more easily. |
Switch to GH Actions
Done in #693 |
PHP 8 is still not built because it'd require major test suite refactorings to get it to work (as we can't run phpunit 4.x on PHP 8).