Full_Sync_Immediately: Check module existence to prevent array key warnings #46475
Conversation
… names as array keys
coder-karen
added
[Type] Bug
|
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 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.
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
| if ( isset( $progress[ $module_name ] ) && array_key_exists( $module_name, $config ) ) { | ||
| $progress[ $module_name ] = $module->send_full_sync_actions( $config[ $module_name ], $progress[ $module_name ], $send_until, $started ); | ||
| if ( isset( $progress[ $module_name ]['error'] ) ) { | ||
| unset( $progress[ $module_name ]['error'] ); | ||
| $this->update_status( array( 'progress' => $progress ) ); | ||
| return false; | ||
| } elseif ( ! $progress[ $module_name ]['finished'] ) { | ||
| $this->update_status( array( 'progress' => $progress ) ); | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if ( $this->get_status()['started'] !== $started ) { | ||
| // Full sync was restarted, stop sending. | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Only end if everything is finished. If not, persist and try again next time. | ||
| // We may end up in this state if get_remaining_modules_to_send returns more modules than we initially stored in $progress or $config. | ||
| $all_progress_finished = true; | ||
| foreach ( $progress as $progress_module_name ) { | ||
| if ( empty( $progress_module_name['finished'] ) || true !== $progress_module_name['finished'] ) { | ||
| $all_progress_finished = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ( ! $all_progress_finished ) { | ||
| $this->update_status( array( 'progress' => $progress ) ); | ||
| return true; // release lock and continue later | ||
| } |
There was a problem hiding this comment.
The new logic for handling missing modules (lines 399-409) and the verification loop (lines 419-430) lack test coverage. Given that the project has existing test infrastructure and this change addresses a critical bug scenario (modules appearing/disappearing during sync execution), consider adding tests that verify: 1) modules not in $progress or $config are properly skipped without warnings, 2) sync completes successfully when all tracked modules finish, and 3) the behavior when module lists change mid-sync.
| foreach ( $progress as $progress_module_name ) { | ||
| if ( empty( $progress_module_name['finished'] ) || true !== $progress_module_name['finished'] ) { |
There was a problem hiding this comment.
The variable name 'progress_module_name' is misleading. This variable doesn't contain a module name string - it contains a module's progress status array (which should have keys like 'finished', 'last_sent', etc.). Consider renaming it to 'module_progress' or 'progress_status' to better reflect what it contains.
| foreach ( $progress as $progress_module_name ) { | |
| if ( empty( $progress_module_name['finished'] ) || true !== $progress_module_name['finished'] ) { | |
| foreach ( $progress as $module_progress ) { | |
| if ( empty( $module_progress['finished'] ) || true !== $module_progress['finished'] ) { |
| // We may end up in this state if get_remaining_modules_to_send returns more modules than we initially stored in $progress or $config. | ||
| $all_progress_finished = true; | ||
| foreach ( $progress as $progress_module_name ) { | ||
| if ( empty( $progress_module_name['finished'] ) || true !== $progress_module_name['finished'] ) { |
There was a problem hiding this comment.
The condition 'true !== $progress_module_name['finished']' is redundant given the preceding 'empty()' check. If 'empty()' returns false (meaning the value exists and is truthy), the only possible value that would pass the 'empty()' check but still need to be compared to 'true' would be a truthy non-boolean value. However, throughout the codebase, 'finished' is consistently set to 'true' (boolean). The 'empty()' check alone is sufficient here.
| if ( empty( $progress_module_name['finished'] ) || true !== $progress_module_name['finished'] ) { | |
| if ( empty( $progress_module_name['finished'] ) ) { |
| $all_progress_finished = true; | ||
| foreach ( $progress as $progress_module_name ) { | ||
| if ( empty( $progress_module_name['finished'] ) || true !== $progress_module_name['finished'] ) { | ||
| $all_progress_finished = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The combination of skipping modules (line 399) and the new verification logic (lines 419-425) could create a deadlock. If a module exists in $progress as unfinished but is no longer returned by get_remaining_modules_to_send() (e.g., removed from $config or Modules::get_module() returns null for it), the module will be skipped during processing but the verification loop will detect it's unfinished and prevent sync completion. On subsequent calls to send(), the same situation repeats indefinitely. Consider checking the verification loop against the same conditions used in the skip check - only verify modules that exist in both $progress and $config.
…maining_modules_to_send
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.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:
A code review should suffice here (I also couldn't naturally replicate this).
Testing the happy path: