PHP 8.1 compatibility fixes #3357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wjrosa
changed the title
wjrosa
changed the title
…ps://github.com/woocommerce/woocommerce-gateway-stripe into fix/compatibility-with-php-8-1-and-phpunit-10-5
…ility-with-php-8-1-and-phpunit-10-5
wjrosa
changed the base branch from
develop
to
fix/fix-connection-array-merge-error
wjrosa
requested review from
a team and
diegocurbelo
and removed request for
a team
Base automatically changed from
fix/fix-connection-array-merge-error
to
develop
August 14, 2024 14:15
diegocurbelo
requested changes
Aug 15, 2024
There was a problem hiding this comment.
Thanks for working on the newer PHP fixes @wjrosa!
The code changes look good, and all tests pass*. I left a few comments with questions.
- The E2E checks fail in the PR, but they pass using the same docker env locally.
We already have an issue to fix the flakiness.
tests/e2e/tests/_legacy-experience/woocommerce-blocks/sca-card.spec.js
Outdated
Show resolved
Hide resolved
| @@ -158,11 +158,17 @@ function ( $id ) { | |||
| $ids_to_migrate = $this->get_subs_ids_to_migrate(); | |||
|
|
|||
| $this->logger_mock | |||
| ->expects( $this->at( 1 ) ) | |||
| ->expects( $this->exactly( 2 ) ) | |||
There was a problem hiding this comment.
Why do we need to update these tests? The new update_main_stripe_settings does not log anything 🤷🏼
Do we know where is the additional log entry coming from?
There was a problem hiding this comment.
at() was deprecated in PHPUnit 9. So, it threw many warnings when running the tests. I decided to refactor them here already. It is not a blocker, as we could wait for PHPUnit 10 (which removed this). I can also revert this if that is preferable.
…port payment method settings as well
diegocurbelo
approved these changes
Aug 19, 2024
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2411
Base PR: #3355
Changes proposed in this Pull Request:
This PR fixes issues when running the application on PHP 8.1. Basically, a new CI test node was added with this specific version, and deprecation issues were fixed. Especially calls to
get_option(), as it can returnfalseand it is not possible to automatically convert it to empty arrays anymore.To better solve the issue above, I have centralized the retrieval, update, and deletion of the main Stripe settings option on the
WC_Stripe_Helperclass, and replaced all calls to those functions with the new static methods.Initially, this PR should also add support for running tests with PHPUnit 10.5, but I dropped it since most of the base classes required to run our
wordpress-tests-libwere removed on the major version. So, in order to progress with it we should also work onwordpress-tests-libfirst. There's a long conversation about this issue (3 years) here.Testing instructions
Code review should be enough. Check if the tests are still passing.
changelog.txtandreadme.txt(or does not apply)Post merge