-
Notifications
You must be signed in to change notification settings - Fork 842
Full_Sync_Immediately: Check module existence to prevent array key warnings #46475
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
base: trunk
Are you sure you want to change the base?
Full_Sync_Immediately: Check module existence to prevent array key warnings #46475
Conversation
… names as array keys
|
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! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
… existing behavior
…ed in these specific cases before.
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.
Pull request overview
This PR addresses array key warnings that occur when sync modules are added or removed during a full sync operation. The fix adds checks using the null coalescing operator before accessing module-specific configuration and progress data, and extracts the module name into a variable to reduce repeated method calls.
Key Changes:
- Adds null coalescing operator checks for module config and progress array keys
- Stores module name in a variable to avoid repeated
$module->name()calls - Corrects doc block parameter type from
stringtoarrayfor$config
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/packages/sync/src/modules/class-full-sync-immediately.php | Adds array key existence checks and extracts module name to variable |
| projects/packages/sync/src/modules/class-module.php | Corrects parameter type documentation and formatting |
| projects/packages/sync/changelog/update-full_sync_immediately-send-check-modules-exist | Adds changelog entry for the bug fix |
projects/packages/sync/src/modules/class-full-sync-immediately.php
Outdated
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
projects/packages/sync/src/modules/class-full-sync-immediately.php
Outdated
Show resolved
Hide resolved
projects/packages/sync/src/modules/class-full-sync-immediately.php
Outdated
Show resolved
Hide resolved
projects/packages/sync/src/modules/class-full-sync-immediately.php
Outdated
Show resolved
Hide resolved
projects/packages/sync/src/modules/class-full-sync-immediately.php
Outdated
Show resolved
Hide resolved
…ediately-send-check-modules-exist
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
projects/plugins/jetpack/changelog/update-full_sync_immediately-send-check-modules-exist
Outdated
Show resolved
Hide resolved
projects/packages/sync/src/modules/class-full-sync-immediately.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/tests/php/sync/Jetpack_Sync_Full_Immediately_Test.php
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
…ious behavior when a key exists but its value is null
Fixes SYNC-173
Proposed changes:
$progressand$configarrays before attempting to access them. Previously with the warnings we were still processing insend_full_sync_actions(but with empty$statusand$config). If we retain the existing behavior and pass null defaults in those cases, we generate more warnings, withinclass-module.php. So just replicating existing behavior isn't ideal in this case.$module->name()in a variable, reducing repeated method calls.Undefined array key "woocommerce_hpos_orders", and alsoUndefined array key "sent"warnings inclass-module.php.$configor$progressinitially insend, vs when we get modules inget_remaining_modules_to_send.get_remaining_modules_to_sendsees the extra module - and we can then test that our new guard prevents callingsend_full_sync_actionsfor that module, but still completes the full sync.There are two other options here:
get_remaining_modules_to_sendwe can ensure we don't continue if$status['progress'][ $module_name ]is not set.$progressand$confighave the up to date modules.Why the current approach instead of these?
jetpack_full_sync_startconfig/range.Other information:
Jetpack product discussion
p1763754755558699/1763754199.536589-slack-C05PV073SG3
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
To test this out, I found the best approach was to add some code. To replicate the warnings, on trunk add the following code just below the foreach:
Separately you can also test a module in an unfinished progress state (same location for the code):
Lastly, we can also test some unusual config set up by adding the following in place of or after the
$full_sync_configcheck instart(we can't set$full_sync_configto null directly as we assume$configexists in places likeget_remaining_modules_to_send):Running tests:
jetpack docker phpunit jetpack -- --filter=Jetpack_Sync_Full_Immediately_Test