-
Notifications
You must be signed in to change notification settings - Fork 850
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
Support testing against PHP 8 and 8.1. #1209
Conversation
8e76028
to
7351087
Compare
ca140f7
to
95fe0f7
Compare
62ef9ff
to
4b3f53b
Compare
@dcr-stripe will you take a look at this? |
4b3f53b
to
40ae092
Compare
40ae092
to
20bc5a2
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.
This looks good to me overall! Thanks for all your hard work on this Richard! 🎉
(I can't approve since it's my own PR)
fail-fast: false | ||
matrix: | ||
php-version: | ||
- "7.4" |
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.
Should we run this on 8.1 as well?
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.
Yes, I have that in a forthcoming PR. There are some warnings I need to address first.
"php-coveralls/php-coveralls": "^2.1", | ||
"squizlabs/php_codesniffer": "^3.3", | ||
"symfony/process": "~3.4", |
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.
Confirming this was just a stale require?
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.
Good call. I assumed this too, but I was wrong. We actually use it in StripeMock.php
which apparently attempts to boot up stripe-mock if you aren't already running it?
In our CI we always boot up stripe-mock in a separate step, and I always boot stripe-mock up manually, so I went ahead and removed this file, and now the devDependency is unused.
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.
LGTM - good find!
This is awesome, thanks for your work! |
Summary
This PR:
composer.json
to require (as a devDependency) EITHER phpunit 5.7, which is compatible with PHP <=7.2; OR phpunit 9.0, which is compatible with PHP 8.TestCase
class and add some "shims" over top of them.|| 2.17.1
is provided as a fallback so composer install doesn't break on old versions of PHP)fixes #1200 #1199