IDC: revalidate IDC error #46268
IDC: revalidate IDC error #46268
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?
|
github-actions
bot
added
[Package] Connection
[Plugin] Automattic For Agencies Client
[Plugin] Backup
|
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. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Inspect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Paypal Payment buttons plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcloud Sso plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
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
|
There was a problem hiding this comment.
Pull request overview
This PR adds IDC (Identity Crisis) revalidation functionality to the Jetpack Connection package. When an IDC is detected, the system will now periodically validate with WordPress.com to check if the issue has been resolved, using progressive backoff delays (1 hour initially, doubling each time, up to 30 days maximum).
Key Changes
- Introduces progressive validation timing for IDC with exponential backoff (1h → 2h → 4h → ... → 30 days max)
- Adds remote validation via API calls to WordPress.com to check if detected IDC still exists
- Ensures backward compatibility by adding timing fields to existing IDC options
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/connection/src/identity-crisis/class-identity-crisis.php | Core implementation: adds constants, validation timing logic, remote API validation, and backward-compatible timing fields to IDC options |
| projects/packages/connection/tests/php/identity-crisis/Identity_Crisis_Test.php | Test coverage for new validation logic including timing validation, backward compatibility, and error handling scenarios |
| projects/packages/connection/changelog/add-idc-revalidate | Changelog entry for the connection package |
| projects/plugins/jetpack/changelog/add-idc-revalidate | Changelog entry for Jetpack plugin (uses "enhancement" type) |
| projects/plugins/*/changelog/add-idc-revalidate | Changelog entries for other affected plugins (backup, boost, inspect, mu-wpcom-plugin, paypal-payment-buttons, protect, search, social, starter-plugin, videopress, wpcloud-sso, wpcomsh) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( $sync_error['next_check_delay'] >= self::IDC_VALIDATION_MAX_DELAY ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check if enough time has passed since the last check. | ||
| $time_since_last_check = time() - $sync_error['last_checked']; | ||
| return $time_since_last_check >= $sync_error['next_check_delay']; |
There was a problem hiding this comment.
Missing array key validation. The method assumes $sync_error['next_check_delay'] and $sync_error['last_checked'] exist but doesn't validate this. If these keys are missing, it will cause PHP warnings or errors. Add checks with isset() or use the null coalescing operator before accessing these array keys.
| if ( $sync_error['next_check_delay'] >= self::IDC_VALIDATION_MAX_DELAY ) { | |
| return false; | |
| } | |
| // Check if enough time has passed since the last check. | |
| $time_since_last_check = time() - $sync_error['last_checked']; | |
| return $time_since_last_check >= $sync_error['next_check_delay']; | |
| if ( ( $sync_error['next_check_delay'] ?? 0 ) >= self::IDC_VALIDATION_MAX_DELAY ) { | |
| return false; | |
| } | |
| // Check if enough time has passed since the last check. | |
| $time_since_last_check = time() - ( $sync_error['last_checked'] ?? 0 ); | |
| return $time_since_last_check >= ( $sync_error['next_check_delay'] ?? 0 ); |
| public static function validate_idc_from_remote( $sync_error ) { | ||
| $blog_id = Jetpack_Options::get_option( 'id' ); | ||
| if ( ! $blog_id ) { | ||
| // Site not registered, can't make API call. | ||
| return false; | ||
| } | ||
|
|
||
| // Make a lightweight API call to WordPress.com. | ||
| // The response will automatically include 'idc_detected' if URLs still mismatch. | ||
| $response = Client::wpcom_json_api_request_as_blog( | ||
| sprintf( 'sites/%d', $blog_id ), | ||
| '2', | ||
| array( 'method' => 'GET' ), | ||
| null, | ||
| 'wpcom' | ||
| ); | ||
|
|
||
| // Handle network/API errors - do nothing, will retry at next scheduled interval. | ||
| if ( is_wp_error( $response ) || 200 !== wp_remote_retrieve_response_code( $response ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| $body = json_decode( wp_remote_retrieve_body( $response ), true ); | ||
| if ( ! is_array( $body ) ) { | ||
| // Malformed response - do nothing. | ||
| return false; | ||
| } | ||
|
|
||
| // Check if WordPress.com still detects an IDC. | ||
| if ( isset( $body['idc_detected'] ) && is_array( $body['idc_detected'] ) ) { | ||
| // IDC still exists - refresh with latest data from WordPress.com and update timing. | ||
| $fresh_idc_data = self::get_sync_error_idc_option( $body['idc_detected'] ); | ||
|
|
||
| // Preserve and update timing fields with exponential backoff. | ||
| $fresh_idc_data['last_checked'] = time(); | ||
| $fresh_idc_data['next_check_delay'] = min( | ||
| $sync_error['next_check_delay'] * 2, | ||
| self::IDC_VALIDATION_MAX_DELAY | ||
| ); | ||
|
|
||
| Jetpack_Options::update_option( 'sync_error_idc', $fresh_idc_data ); | ||
| return false; | ||
| } | ||
|
|
||
| // No IDC detected in response - WordPress.com URLs now match, clear the local IDC. | ||
| Jetpack_Options::delete_option( 'sync_error_idc' ); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Missing array key validation. The method assumes $sync_error['next_check_delay'] exists at line 532 but doesn't validate this. While the caller validate_sync_error_idc_option() adds these fields, the method is public and could be called from elsewhere. Add validation with isset() or document that this parameter must contain these keys.
| } | ||
|
|
||
| // Check if WordPress.com still detects an IDC. | ||
| if ( isset( $body['idc_detected'] ) && is_array( $body['idc_detected'] ) ) { |
There was a problem hiding this comment.
The condition checks if idc_detected is an array, but doesn't verify that it's non-empty. An empty array would pass this check and proceed to call get_sync_error_idc_option() with an empty array, which might not be the intended behavior. Consider adding a check for a non-empty array: isset( $body['idc_detected'] ) && is_array( $body['idc_detected'] ) && ! empty( $body['idc_detected'] )
| if ( isset( $body['idc_detected'] ) && is_array( $body['idc_detected'] ) ) { | |
| if ( isset( $body['idc_detected'] ) && is_array( $body['idc_detected'] ) && ! empty( $body['idc_detected'] ) ) { |
| /** | ||
| * Test validate_idc_from_remote() returns false when site is not registered. | ||
| */ | ||
| public function test_validate_idc_from_remote_returns_false_when_not_registered() { | ||
| Jetpack_Options::delete_option( 'id' ); | ||
|
|
||
| $sync_error = Identity_Crisis::get_sync_error_idc_option(); | ||
| $result = Identity_Crisis::validate_idc_from_remote( $sync_error ); | ||
|
|
||
| $this->assertFalse( $result ); | ||
| } | ||
|
|
||
| /** | ||
| * Test validate_idc_from_remote() returns false and doesn't change option on network error. | ||
| */ | ||
| public function test_validate_idc_from_remote_handles_network_error_gracefully() { | ||
| Jetpack_Options::update_option( 'id', 12345 ); | ||
| $initial_sync_error = Identity_Crisis::get_sync_error_idc_option(); | ||
| Jetpack_Options::update_option( 'sync_error_idc', $initial_sync_error ); | ||
|
|
||
| // Mock a network error response. | ||
| $mock_callback = function () { | ||
| return new \WP_Error( 'http_request_failed', 'Network error' ); | ||
| }; | ||
| add_filter( 'pre_http_request', $mock_callback, 10, 3 ); | ||
|
|
||
| $result = Identity_Crisis::validate_idc_from_remote( $initial_sync_error ); | ||
|
|
||
| remove_filter( 'pre_http_request', $mock_callback ); | ||
| $stored_option = Jetpack_Options::get_option( 'sync_error_idc' ); | ||
|
|
||
| Jetpack_Options::delete_option( 'id' ); | ||
| Jetpack_Options::delete_option( 'sync_error_idc' ); | ||
|
|
||
| $this->assertFalse( $result ); | ||
| // Option should be unchanged. | ||
| $this->assertEquals( $initial_sync_error, $stored_option ); | ||
| } | ||
|
|
||
| /** | ||
| * Test validate_idc_from_remote() returns false and doesn't change option on non-200 response. | ||
| */ | ||
| public function test_validate_idc_from_remote_handles_non_200_response() { | ||
| Jetpack_Options::update_option( 'id', 12345 ); | ||
| $initial_sync_error = Identity_Crisis::get_sync_error_idc_option(); | ||
| Jetpack_Options::update_option( 'sync_error_idc', $initial_sync_error ); | ||
|
|
||
| // Mock a 500 error response. | ||
| $mock_callback = function () { | ||
| return array( | ||
| 'headers' => array(), | ||
| 'body' => 'Internal server error', | ||
| 'response' => array( | ||
| 'code' => 500, | ||
| 'message' => 'Internal Server Error', | ||
| ), | ||
| 'cookies' => array(), | ||
| 'filename' => null, | ||
| ); | ||
| }; | ||
| add_filter( 'pre_http_request', $mock_callback, 10, 3 ); | ||
|
|
||
| $result = Identity_Crisis::validate_idc_from_remote( $initial_sync_error ); | ||
|
|
||
| remove_filter( 'pre_http_request', $mock_callback ); | ||
| $stored_option = Jetpack_Options::get_option( 'sync_error_idc' ); | ||
|
|
||
| Jetpack_Options::delete_option( 'id' ); | ||
| Jetpack_Options::delete_option( 'sync_error_idc' ); | ||
|
|
||
| $this->assertFalse( $result ); | ||
| // Option should be unchanged. | ||
| $this->assertEquals( $initial_sync_error, $stored_option ); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the successful IDC clearing case. There should be a test that mocks a successful 200 response without idc_detected in the body to verify that validate_idc_from_remote() returns true and clears the IDC option. This would complete test coverage for all branches of the method.
| /** | ||
| * Test validate_idc_from_remote() returns false when site is not registered. | ||
| */ | ||
| public function test_validate_idc_from_remote_returns_false_when_not_registered() { | ||
| Jetpack_Options::delete_option( 'id' ); | ||
|
|
||
| $sync_error = Identity_Crisis::get_sync_error_idc_option(); | ||
| $result = Identity_Crisis::validate_idc_from_remote( $sync_error ); | ||
|
|
||
| $this->assertFalse( $result ); | ||
| } | ||
|
|
||
| /** | ||
| * Test validate_idc_from_remote() returns false and doesn't change option on network error. | ||
| */ | ||
| public function test_validate_idc_from_remote_handles_network_error_gracefully() { | ||
| Jetpack_Options::update_option( 'id', 12345 ); | ||
| $initial_sync_error = Identity_Crisis::get_sync_error_idc_option(); | ||
| Jetpack_Options::update_option( 'sync_error_idc', $initial_sync_error ); | ||
|
|
||
| // Mock a network error response. | ||
| $mock_callback = function () { | ||
| return new \WP_Error( 'http_request_failed', 'Network error' ); | ||
| }; | ||
| add_filter( 'pre_http_request', $mock_callback, 10, 3 ); | ||
|
|
||
| $result = Identity_Crisis::validate_idc_from_remote( $initial_sync_error ); | ||
|
|
||
| remove_filter( 'pre_http_request', $mock_callback ); | ||
| $stored_option = Jetpack_Options::get_option( 'sync_error_idc' ); | ||
|
|
||
| Jetpack_Options::delete_option( 'id' ); | ||
| Jetpack_Options::delete_option( 'sync_error_idc' ); | ||
|
|
||
| $this->assertFalse( $result ); | ||
| // Option should be unchanged. | ||
| $this->assertEquals( $initial_sync_error, $stored_option ); | ||
| } | ||
|
|
||
| /** | ||
| * Test validate_idc_from_remote() returns false and doesn't change option on non-200 response. | ||
| */ | ||
| public function test_validate_idc_from_remote_handles_non_200_response() { | ||
| Jetpack_Options::update_option( 'id', 12345 ); | ||
| $initial_sync_error = Identity_Crisis::get_sync_error_idc_option(); | ||
| Jetpack_Options::update_option( 'sync_error_idc', $initial_sync_error ); | ||
|
|
||
| // Mock a 500 error response. | ||
| $mock_callback = function () { | ||
| return array( | ||
| 'headers' => array(), | ||
| 'body' => 'Internal server error', | ||
| 'response' => array( | ||
| 'code' => 500, | ||
| 'message' => 'Internal Server Error', | ||
| ), | ||
| 'cookies' => array(), | ||
| 'filename' => null, | ||
| ); | ||
| }; | ||
| add_filter( 'pre_http_request', $mock_callback, 10, 3 ); | ||
|
|
||
| $result = Identity_Crisis::validate_idc_from_remote( $initial_sync_error ); | ||
|
|
||
| remove_filter( 'pre_http_request', $mock_callback ); | ||
| $stored_option = Jetpack_Options::get_option( 'sync_error_idc' ); | ||
|
|
||
| Jetpack_Options::delete_option( 'id' ); | ||
| Jetpack_Options::delete_option( 'sync_error_idc' ); | ||
|
|
||
| $this->assertFalse( $result ); | ||
| // Option should be unchanged. | ||
| $this->assertEquals( $initial_sync_error, $stored_option ); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the case where idc_detected is present in the response body and the IDC option is refreshed with updated timing fields. This would verify that the exponential backoff logic works correctly and that the IDC option is updated rather than cleared.
| if ( ! is_array( $body ) ) { | ||
| // Malformed response - do nothing. |
There was a problem hiding this comment.
Missing JSON decoding error handling. The json_decode() call should check for JSON errors using json_last_error() to distinguish between malformed JSON and valid JSON that decodes to something other than an array. Currently, both cases are treated the same way, which could mask legitimate JSON decoding errors in debugging.
| if ( ! is_array( $body ) ) { | |
| // Malformed response - do nothing. | |
| if ( json_last_error() !== JSON_ERROR_NONE ) { | |
| // Malformed JSON response - do nothing. | |
| return false; | |
| } | |
| if ( ! is_array( $body ) ) { | |
| // Valid JSON, but not an array - unexpected response structure. |
Fixes CONNECT-124
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: