check both stores for an action while migration is in progress

[rrennick]

Closes #486

This PR updates the Hybrid data store to use the primary & secondary data stores' fetch_action to determine whether an action ID exists in that store before using the store to retrieve action data. Both data stores implement exception handling in fetch_action and return a NullAction action when the action is not found/valid.

• Determine the minimum action ID in the tables data store SELECT MIN(action_id) FROM wp_actionscheduler_actions;
• Ensure your migration is complete
• Create a number of pending actions

add_action( 'init', function() {
	for ( $i = 1; $i < 200; $i++ )
	as_enqueue_async_action( 'as-async-test' . $i );
} );

• Add a log to check that actions are being processed before the migration action is processed

add_action( 'action_scheduler_after_execute', function( $action_id, $action ) {
	error_log( $action->get_hook() );
}, 10, 2 );

• Update your demarkation ID to be 10000 greater than the maximum action_id in the actions table
  wp option update action_scheduler_hybrid_store_demarkation 123456
• Reset the migration complete flag wp option delete action_scheduler_migration_status
• Load the Tools -> Scheduled Actions screen
• Check the Pending/Completed list for the test actions to be processed
• Migration should complete
• No errors or warnings should be logged

[rrennick]

@thenbrent Could you give this one a sanity check where I added null return values?

[thenbrent]

This is a bit of a dangerous change, but it looks good @rrennick. I like the introduction of a central get_store_from_action_id() method.

The reason I says it's a dangerous changes is because prior to this patch, if an action had an $action_id < $this->demarkation_id, it had to exist in the old, post data store for it to be run. Now it can exist in either and still be run.

Processing actions under new conditions creates the risk of processing an action when it shouldn't be run. I can't see any conditions where this would cause a problem. Even when considering corner-cases like upgrade-downgrade-upgrade. However, I want to mention the risk as it should be considered before merging.

[becdetat]

I'm unqualified to talk about the risk but I followed the instructions and I'm pretty sure it tested ok.

[rrennick]

Now it can exist in either and still be run.

@thenbrent Thanks, good point.

@becdetat Thanks for the feedback :)

[rrennick]

@jeffstieler This is ready for a review.

[jeffstieler]

This tested well with the provided instructions, but there seems to be some copy-pasta in the get_store_from_action_id() logic.

[rrennick]

@jeffstieler This is ready for another review.

[jeffstieler]

Code looks good and (still) tests well.