-
Notifications
You must be signed in to change notification settings - Fork 14
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 PHP 8.0; Drop 5.6, 7.0 & 7.1 #96
Conversation
42eb3ce
to
6886b36
Compare
Fixes #88 To support PHP 8.0 we have to update PHPUnit: only versions 8 and 9 support PHP 8.0, see https://phpunit.de/supported-versions.html Then we have to update the test suite as required by breaking changes in PHPUnit 8. And this makes the test suite to stop working with older versions of PHP: 5.6, 7.0 and 7.1, so we drop support for these. Anyway these old versions of PHP reached EOL a while ago, see https://en.wikipedia.org/wiki/PHP#Release_history
Added @kavalerov as a reviewer as he may have opinions on dropped runtime support. |
I haven't tested the SDK itself, but change in supported versions looks right to me, yes. |
For visibility - the change in supported versions here is in line with current PHP support |
To get the test suite working with PHP 8.0 we have to upgrade PHPUnit. Only versions 8 and 9 support PHP 8.0, see https://phpunit.de/supported-versions.html Note that there are no changes to the src folder. So this version could work with PHP 5.6, 7.0 and 7.1, but the tests won't pass. Breaking changes in PHPUnit require to update the test suite. So the tests either pass with PHP 8.0 or with the older versions, but not both. PHP 5.6 error, see https://github.com/ably/ably-php/runs/2041202213?check_suite_focus=true
PHP 7.0 error, see https://github.com/ably/ably-php/runs/2041202230?check_suite_focus=true
PHP 7.1 error, see https://github.com/ably/ably-php/runs/2041202249?check_suite_focus=true
While the commit is large, there are only a few changes that repeat through the test suite:
|
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, thanks
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.
Thanks, @jdavid - I've asked @owenpearson to take a look at the phpunit.xml
file as there are alternative element names we can use which avoid non-inclusive terminology. He will follow on with a commit to address this.
Clearly this got overlooked when we renamed the default branch, despite being one of the terms we identified at the time.
Add support for PHP 8.0