Prune duplicate recurring events (#251)

WPprodigy · web-flow · commit 83631b48e7de · 2022-01-24T14:56:21.000-06:00
diff --git a/README.md b/README.md
@@ -25,6 +25,13 @@ By default the plugin disables default WP cron processing. It is recommended to
 
 ## Frequently Asked Questions ##
 
+### Deviations from WordPress Core ###
+
+* Cron jobs are stored in a custom table and not in the `cron` option in wp_options. As long relevent code uses WP core functions for retrieving events and not direct SQL, all will stay compatible.
+* Duplicate recurring events with the same action/args/schedule are prevented. If multiple of the same action is needed on the same schedule, can add an arbitrary number to the args array.
+* When the cron control runner is running events, it does so via WP CLI. So the environment can be slightly different than that of a normal web request.
+* The cron control runner can process multiple events in parallel, whereas core cron only did 1 at a time. By default, events with the same action will not run in parallel unless specifically granted permission to do so.
+
 ### Adding Internal Events ###
 
 **This should be done sparingly as "Internal Events" bypass certain locks and limits built into the plugin.** Overuse will lead to unexpected resource usage, and likely resource exhaustion.
diff --git a/__tests__/unit-tests/test-events-store.php b/__tests__/unit-tests/test-events-store.php
@@ -203,4 +203,33 @@ public function test_query_raw_events() {
 		$this->assertEquals( 1, count( $result ), 'returned event from second page' );
 		$this->assertEquals( $event_two->get_timestamp(), $result[0]->timestamp, 'found the right event' );
 	}
+
+	public function test_query_raw_events_orderby() {
+		$store = Events_Store::instance();
+
+		$event_one   = Utils::create_test_event( [ 'timestamp' => 5, 'action' => 'test_query_raw_events_orderby' ] );
+		$event_two   = Utils::create_test_event( [ 'timestamp' => 2, 'action' => 'test_query_raw_events_orderby' ] );
+		$event_three = Utils::create_test_event( [ 'timestamp' => 3, 'action' => 'test_query_raw_events_orderby' ] );
+		$event_four  = Utils::create_test_event( [ 'timestamp' => 1, 'action' => 'test_query_raw_events_orderby' ] );
+
+		// Default orderby should be timestamp ASC
+		$result = $store->_query_events_raw();
+		$this->assertEquals( 4, count( $result ), 'returned the correct amount of events' );
+		$this->assertEquals( $event_four->get_timestamp(), $result[0]->timestamp, 'the oldest "due now" event is returned first' );
+
+		// Fetch by timestamp in descending order.
+		$result = $store->_query_events_raw( [ 'orderby' => 'timestamp', 'order' => 'desc' ] );
+		$this->assertEquals( 4, count( $result ), 'returned the correct amount of events' );
+		$this->assertEquals( $event_one->get_timestamp(), $result[0]->timestamp, 'the farthest "due now" event is returned first' );
+
+		// Fetch by ID in ascending order.
+		$result = $store->_query_events_raw( [ 'orderby' => 'ID', 'order' => 'asc' ] );
+		$this->assertEquals( 4, count( $result ), 'returned the correct amount of events' );
+		$this->assertEquals( $event_one->get_id(), $result[0]->ID, 'the lowest ID is returned first' );
+
+		// Fetch by ID in descending order.
+		$result = $store->_query_events_raw( [ 'orderby' => 'ID', 'order' => 'desc' ] );
+		$this->assertEquals( 4, count( $result ), 'returned the correct amount of events' );
+		$this->assertEquals( $event_four->get_id(), $result[0]->ID, 'the highest ID is returned first' );
+	}
 }
diff --git a/__tests__/unit-tests/test-internal-events.php b/__tests__/unit-tests/test-internal-events.php
@@ -61,4 +61,35 @@ function test_migrate_legacy_cron_events() {
 		$cron_row = $wpdb->get_row( "SELECT * FROM $wpdb->options WHERE option_name = 'cron'" );
 		$this->assertNull( $cron_row, 'cron option was deleted' );
 	}
