-
Notifications
You must be signed in to change notification settings - Fork 35
Add PHP 8.5 support #137
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
base: master
Are you sure you want to change the base?
Add PHP 8.5 support #137
Conversation
Summary of ChangesHello @wireblue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to extend the project's compatibility to PHP 8.5, allowing it to run on the latest stable PHP version. While the core change involves a simple update to the "composer.json" file, the author has proactively highlighted a significant deprecation warning related to "PDO::MYSQL_ATTR_SSL_CA" in PHP 8.5, which currently impacts testing environments. The PR also opens a dialogue about the appropriate release strategy for this update, considering its potential implications and dependencies on external fixes. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds support for PHP 8.5 by updating the php version constraint in composer.json. While keeping up with new PHP versions is important, this change is premature as it causes the test suite to fail, an issue you've already identified in the pull request description. My review focuses on the importance of a stable, passing test suite before merging such changes.
| ], | ||
| "require": { | ||
| "php": "^8.2|^8.3|^8.4", | ||
| "php": "^8.2|^8.3|^8.4|^8.5", |
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.
While adding support for PHP 8.5 is a good goal, this change should not be merged until the test suite passes for all supported PHP versions, including 8.5. As you've noted in the description, this change currently causes tests to fail due to deprecation warnings from dependencies. It's crucial to maintain a green build to ensure the stability and reliability of the package. I'd recommend marking this PR as a draft until the underlying dependency issues are resolved and the CI is green.
This PR adds PHP 8.5 support.
Known Issues
PHP 8.5 has deprecated the
PDO::MYSQL_ATTR_SSL_CAconstant. Instead it should be replaced with\Pdo\Mysql::ATTR_SSL_CA.This issue is found in Laravel's
config/database.phpfile, which is also present inorchestral/testbench-core. Currently all tests are failing with deprecation warnings because of this. If you openvendor/orchestra/testbench-core/laravel/config/database.phpand comment out those deprecated lines, the tests pass.I can see Testbench and Laravel are aware of the issue, and it seems they're waiting on a polyfill from Symfony to be merged in.
Breaking change?
In my original issue it was suggested this PHP upgrade could be bundled with #136 to form a new major release. If the
PDO::MYSQL_ATTR_SSL_CAdeprecation issues aren't solved in a timely manner perhaps it would make sense to release the xero-php-oauth2 upgrades first, and the PHP bump can be released later as a minor release.