-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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: HPOS keeping disabled when the database tables were created via enabling the setting #40466
Conversation
After the fix HPOS will be disabled only if the creation of the tables fail. Additionally, failure to create the tables will be logged.
Test Results SummaryCommit SHA: 98d923f
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @jorgeatorres, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
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.
@Konamiman: This is missing the changelog file. Other than that, it looks good to me. Thank you for working on this!
|
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. 🎉
Submission Review Guidelines:
Changes proposed in this Pull Request:
When the HPOS tables don't exist and are created on the fly by setting the new tables as authoritative (via the dedicated UI tool, or by setting the new tables as authoritative in setting), the setting wasn't changing and the posts table were still authoritative after the tables creation was completed. With the fix, this will be the case only when the creation of the tables fails.
Additionally:
DatabaseUtil::parse_dbdelta_output
is fixed (the returned table names had a(
appended at the end).How to test the changes in this Pull Request:
Happy path
wp wc cot disable
).wp_wc_orders
).wp wc cot enable
).Error path
To test the error case you'll need to do some temporary changes to the code (there's no need to delete the tables this time, just apply these changes while the new tables are authoritative):
DataSyncrhonizer::create_database_tables
(the one that invokesdbdelta
).OrdersTableDataStore::get_database_schema
:Now try to set the new tables as authoritative with
wp wc cot enable
. You'll get a " [Failed] Orders table could not be created" error, the posts table will be authoritative now, and if you check the logs you'll see the following entry: ERROR HPOS tables are missing in the database and couldn't be created. The missing tables are: wp_wc_foobar, wp_wc_fizzbuzz.Changelog entry
Significance
Type
Message
Fix: HPOS keeping disabled when the database tables were created via enabling the setting
Comment