+
+	function test_prune_duplicate_events() {
+		// We don't prune single events, even if duplicates.
+		$original_single_event  = Utils::create_test_event( [ 'timestamp' => time(), 'action' => 'single_event', 'args' => [ 'same' ] ] );
+		$duplicate_single_event = Utils::create_test_event( [ 'timestamp' => time() + 100, 'action' => 'single_event', 'args' => [ 'same' ] ] );
+		$unique_single_event    = Utils::create_test_event( [ 'timestamp' => time() + 200, 'action' => 'single_event', 'args' => [ 'unique' ] ] );
+
+		// We do prune duplicate recurring events.
+		$original_recurring_event   = Utils::create_test_event( [ 'timestamp' => time() + 500, 'action' => 'recurring_event', 'schedule' => 'hourly', 'interval' => \HOUR_IN_SECONDS ] );
+		$duplicate_recurring_event  = Utils::create_test_event( [ 'timestamp' => time() + 100, 'action' => 'recurring_event', 'schedule' => 'hourly', 'interval' => \HOUR_IN_SECONDS ] );
+		$duplicate_recurring_event2 = Utils::create_test_event( [ 'timestamp' => time() + 200, 'action' => 'recurring_event', 'schedule' => 'hourly', 'interval' => \HOUR_IN_SECONDS ] );
+		$unique_recurring_event     = Utils::create_test_event( [ 'timestamp' => time() + 100, 'action' => 'recurring_event', 'schedule' => 'hourly', 'interval' => \HOUR_IN_SECONDS, 'args' => [ 'unique' ] ] );
+
+		// Run the pruning.
+		Cron_Control\Internal_Events::instance()->clean_legacy_data();
+
+		// Should have 5 events left, and the oldest IDs should have been kept..
+		$remaining_events = Cron_Control\Events::query( [ 'limit' => 100, 'orderby' => 'ID', 'order' => 'ASC' ] );
+		$this->assertEquals( 5, count( $remaining_events ), 'correct number of registered events left after pruning' );
+		$this->assertEquals( $remaining_events[0]->get_id(), $original_single_event->get_id(), 'original single event was kept' );
+		$this->assertEquals( $remaining_events[1]->get_id(), $duplicate_single_event->get_id(), 'duplicate single event was also kept' );
+		$this->assertEquals( $remaining_events[2]->get_id(), $unique_single_event->get_id(), 'unique single event was kept' );
+		$this->assertEquals( $remaining_events[3]->get_id(), $original_recurring_event->get_id(), 'original recurring event was kept' );
+		$this->assertEquals( $remaining_events[4]->get_id(), $unique_recurring_event->get_id(), 'unique recurring event was kept' );
+
+		// The two duplicates should be marked as completed now.
+		$duplicate_recurring_1 = Cron_Control\Event::get( $duplicate_recurring_event->get_id() );
+		$duplicate_recurring_2 = Cron_Control\Event::get( $duplicate_recurring_event2->get_id() );
+		$this->assertEquals( $duplicate_recurring_1->get_status(), Cron_Control\Events_Store::STATUS_COMPLETED, 'duplicate recurring event 1 was marked as completed' );
+		$this->assertEquals( $duplicate_recurring_2->get_status(), Cron_Control\Events_Store::STATUS_COMPLETED, 'duplicate recurring event 2 was marked as completed' );
+	}
 }
diff --git a/includes/class-events-store.php b/includes/class-events-store.php
@@ -633,6 +633,10 @@ public function _query_events_raw( array $args = [] ): array {
 				'default'    => 1,
 				'validation' => fn( $page ) => is_int( $page ) && $page >= 1,
 			],
+			'orderby' => [
+				'default'    => 'timestamp',
+				'validation' => fn( $orderby ) => is_null( $orderby ) || ( is_string( $orderby ) && in_array( $orderby, [ 'timestamp', 'ID' ], true ) ),
+			],
 			'order' => [
 				'default'    => 'ASC',
 				'validation' => fn( $order ) => is_string( $order ) && in_array( strtoupper( $order ), [ 'ASC', 'DESC'], true ),
@@ -716,9 +720,10 @@ public function _query_events_raw( array $args = [] ): array {
 			}
 		}
 
