Migrate all of Woo cron jobs to use action scheduler #59325
Conversation
Testing GuidelinesHi @opr , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
📝 WalkthroughWalkthroughThe changes migrate WooCommerce's background job scheduling from WordPress cron to Action Scheduler for tasks such as session cleanup, unpaid order cancellation, sales events, and log cleanup. Related scheduling logic is refactored, new filter hooks are introduced, and test coverage for legacy cron jobs is removed. Uninstall routines now clear both cron and Action Scheduler jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant WooCommerce
participant ActionScheduler
participant WordPressCron
WooCommerce->>ActionScheduler: register_recurring_actions()
alt Action Scheduler available
ActionScheduler-->>ActionScheduler: Schedule recurring actions (sales, cleanup, etc.)
ActionScheduler-->>ActionScheduler: Schedule single actions (cancel unpaid orders, etc.)
else
WooCommerce->>WordPressCron: Fallback to wp_schedule_single_event/wp_clear_scheduled_hook
end
sequenceDiagram
participant UninstallScript
participant ActionScheduler
participant WordPressCron
UninstallScript->>WordPressCron: wp_clear_scheduled_hook (for WooCommerce hooks)
alt Action Scheduler available
UninstallScript->>ActionScheduler: as_unschedule_all_actions (for WooCommerce hooks)
end
sequenceDiagram
participant wc_format_option_hold_stock_minutes
participant ActionScheduler
participant WordPressCron
wc_format_option_hold_stock_minutes->>ActionScheduler: as_unschedule_all_actions (if available)
alt Action Scheduler available
wc_format_option_hold_stock_minutes->>ActionScheduler: as_schedule_single_action (cancel unpaid orders)
else
wc_format_option_hold_stock_minutes->>WordPressCron: wp_schedule_single_event (cancel unpaid orders)
end
sequenceDiagram
participant wc_cancel_unpaid_orders
participant ActionScheduler
participant WordPressCron
wc_cancel_unpaid_orders->>ActionScheduler: as_unschedule_all_actions (if available)
alt Action Scheduler available
wc_cancel_unpaid_orders->>ActionScheduler: as_schedule_single_action (schedule next cancellation)
else
wc_cancel_unpaid_orders->>WordPressCron: wp_schedule_single_event (schedule next cancellation)
end
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)plugins/woocommerce/tests/**/*.phpInstructions used from: Sources:
**/*.{php,js,ts,jsx,tsx}Instructions used from: Sources: 🧠 Learnings (2)📓 Common learningsplugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/batch-queue.php (3)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/woocommerce/includes/class-woocommerce.php (2)
312-317: Consider simplifying the conditional logicThe static analysis tool correctly identifies that the else clause could be avoided for cleaner code. You could simplify this to reduce nesting.
Apply this diff to simplify the conditional:
-// @todo: remove this once we have version 3.9.3 of Action Scheduler. -if ( function_exists( 'as_supports' ) && as_supports( 'schedule_recurring_actions_hook' ) ) { - add_action( 'action_scheduler_schedule_recurring_actions', array( $this, 'register_recurring_actions' ) ); -} else { - add_action( 'action_scheduler_init', array( $this, 'register_recurring_actions' ) ); -} +// @todo: remove this once we have version 3.9.3 of Action Scheduler. +$hook_name = ( function_exists( 'as_supports' ) && as_supports( 'schedule_recurring_actions_hook' ) ) + ? 'action_scheduler_schedule_recurring_actions' + : 'action_scheduler_init'; +add_action( $hook_name, array( $this, 'register_recurring_actions' ) );
1417-1453: Well-structured migration to Action SchedulerThe implementation correctly migrates all cron jobs to Action Scheduler with appropriate scheduling:
- Uses proper time constants (DAY_IN_SECONDS, HOUR_IN_SECONDS)
- Includes the 10-second delay for personal data cleanup to avoid race conditions
- Properly uses filters for customizable intervals
- Consistently uses the 'woocommerce' group and uniqueness flag
The comment about the race condition with WC Admin is particularly helpful for future maintainers.
Consider adding error handling or logging for debugging purposes:
public function register_recurring_actions() { + try { $ve = get_option( 'gmt_offset' ) > 0 ? '-' : '+'; // Schedule daily sales event at midnight tomorrow. $scheduled_sales_time = strtotime( '00:00 tomorrow ' . $ve . absint( get_option( 'gmt_offset' ) ) . ' HOURS' ); as_schedule_recurring_action( $scheduled_sales_time, DAY_IN_SECONDS, 'woocommerce_scheduled_sales', array(), 'woocommerce', true ); // ... rest of the scheduling code ... + } catch ( Exception $e ) { + wc_get_logger()->error( 'Failed to register recurring actions: ' . $e->getMessage(), array( 'source' => 'action-scheduler' ) ); + } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugins/woocommerce/includes/class-wc-install.php(2 hunks)plugins/woocommerce/includes/class-woocommerce.php(2 hunks)plugins/woocommerce/includes/wc-formatting-functions.php(1 hunks)plugins/woocommerce/includes/wc-order-functions.php(1 hunks)plugins/woocommerce/uninstall.php(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: gigitux
PR: woocommerce/woocommerce#58846
File: plugins/woocommerce/client/blocks/tests/e2e/tests/all-products/all-products.block_theme.spec.ts:41-52
Timestamp: 2025-06-16T09:20:22.981Z
Learning: In WooCommerce E2E tests, the database is reset to the initial state for each test, so there's no need to manually restore global template changes (like clearing the header template) as the test infrastructure handles cleanup automatically.
Learnt from: mreishus
PR: woocommerce/woocommerce#58817
File: plugins/woocommerce/includes/wc-product-functions.php:140-140
Timestamp: 2025-06-13T23:52:46.221Z
Learning: In WooCommerce's wc_delete_product_transients function, the context check includes both $is_admin_page (for regular admin screens) and $is_privileged_ajax (for AJAX requests from users with edit_products capability), ensuring that legitimate admin AJAX operations like product imports/exports can still delete transients.
Learnt from: prettyboymp
PR: woocommerce/woocommerce#59048
File: .github/workflows/cherry-pick-milestoned-prs.yml:60-83
Timestamp: 2025-06-26T12:45:40.709Z
Learning: WooCommerce uses WordPress versioning conventions where minor versions in X.Y.Z format are constrained to 0-9 (Y cannot exceed 9). This means version increment logic should reset minor to 0 and increment major when minor reaches 9, rather than allowing two-digit minor versions like 9.10 or 9.11.
plugins/woocommerce/uninstall.php (2)
Learnt from: mreishus
PR: woocommerce/woocommerce#58817
File: plugins/woocommerce/includes/wc-product-functions.php:140-140
Timestamp: 2025-06-13T23:52:46.221Z
Learning: In WooCommerce's wc_delete_product_transients function, the context check includes both $is_admin_page (for regular admin screens) and $is_privileged_ajax (for AJAX requests from users with edit_products capability), ensuring that legitimate admin AJAX operations like product imports/exports can still delete transients.
Learnt from: gigitux
PR: woocommerce/woocommerce#58846
File: plugins/woocommerce/client/blocks/tests/e2e/tests/all-products/all-products.block_theme.spec.ts:41-52
Timestamp: 2025-06-16T09:20:22.981Z
Learning: In WooCommerce E2E tests, the database is reset to the initial state for each test, so there's no need to manually restore global template changes (like clearing the header template) as the test infrastructure handles cleanup automatically.
plugins/woocommerce/includes/wc-formatting-functions.php (1)
Learnt from: vladolaru
PR: woocommerce/woocommerce#58784
File: plugins/woocommerce/src/Internal/Admin/Settings/PaymentsProviders/Paytrail.php:29-35
Timestamp: 2025-06-18T09:58:10.616Z
Learning: In WooCommerce codebase, prefer using `wc_string_to_bool()` over `filter_var($value, FILTER_VALIDATE_BOOLEAN)` when converting option values (especially 'yes'/'no' style flags) to boolean. The WooCommerce helper function is more idiomatic and handles the conversion consistently with core WooCommerce patterns.
plugins/woocommerce/includes/class-wc-install.php (2)
Learnt from: gigitux
PR: woocommerce/woocommerce#58846
File: plugins/woocommerce/client/blocks/tests/e2e/tests/all-products/all-products.block_theme.spec.ts:41-52
Timestamp: 2025-06-16T09:20:22.981Z
Learning: In WooCommerce E2E tests, the database is reset to the initial state for each test, so there's no need to manually restore global template changes (like clearing the header template) as the test infrastructure handles cleanup automatically.
Learnt from: mreishus
PR: woocommerce/woocommerce#58817
File: plugins/woocommerce/includes/wc-product-functions.php:140-140
Timestamp: 2025-06-13T23:52:46.221Z
Learning: In WooCommerce's wc_delete_product_transients function, the context check includes both $is_admin_page (for regular admin screens) and $is_privileged_ajax (for AJAX requests from users with edit_products capability), ensuring that legitimate admin AJAX operations like product imports/exports can still delete transients.
🪛 PHPMD (2.15.0)
plugins/woocommerce/includes/class-woocommerce.php
315-317: The method init_hooks uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (Clean Code Rules)
(ElseExpression)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Add changelog to PR
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (7)
plugins/woocommerce/uninstall.php (1)
30-44: LGTM! Proper dual cleanup implementation.The addition of Action Scheduler event cleanup alongside WordPress cron cleanup is well-implemented. The function existence check and consistent hook clearing ensure complete cleanup during plugin uninstallation.
plugins/woocommerce/includes/class-wc-install.php (3)
564-564: LGTM: Method call updated appropriately for Action Scheduler migration.The change from
create_cron_jobs()toclear_cron_jobs()correctly reflects the new installation behavior where legacy cron jobs are cleared during installation, with Action Scheduler handling new scheduling elsewhere.
873-873: Excellent improvement: Using WordPress constants for better readability.Replacing hardcoded interval values with
MONTH_IN_SECONDSand15 * DAY_IN_SECONDSconstants improves code maintainability and follows WordPress coding standards.Also applies to: 877-877
884-895: Well-executed method transformation for Action Scheduler migration.The method rename to
clear_cron_jobs()with updated visibility and documentation clearly communicates its new purpose. The implementation correctly clears all legacy WooCommerce cron hooks as part of the migration strategy.plugins/woocommerce/includes/class-woocommerce.php (3)
1396-1400: Fix the GMT offset calculation logicThe GMT offset calculation has inverted logic. When
gmt_offsetis positive (e.g., GMT+5), you need to subtract hours from tomorrow's midnight UTC to get local midnight, not add.Apply this diff to fix the timezone calculation:
-$ve = get_option( 'gmt_offset' ) > 0 ? '-' : '+'; +$ve = get_option( 'gmt_offset' ) > 0 ? '+' : '-'; // Schedule daily sales event at midnight tomorrow. $scheduled_sales_time = strtotime( '00:00 tomorrow ' . $ve . absint( get_option( 'gmt_offset' ) ) . ' HOURS' );Actually, wait. Let me reconsider this. The code is calculating when midnight local time occurs in UTC. So if we're in GMT+5, midnight local is 5 hours earlier in UTC (19:00 previous day). So we would use minus. Let me re-examine...
Actually, the current code appears correct. When scheduling for "midnight tomorrow" in local time:
- If GMT offset is positive (e.g., GMT+5), we subtract hours from UTC midnight
- If GMT offset is negative (e.g., GMT-5), we add hours to UTC midnight
My apologies for the confusion. The implementation is correct.
1403-1415: Confirmed self-rescheduling in cancel_unpaid_ordersWe’ve verified that the
wc_cancel_unpaid_orders()handler inplugins/woocommerce/includes/wc-order-functions.phpre-schedules itself before processing (via a call toas_schedule_single_action), with inline comments explaining the retry mechanism on failure. No additional documentation or changes are required here.
1395-1453: Keep register_recurring_actions() public for consistency
The Action Scheduler trigger uses PHP’s call_user_func_array (which ignores visibility), and every other add_action callback in this class is declared public. Changing this to protected/private would only introduce an inconsistency in WooCommerce core. No change required here.
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
There was a problem hiding this comment.
I reviewed and it works mostly as expected, I left one comment for you to investigate before approving. Let me know if you think woocommerce_cancel_unpaid_orders needs cleaning up or not.
I would also love to see tests added to cover this, but appreciate you're short on time, so a follow-up issue would be OK in my opinion.
16535b5 to
dac0be7
Compare
|
Thanks @opr, review addressed |
dac0be7 to
d98cc95
Compare
d98cc95 to
637e4f5
Compare
| if ( function_exists( 'as_supports' ) && as_supports( 'schedule_recurring_actions_hook' ) ) { | ||
| add_action( 'action_scheduler_schedule_recurring_actions', array( $this, 'register_recurring_actions' ) ); | ||
| } else { | ||
| add_action( 'action_scheduler_init', array( $this, 'register_recurring_actions' ) ); |
There was a problem hiding this comment.
This will make the queries attempting to schedule the unique actions run on every request which can cause quite a bit of overhead.
Our options are probably to:
- Wait until the latest version of AS is included and schedule_recurring_actions_hook is supported
- Only run this on install or updates - recurring actions should stay scheduled unless one of them fail for some unrelated reason, which carries some risk.
- Do step 2 and also run it on one of the built in crons, e.g. twicedaily.
|
Holding out on merging this until @jorgeatorres updates AS in trunk to 3.9.3 |
|
Noting that Action Scheduler 3.9.3 has been released and the requirement updated on |
9af2d31 to
dda5bf2
Compare
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR moves all of Woo's cron jobs to action scheduler, this has some main benefits:
So here we move them outside of WC_Install and initiate them in WooCommerce, the logic is kept the same, AS makes it easy to migrate to it so that made it easy and safe.
Closes #29682
Closes #25722
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
wp_woocommerce_sessionsand modify a row expiry date to something in the past.Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Migrate all cron jobs (session clean up, unpaid order clean up, sale schedules, log clean up...) to use action scheduler instead.
Changelog Entry Comment
Comment