POC: Move WooCommerce Analytics Sync into the shared Sync package#50715
POC: Move WooCommerce Analytics Sync into the shared Sync package#50715layoutd wants to merge 10 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 4 files.
3 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
| * | ||
| * Additive: appends to whatever module list is already configured rather than | ||
| * replacing it. When a legacy consumer has already contributed its own module, | ||
| * removes the shared class that Config data settings may have added earlier. |
There was a problem hiding this comment.
I think there might be an ordering race here that can double-register the module on a Woo AI + PA site.
The stand-down works by checking whether a legacy module is already in the list. But ai-sync's filter also hooks at PHP_INT_MAX, and it seems that we register earlier so we keep the shared class.
Two ways out I can see: dedupe by name() in Modules::initialize_modules() , or as a stopgap, add the shared class to ai-sync's detection list before this ships.
There was a problem hiding this comment.
Good point. Fixed in b3e0368 by deduplicating initialized Sync modules by name(), with later registrations winning; regression coverage exercises both orders. This keeps Woo AI’s legacy module authoritative on mixed installs.
| @@ -32,14 +34,12 @@ | |||
| use WC_Order_Factory; | |||
| use WC_Tax; | |||
|
|
|||
| defined( 'ABSPATH' ) || exit; | |||
There was a problem hiding this comment.
Tiny consistency nit: should we add a guard?
jetpack/projects/packages/sync/src/modules/class-protect.php
Lines 13 to 15 in 34a3089
There was a problem hiding this comment.
Good catch, restored the direct-access guard in 65a1828, and added it to the moved utility trait for consistency.
Addresses WOOA7S-1771
Proposed changes
woocommerce_analyticsSync module and its utilities from Premium Analytics intoprojects/packages/sync.WooCommerce_Analytics_SettingsAPI so high-volume Analytics syncing remains disabled unless a consumer explicitly enables it.jetpack_sync_active_modulescallable as the proposed consumer-neutral receiver capability signal instead of introducing another synced option.This POC intentionally limits the first step to Jetpack Sync and Premium Analytics. It does not migrate Woo AI or update the WordPress.com receiver; on mixed installs, Woo AI's existing module remains authoritative so both implementations do not register the same events.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No. This moves the existing WooCommerce Analytics Sync payloads into a shared package and preserves their wire contract. It does not add new tracked fields or receiver behavior.
Testing instructions
Automattic\Jetpack\Sync\Modules::get_modules()afterplugins_loaded; confirm exactly one module reportsname() === 'woocommerce_analytics'and it uses the shared Jetpack Sync class.woocommerce_analyticsmodule remains and Woo AI's existing implementation takes precedence.