-		// TODO: adjust for situations where we don't need to sort.
-		$sql .= ' ORDER BY timestamp';
-		$sql .= strtoupper( $parsed_args['order'] ) === 'ASC' ? ' ASC' : ' DESC';
+		if ( ! is_null( $parsed_args['orderby'] ) ) {
+			$sql .= ' ORDER BY ' . $parsed_args['orderby'];
+			$sql .= strtoupper( $parsed_args['order'] ) === 'ASC' ? ' ASC' : ' DESC';
+		}
 
 		// Skip paging/limits if "-1" was passed to get all events.
 		if ( $parsed_args['limit'] >= 1 ) {
@@ -740,7 +745,7 @@ public function _query_events_raw( array $args = [] ): array {
 
 		// Conditionally use a more specific cache for common FE queries, helping avoid most bulk invalidations.
 		$allowed_arg_count = array_key_exists( 'timestamp', $args ) ? 4 : 3;
-		if ( 1 === $args['limit'] && count( $args ) === $allowed_arg_count ) {
+		if ( isset( $args['limit'] ) && 1 === $args['limit'] && count( $args ) === $allowed_arg_count ) {
 			$has_timestamp = array_key_exists( 'timestamp', $args ) && ! is_null( $args['timestamp'] );
 
 			// Request was for the next event based on action/args, i.e. wp_next_scheduled()
diff --git a/includes/class-internal-events.php b/includes/class-internal-events.php
@@ -216,6 +216,7 @@ public function clean_legacy_data() {
 		// While this plugin doesn't use this locking mechanism, other code may check the value.
 		delete_transient( 'doing_cron' );
 
+		$this->prune_duplicate_events();
 		$this->ensure_internal_events_have_correct_schedule();
 	}
 
@@ -255,6 +256,34 @@ private function migrate_legacy_cron_events() {
 		}
 	}
 
+	// Recurring events that have the same action/args/schedule are unnecessary. We can safely remove them.
+	private function prune_duplicate_events() {
+		$events = Events::query( [ 'limit' => -1, 'orderby' => 'ID', 'order' => 'ASC' ] );
+
+		$original_events  = [];
+		$duplicate_events = [];
+		foreach ( $events as $event ) {
+			if ( ! $event->is_recurring() ) {
+				// Only interested in recurring events.
+				continue;
+			}
+
+			$unique_key = sha1( "{$event->get_action()}:{$event->get_instance()}:{$event->get_schedule()}" );
+			if ( ! isset( $original_events[ $unique_key ] ) ) {
+				// The first occurrence, will also be the oldest (lowest ID).
+				$original_events[ $unique_key ] = true;
+			} else {
+				// Found a duplicate!
+				$duplicate_events[] = $event;
+			}
+		}
+
+		foreach ( $duplicate_events as $duplicate_event ) {
+			// For now we'll just complete them.
+			$duplicate_event->complete();
+		}
+	}
+
 	private function ensure_internal_events_have_correct_schedule() {
 		$schedules = wp_get_schedules();
 
diff --git a/languages/cron-control.pot b/languages/cron-control.pot
@@ -4,7 +4,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: Cron Control 3.0\n"
 "Report-Msgid-Bugs-To: https://wordpress.org/support/plugin/cron-control\n"
-"POT-Creation-Date: 2022-01-19 22:39:12+00:00\n"
+"POT-Creation-Date: 2022-01-20 03:55:29+00:00\n"
 "MIME-Version: 1.0\n"
 "Content-Type: text/plain; charset=utf-8\n"
 "Content-Transfer-Encoding: 8bit\n"
@@ -58,11 +58,11 @@ msgstr ""
 msgid "Job with action `%1$s` and arguments `%2$s` executed."
 msgstr ""
 
-#: includes/class-internal-events.php:110
+#: includes/class-internal-events.php:90
 msgid "Cron Control internal job - every 2 minutes (used to be 1 minute)"
 msgstr ""
 
-#: includes/class-internal-events.php:114
+#: includes/class-internal-events.php:94
 msgid "Cron Control internal job - every 10 minutes"
 msgstr ""
 
diff --git a/readme.txt b/readme.txt
@@ -25,6 +25,13 @@ By default the plugin disables default WP cron processing. It is recommended to
 
 == Frequently Asked Questions ==
 
+= Deviations from WordPress Core =
+
+* Cron jobs are stored in a custom table and not in the `cron` option in wp_options. As long relevent code uses WP core functions for retrieving events and not direct SQL, all will stay compatible.
+* Duplicate recurring events with the same action/args/schedule are prevented. If multiple of the same action is needed on the same schedule, can add an arbitrary number to the args array.
+* When the cron control runner is running events, it does so via WP CLI. So the environment can be slightly different than that of a normal web request.
+* The cron control runner can process multiple events in parallel, whereas core cron only did 1 at a time. By default, events with the same action will not run in parallel unless specifically granted permission to do so.
+
 = Adding Internal Events =
 
 **This should be done sparingly as "Internal Events" bypass certain locks and limits built into the plugin.** Overuse will lead to unexpected resource usage, and likely resource exhaustion.
