From cb075b7bdafeba28872bddd6bc3159257070638e Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Mon, 22 Apr 2024 20:03:04 +0200 Subject: [PATCH] Scheduled Updates: Only validate schedule when logging (#36961) * Scheduled Updates: Only validate schedule when logging Removes all other schedule_exists checks to simplify clearing logs after a schedule was deleted. This will also help with moving non-schedule operations in the rest api endpoint to action callbacks in #36835. * Update function docs --- .../scheduled-updates/.phan/baseline.php | 3 +- .../changelog/remove-schedule-check | 4 + .../src/class-scheduled-updates-logs.php | 79 +++++----------- ...-rest-api-v2-endpoint-update-schedules.php | 6 +- .../php/class-scheduled-updates-logs-test.php | 92 +++++++++---------- 5 files changed, 76 insertions(+), 108 deletions(-) create mode 100644 projects/packages/scheduled-updates/changelog/remove-schedule-check diff --git a/projects/packages/scheduled-updates/.phan/baseline.php b/projects/packages/scheduled-updates/.phan/baseline.php index 359c87df8e588..0d2df5ed97d46 100644 --- a/projects/packages/scheduled-updates/.phan/baseline.php +++ b/projects/packages/scheduled-updates/.phan/baseline.php @@ -10,11 +10,11 @@ return [ // # Issue statistics: // PhanUndeclaredProperty : 40+ occurrences - // PhanCompatibleAccessMethodOnTraitDefinition : 2 occurrences // PhanPluginMixedKeyNoKey : 2 occurrences // PhanRedundantCondition : 2 occurrences // PhanTypeArraySuspiciousNullable : 2 occurrences // PhanUndeclaredClassMethod : 2 occurrences + // PhanCompatibleAccessMethodOnTraitDefinition : 1 occurrence // PhanNoopNew : 1 occurrence // PhanTypeMismatchArgumentProbablyReal : 1 occurrence // PhanUndeclaredFunction : 1 occurrence @@ -24,7 +24,6 @@ 'src/class-scheduled-updates.php' => ['PhanRedundantCondition', 'PhanTypeMismatchArgumentProbablyReal', 'PhanUndeclaredClassMethod'], 'src/pluggable.php' => ['PhanTypeArraySuspiciousNullable'], 'src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php' => ['PhanPluginMixedKeyNoKey', 'PhanUndeclaredFunction'], - 'tests/php/class-scheduled-updates-logs-test.php' => ['PhanCompatibleAccessMethodOnTraitDefinition', 'PhanUndeclaredProperty'], 'tests/php/class-scheduled-updates-test.php' => ['PhanCompatibleAccessMethodOnTraitDefinition', 'PhanUndeclaredProperty'], 'tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php' => ['PhanNoopNew'], ], diff --git a/projects/packages/scheduled-updates/changelog/remove-schedule-check b/projects/packages/scheduled-updates/changelog/remove-schedule-check new file mode 100644 index 0000000000000..660863fcf2f7d --- /dev/null +++ b/projects/packages/scheduled-updates/changelog/remove-schedule-check @@ -0,0 +1,4 @@ +Significance: major +Type: removed + +Scheduled Updates: Removed checks for valid schedules when retreiving and clearing logs diff --git a/projects/packages/scheduled-updates/src/class-scheduled-updates-logs.php b/projects/packages/scheduled-updates/src/class-scheduled-updates-logs.php index 284ec896de610..e643147cb5b9d 100644 --- a/projects/packages/scheduled-updates/src/class-scheduled-updates-logs.php +++ b/projects/packages/scheduled-updates/src/class-scheduled-updates-logs.php @@ -7,8 +7,6 @@ namespace Automattic\Jetpack; -use WP_Error; - /** * Scheduled_Update_Logs class * @@ -46,13 +44,18 @@ class Scheduled_Updates_Logs { * @param string $message Optional. The message associated with the event. * @param mixed $context Optional. Additional context data associated with the event. * @param int $timestamp Optional. The Unix timestamp of the log entry. Default is the current time. - * - * @return WP_Error|null + * @return bool True if the log was successfully saved, false otherwise. */ public static function log( $schedule_id, $action, $message = null, $context = null, $timestamp = null ) { + $events = wp_get_scheduled_events( Scheduled_Updates::PLUGIN_CRON_HOOK ); + if ( ! isset( $events[ $schedule_id ] ) ) { + return false; + } + if ( null === $timestamp ) { $timestamp = wp_date( 'U' ); } + $log_entry = array( 'timestamp' => intval( $timestamp ), 'action' => $action, @@ -62,17 +65,13 @@ public static function log( $schedule_id, $action, $message = null, $context = n $logs = get_option( self::OPTION_NAME, array() ); - if ( ! self::is_valid_schedule( $schedule_id ) ) { - return new WP_Error( 'invalid_schedule_id', 'Invalid schedule ID' ); - } - if ( ! isset( $logs[ $schedule_id ] ) ) { $logs[ $schedule_id ] = array(); } $logs[ $schedule_id ][] = $log_entry; - // Keep only the logs for the last MAX_RUNS_PER_SCHEDULE runs per schedule_id + // Keep only the logs for the last MAX_RUNS_PER_SCHEDULE runs per schedule_id. $start_count = 0; $last_two_runs = array(); for ( $i = count( $logs[ $schedule_id ] ) - 1; $i >= 0; $i-- ) { @@ -87,30 +86,31 @@ public static function log( $schedule_id, $action, $message = null, $context = n $last_two_runs = array_reverse( $last_two_runs ); $logs[ $schedule_id ] = $last_two_runs; - update_option( self::OPTION_NAME, $logs ); + return update_option( self::OPTION_NAME, $logs ); } /** * Retrieves the logs for a specific schedule_id or all logs if no schedule_id is provided. * + * If a schedule_id is provided, the logs for that specific schedule are returned. + * If no schedule_id is provided, all logs are returned, with each schedule_id as a key in the array. + * * @param string|null $schedule_id Optional. The ID of the schedule. If not provided, all logs will be returned. + * @return array { + * An array containing the logs, split by run. + * Each run is an array of log entries, where each log entry is an associative array containing the following keys: * - * @return array|WP_Error - * An array containing the logs, split by run. - * If a schedule_id is provided, the logs for that specific schedule are returned. - * If no schedule_id is provided, all logs are returned, with each schedule_id as a key in the array. - * Each run is an array of log entries, where each log entry is an associative array - * containing the following keys: - * - 'timestamp' (int): The Unix timestamp of the log entry. - * - 'action' (string): The action constant representing the event. - * - 'message' (string|null): The message associated with the event, if available. - * - 'context' (mixed|null): Additional context data associated with the event, if available. + * @type int $timestamp The Unix timestamp of the log entry. + * @type string $action The action constant representing the event. + * @type string|null $message The message associated with the event, if available. + * @type mixed|null $context Additional context data associated with the event, if available. + * } */ public static function get( $schedule_id = null ) { $logs = get_option( self::OPTION_NAME, array() ); if ( null === $schedule_id ) { - // Return all logs if no schedule_id is provided + // Return all logs if no schedule_id is provided. $all_logs = array(); foreach ( $logs as $schedule_id => $schedule_logs ) { $all_logs[ $schedule_id ] = self::split_logs_into_runs( $schedule_logs ); @@ -118,10 +118,6 @@ public static function get( $schedule_id = null ) { return $all_logs; } - if ( ! self::is_valid_schedule( $schedule_id ) ) { - return new WP_Error( 'invalid_schedule_id', 'Invalid schedule ID' ); - } - if ( ! isset( $logs[ $schedule_id ] ) ) { return array(); } @@ -134,25 +130,16 @@ public static function get( $schedule_id = null ) { * Clears the logs for a specific schedule_id or all logs if no schedule_id is provided. * * @param string|null $schedule_id Optional. The ID of the schedule. If not provided, all logs will be cleared. - * @param bool $skip_validation Optional. Whether to skip the validation of the schedule_id. - * - * @return WP_Error|null */ - public static function clear( string $schedule_id = null, $skip_validation = false ) { + public static function clear( string $schedule_id = null ) { $logs = get_option( self::OPTION_NAME, array() ); if ( null === $schedule_id ) { - // Clear all logs if no schedule_id is provided + // Clear all logs if no schedule_id is provided. $logs = array(); } else { - if ( ! $skip_validation && ! self::is_valid_schedule( $schedule_id ) ) { - return new WP_Error( 'invalid_schedule_id', 'Invalid schedule ID' ); - } - - if ( isset( $logs[ $schedule_id ] ) ) { - // Clear the logs for the specific schedule_id - unset( $logs[ $schedule_id ] ); - } + // Clear the logs for the specific schedule_id. + unset( $logs[ $schedule_id ] ); } update_option( self::OPTION_NAME, $logs ); @@ -239,22 +226,6 @@ private static function split_logs_into_runs( $logs ) { return $runs; } - /** - * Returns whether a schedule_id is valid. - * - * @param string $schedule_id The schedule id. - * @return bool - */ - private static function is_valid_schedule( $schedule_id ) { - $events = wp_get_scheduled_events( Scheduled_Updates::PLUGIN_CRON_HOOK ); - - if ( ! isset( $events[ $schedule_id ] ) ) { - return false; - } - - return true; - } - /** * Replaces the logs with the old schedule ID with new ones. * diff --git a/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php b/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php index 98e91b47cb083..615d7438fffce 100644 --- a/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php +++ b/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php @@ -418,9 +418,9 @@ public function add_log( $request ) { $message = $request['message']; $context = $request['context']; - $log = Scheduled_Updates_Logs::log( $schedule_id, $action, $message, $context ); + $success = Scheduled_Updates_Logs::log( $schedule_id, $action, $message, $context ); - if ( is_wp_error( $log ) ) { + if ( ! $success ) { return new WP_Error( 'rest_invalid_schedule', __( 'The schedule could not be found.', 'jetpack-scheduled-updates' ), array( 'status' => 404 ) ); } @@ -513,7 +513,7 @@ public function delete_item( $request, $clear_logs = true ) { // Delete logs if ( $clear_logs ) { - Scheduled_Updates_Logs::clear( $request['schedule_id'], true ); + Scheduled_Updates_Logs::clear( $request['schedule_id'] ); } return rest_ensure_response( true ); diff --git a/projects/packages/scheduled-updates/tests/php/class-scheduled-updates-logs-test.php b/projects/packages/scheduled-updates/tests/php/class-scheduled-updates-logs-test.php index 2105529afcefe..e70540b9c3268 100644 --- a/projects/packages/scheduled-updates/tests/php/class-scheduled-updates-logs-test.php +++ b/projects/packages/scheduled-updates/tests/php/class-scheduled-updates-logs-test.php @@ -7,8 +7,6 @@ namespace Automattic\Jetpack; -use WP_Error; - /** * Test class for Scheduled_Updates_Logs. * @@ -23,6 +21,13 @@ class Scheduled_Updates_Logs_Test extends \WorDBless\BaseTestCase { */ use \phpmock\phpunit\PHPMock; + /** + * Admin user ID. + * + * @var int + */ + public $admin_id; + /** * Set up before class. * @@ -31,7 +36,8 @@ class Scheduled_Updates_Logs_Test extends \WorDBless\BaseTestCase { */ public static function set_up_before_class() { parent::set_up_before_class(); - \phpmock\phpunit\PHPMock::defineFunctionMock( 'Automattic\Jetpack', 'realpath' ); + + static::defineFunctionMock( 'Automattic\Jetpack', 'realpath' ); Scheduled_Updates::init(); Scheduled_Updates::load_rest_api_endpoints(); } @@ -46,7 +52,7 @@ protected function set_up() { \WorDBless\Users::init()->clear_all_users(); Scheduled_Updates::init(); - // Initialize the admin + // Initialize the admin. $this->admin_id = wp_insert_user( array( 'user_login' => 'dumasdasdasmy_user', @@ -77,19 +83,19 @@ protected function tear_down() { public function test_log_and_get_logs() { $schedule_id = $this->create_schedule( 1 ); - // Test logging events + // Test logging events. Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates' ); Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATE_SUCCESS, 'Plugin updated successfully', array( 'plugin' => 'test-plugin' ) ); Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATES_SUCCESS, 'Plugin updates completed' ); - // Test retrieving logs + // Test retrieving logs. $logs = Scheduled_Updates_Logs::get( $schedule_id ); - // Assert that logs are split into runs correctly + // Assert that logs are split into runs correctly. $this->assertCount( 1, $logs ); $this->assertCount( 3, $logs[0] ); - // Assert log entry values + // Assert log entry values. $this->assertEquals( Scheduled_Updates_Logs::PLUGIN_UPDATES_START, $logs[0][0]['action'] ); $this->assertEquals( 'Starting plugin updates', $logs[0][0]['message'] ); $this->assertEquals( Scheduled_Updates_Logs::PLUGIN_UPDATE_SUCCESS, $logs[0][1]['action'] ); @@ -108,16 +114,16 @@ public function test_log_and_get_logs() { public function test_max_runs_per_schedule() { $schedule_id = $this->create_schedule( 1 ); - // Log events for more than MAX_RUNS_PER_SCHEDULE + // Log events for more than MAX_RUNS_PER_SCHEDULE. for ( $i = 1; $i <= Scheduled_Updates_Logs::MAX_RUNS_PER_SCHEDULE + 1; $i++ ) { Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, "Starting plugin updates (Run $i)" ); Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATES_SUCCESS, "Plugin updates completed (Run $i)" ); } - // Test retrieving logs + // Test retrieving logs. $logs = Scheduled_Updates_Logs::get( $schedule_id ); - // Assert that only the last MAX_RUNS_PER_SCHEDULE runs are kept + // Assert that only the last MAX_RUNS_PER_SCHEDULE runs are kept. $this->assertCount( Scheduled_Updates_Logs::MAX_RUNS_PER_SCHEDULE, $logs ); $this->assertEquals( 'Starting plugin updates (Run 2)', $logs[0][0]['message'] ); $this->assertEquals( 'Plugin updates completed (Run 2)', $logs[0][1]['message'] ); @@ -133,11 +139,12 @@ public function test_max_runs_per_schedule() { public function test_log_non_existent_schedule() { $schedule_id = 'non_existent_schedule'; - // Test retrieving logs for a non-existent schedule + // Test retrieving logs for a non-existent schedule. $result = Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates' ); - // Assert that an empty array is returned - $this->assertInstanceOf( WP_Error::class, $result ); + // Assert that nothing was logged. + $this->assertFalse( $result ); + $this->assertArrayNotHasKey( $schedule_id, get_option( Scheduled_Updates_Logs::OPTION_NAME, array() ) ); } /** @@ -148,11 +155,12 @@ public function test_log_non_existent_schedule() { public function test_get_logs_non_existent_schedule() { $schedule_id = 'non_existent_schedule'; - // Test retrieving logs for a non-existent schedule + // Test retrieving logs for a non-existent schedule. $logs = Scheduled_Updates_Logs::get( $schedule_id ); - // Assert that an empty array is returned - $this->assertInstanceOf( WP_Error::class, $logs ); + // Assert that an empty array is returned. + $this->assertIsArray( $logs ); + $this->assertEmpty( $logs ); } /** @@ -165,16 +173,16 @@ public function test_get_all_logs() { $schedule_id_1 = $this->create_schedule( 1 ); $schedule_id_2 = $this->create_schedule( 2 ); - // Log events for multiple schedules + // Log events for multiple schedules. Scheduled_Updates_Logs::log( $schedule_id_1, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates for schedule 1' ); Scheduled_Updates_Logs::log( $schedule_id_1, Scheduled_Updates_Logs::PLUGIN_UPDATES_SUCCESS, 'Plugin updates completed for schedule 1' ); Scheduled_Updates_Logs::log( $schedule_id_2, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates for schedule 2' ); Scheduled_Updates_Logs::log( $schedule_id_2, Scheduled_Updates_Logs::PLUGIN_UPDATES_SUCCESS, 'Plugin updates completed for schedule 2' ); - // Test retrieving all logs + // Test retrieving all logs. $all_logs = Scheduled_Updates_Logs::get(); - // Assert that logs for both schedules are returned + // Assert that logs for both schedules are returned. $this->assertArrayHasKey( $schedule_id_1, $all_logs ); $this->assertArrayHasKey( $schedule_id_2, $all_logs ); $this->assertCount( 1, $all_logs[ $schedule_id_1 ] ); @@ -196,29 +204,29 @@ public function test_clear_logs() { $schedule_id_1 = $this->create_schedule( 1 ); $schedule_id_2 = $this->create_schedule( 2 ); - // Log events for multiple schedules + // Log events for multiple schedules. Scheduled_Updates_Logs::log( $schedule_id_1, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates for schedule 1' ); Scheduled_Updates_Logs::log( $schedule_id_2, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates for schedule 2' ); - // Clear logs for a specific schedule + // Clear logs for a specific schedule. Scheduled_Updates_Logs::clear( $schedule_id_1 ); - // Test retrieving logs after clearing + // Test retrieving logs after clearing. $logs_schedule_1 = Scheduled_Updates_Logs::get( $schedule_id_1 ); $logs_schedule_2 = Scheduled_Updates_Logs::get( $schedule_id_2 ); - // Assert that logs for the cleared schedule are empty + // Assert that logs for the cleared schedule are empty. $this->assertEmpty( $logs_schedule_1 ); - // Assert that logs for the other schedule are still present + // Assert that logs for the other schedule are still present. $this->assertNotEmpty( $logs_schedule_2 ); - // Clear all logs + // Clear all logs. Scheduled_Updates_Logs::clear(); - // Test retrieving all logs after clearing + // Test retrieving all logs after clearing. $all_logs = Scheduled_Updates_Logs::get(); - // Assert that all logs are empty + // Assert that all logs are empty. $this->assertEmpty( $all_logs ); } @@ -230,26 +238,11 @@ public function test_clear_logs() { public function test_clear_logs_non_existent_schedule() { $schedule_id = 'non_existent_schedule'; - // Test retrieving logs for a non-existent schedule - $logs = Scheduled_Updates_Logs::clear( $schedule_id ); - - // Assert that an empty array is returned - $this->assertInstanceOf( WP_Error::class, $logs ); - } - - /** - * Test force clearing logs for a non-existent schedule ID. - * - * @covers ::get - */ - public function test_force_clear_logs_non_existent_schedule() { - $schedule_id = 'non_existent_schedule'; - - // Test retrieving logs for a non-existent schedule - $logs = Scheduled_Updates_Logs::clear( $schedule_id, true ); + // Test clearing logs for a non-existent schedule. + Scheduled_Updates_Logs::clear( $schedule_id ); - // Assert that null is returned - $this->assertNull( $logs ); + // Assert that there's no log for the non-existent schedule. + $this->assertArrayNotHasKey( $schedule_id, get_option( Scheduled_Updates_Logs::OPTION_NAME ) ); } /** @@ -262,7 +255,7 @@ public function test_delete_logs_after_scheduled_update_deletion() { Scheduled_Updates_Logs::log( $schedule_id, Scheduled_Updates_Logs::PLUGIN_UPDATES_START, 'Starting plugin updates' ); - // Test retrieving logs + // Test retrieving logs. $logs = Scheduled_Updates_Logs::get( $schedule_id ); $this->assertCount( 1, $logs ); @@ -276,7 +269,8 @@ public function test_delete_logs_after_scheduled_update_deletion() { $logs = Scheduled_Updates_Logs::get( $schedule_id ); - $this->assertInstanceOf( WP_Error::class, $logs ); + $this->assertIsArray( $logs ); + $this->assertEmpty( $logs ); $schedule_id = $this->create_schedule( 1 );