-
Notifications
You must be signed in to change notification settings - Fork 69
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
Fix multi-currency autoload issue when plugin gets updated #9681
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
b670b85
to
2998a04
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.
Nice work! Code changes look good and test well.
There were just a few comments we should take care of before merging.
Test Instructions
- 🟢 Test: Reproduce the issue and validate fix.
- 🟢 Test: Ensure the issue won't repeat in future upgrades.
Co-authored-by: Hector Lovo <hector.lovo@automattic.com>
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.
The latest changes LGTM
Fixes #9676
Changes proposed in this Pull Request
This issue happens because of the caveat explained in this comment:
woocommerce_get_settings_pages
hook is called and the callback function fails to resolveSettings
.For this reason and to avoid other potential risks, we're moving the multi-currency PHP source files back to their original paths.
This is an intermediary step until we rename the module's namespace and decouple it further from the gateway.
We are also adding a
did_action( 'upgrader_process_complete' )
to prevent the same issue from happening in the future.Testing instructions
Test: Reproduce the issue and validate fix
Note: Remember to install the Query Monitor plugin to be able to see the error on JN.
Noteii: Remember to remove WooPayments and reinstall it from WPORG before repeating the test.
Follow instructions in #9676.
Run
npm run build:release
and upload the contents from/release/woocommerce-payments
fromdevelop
and from this branch to reproduce and later to ensure the issue is fixed.Test: Ensure the issue won't repeat in future upgrades
To ensure the same error won't happen in future releases, we should simulate an autoloaded path change by changing the current plugin and uploading a build from
develop
:npm run build:release
and upload a zip from/release/woocommerce-payments
.develop
, run thebuild:release
script and upload a zip on top of the previous one.did_action( 'upgrader_process_complete' )
from this branch.npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge