-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
PHP 8 Support #2017
PHP 8 Support #2017
Conversation
origin script: php-webdriver/php-webdriver
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
/cc @TeBoring for this approach to patching About 3% of Wordpress runs on PHP 5.4 and 5.5. These calls likely make up only a fraction of the users of this package. The percentage using Composer is even smaller. I will do some data mining to verify the number of calls currently being made to our APIs from these PHP versions to affirm this is acceptable, but I suspect it will be. @Nielsvanpach This is a great PR and I appreciate your contribution! If you'd like, you can remove PHP 5.4 and 5.5 from the asset release and test workflows in this PR as well, and update Dropping support for PHP 5.4 and 5.5 will not require a major version bump. As updating minimum PHP requirement ensures existing installations on PHP 5.4 and 5.5 will not break. |
@bshaffer 5.4 and 5.5 are removed from the workflows and composer.json. All test pass now! |
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.
Just one issue with the whitespace and we can merge!
@@ -11,14 +11,14 @@ The Google API Client Library enables you to work with Google APIs such as Gmail | |||
|
|||
These client libraries are officially supported by Google. However, the libraries are considered complete and are in maintenance mode. This means that we will address critical bugs and security issues but will not add any new features. | |||
|
|||
**NOTE** The actively maintained (v2) version of this client requires PHP 5.4 or above. If you require support for PHP 5.2 or 5.3, use the v1 branch. | |||
**NOTE** The actively maintained (v2) version of this client requires PHP 5.6 or above. If you require support for PHP 5.2 or 5.3, use the v1 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.
I think you're missing a note for the last version that supports PHP 5.4 / 5.5
Hi there, I've made some adjustments to make this package work with PHP 8!
However, we need a consensus on the supported php versions before all tests can pass.
Supported PHP Versions
For PHP versions < 7.2 I've included a script for making the tests PHPUnit ^6 compatible. Thanks to php-webdriver for this approach: https://github.com/php-webdriver/php-webdriver/pull/828/files#
PHP 5.4 and 5.5 are both still supported now. At the moment they are 4 and 5 years EOL. With the release of PHP 8 last month, I believe it might be time to let them go in the latest version (with PHP 8 support).
You can see the test results on my fork: https://github.com/Nielsvanpach/google-api-php-client/actions/runs/421025986
Everything passed, except 5.4 and 5.5. As I already said, I suggest to drop support on them. I don't think it's really useful to spend time making the test suite 5.4 and 5.5 compatible.