Refactor cron event to validate individual URLs

includes/uninstall-functions.php

@@ -19,6 +19,7 @@ function delete_options() {
 	delete_option( 'amp-options' );
 	delete_option( 'amp_css_transient_monitor_time_series' );
 	delete_option( 'amp_customize_setting_modified_timestamps' );
+	delete_option( 'amp_url_validation_queue' ); // See Validation\URLValidationCron::OPTION_KEY.
 
 	$theme_mod_name = 'amp_customize_setting_modified_timestamps';
 	remove_theme_mod( $theme_mod_name );

src/BackgroundTask/CronBasedBackgroundTask.php

@@ -22,6 +22,7 @@ abstract class CronBasedBackgroundTask implements Service, Registerable {
 	const DEFAULT_INTERVAL_HOURLY      = 'hourly';
 	const DEFAULT_INTERVAL_TWICE_DAILY = 'twicedaily';
 	const DEFAULT_INTERVAL_DAILY       = 'daily';
+	const DEFAULT_INTERVAL_WEEKLY      = 'weekly';
 
 	/**
 	 * BackgroundTaskDeactivator instance.

src/BackgroundTask/RecurringBackgroundTask.php

@@ -37,13 +37,19 @@ final public function schedule_event( ...$args ) {
 		}
 
 		$event_name = $this->get_event_name();
-		$timestamp  = wp_next_scheduled( $event_name, $args );
+		$recurrence = $this->get_interval();
 
-		if ( $timestamp ) {
-			return;
+		$scheduled_event = wp_get_scheduled_event( $event_name, $args );
+
+		// Unschedule any existing event which had a differing recurrence.
+		if ( $scheduled_event && $scheduled_event->schedule !== $recurrence ) {
+			wp_unschedule_event( $scheduled_event->timestamp, $event_name, $args );
+			$scheduled_event = null;
 		}
 
-		wp_schedule_event( time(), $this->get_interval(), $event_name, $args );
+		if ( ! $scheduled_event ) {
+			wp_schedule_event( time(), $recurrence, $event_name, $args );
+		}
 	}
 
 	/**

src/Validation/SavePostValidationEvent.php

@@ -49,7 +49,26 @@ final class SavePostValidationEvent extends SingleScheduledBackgroundTask implem
 	 * @return bool Whether needed.
 	 */
 	public static function is_needed() {
-		return URLValidationCron::is_needed();
+
+		/**
+		 * Filters whether to enable URL validation cron tasks.
+		 *
+		 * This is a feature flag used to control whether the sample set of site URLs are scanned on a daily basis and
+		 * whether post permalinks are scheduled for immediate validation as soon as they are updated by a user who has
+		 * DevTools turned off. This conditional flag will be removed once Site Scanning is implemented, likely in v2.2.
+		 *
+		 * @link https://github.com/ampproject/amp-wp/issues/5750
+		 * @link https://github.com/ampproject/amp-wp/issues/4779
+		 * @link https://github.com/ampproject/amp-wp/issues/4795
+		 * @link https://github.com/ampproject/amp-wp/issues/4719
+		 * @link https://github.com/ampproject/amp-wp/issues/5671
+		 * @link https://github.com/ampproject/amp-wp/issues/5101
+		 * @link https://github.com/ampproject/amp-wp/issues?q=label%3A%22Site+Scanning%22
+		 *
+		 * @param bool $enabled Enabled.
+		 * @internal
+		 */
+		return apply_filters( 'amp_temp_validation_cron_tasks_enabled', false );
 	}
 
 	/**

src/Validation/URLValidationCron.php

@@ -3,14 +3,14 @@
  * WP cron process to validate URLs in the background.
  *
  * @package AMP
- * @since 2.1
+ * @since   2.1
  */
 
 namespace AmpProject\AmpWP\Validation;
 
 use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator;
 use AmpProject\AmpWP\BackgroundTask\RecurringBackgroundTask;
-use AmpProject\AmpWP\Infrastructure\Conditional;
+use AMP_Validation_Manager;
 
 /**
  * URLValidationCron class.
@@ -19,75 +19,40 @@
  *
  * @internal
  */
-final class URLValidationCron extends RecurringBackgroundTask implements Conditional {
-
-	/**
-	 * ScannableURLProvider instance.
-	 *
-	 * @var ScannableURLProvider
-	 */
-	private $scannable_url_provider;
-
-	/**
-	 * URLValidationProvider instance.
-	 *
-	 * @var URLValidationProvider
-	 */
-	private $url_validation_provider;
+final class URLValidationCron extends RecurringBackgroundTask {
 
 	/**
 	 * The cron action name.
 	 *
+	 * Note that only one queued URL is currently validated at a time.
+	 *
 	 * @var string
 	 */
 	const BACKGROUND_TASK_NAME = 'amp_validate_urls';
 
 	/**
-	 * The length of time, in seconds, to sleep between each URL validation.
+	 * Option key to store queue for URL validation.
 	 *
-	 * @var int
+	 * @var string
 	 */
-	const DEFAULT_SLEEP_TIME = 1;
+	const OPTION_KEY = 'amp_url_validation_queue';
 
 	/**
-	 * Check whether the service is currently needed.
+	 * ScannableURLProvider instance.
 	 *
-	 * @return bool Whether needed.
+	 * @var ScannableURLProvider
 	 */
-	public static function is_needed() {
-		/**
-		 * Filters whether to enable URL validation cron tasks.
-		 *
-		 * This is a feature flag used to control whether the sample set of site URLs are scanned on a daily basis and
-		 * whether post permalinks are scheduled for immediate validation as soon as they are updated by a user who has
-		 * DevTools turned off. This conditional flag will be removed once Site Scanning is implemented, likely in v2.2.
-		 *
-		 * @link https://github.com/ampproject/amp-wp/issues/5750
-		 * @link https://github.com/ampproject/amp-wp/issues/4779
-		 * @link https://github.com/ampproject/amp-wp/issues/4795
-		 * @link https://github.com/ampproject/amp-wp/issues/4719
-		 * @link https://github.com/ampproject/amp-wp/issues/5671
-		 * @link https://github.com/ampproject/amp-wp/issues/5101
-		 * @link https://github.com/ampproject/amp-wp/issues?q=label%3A%22Site+Scanning%22
-		 *
-		 * @param bool $enabled Enabled.
-		 * @internal
-		 */
-		return apply_filters( 'amp_temp_validation_cron_tasks_enabled', false );
-	}
+	private $scannable_url_provider;
 
 	/**
-	 * Class constructor.
+	 * Constructor.
 	 *
 	 * @param BackgroundTaskDeactivator $background_task_deactivator Service that deactivates background events.
-	 * @param ScannableURLProvider      $scannable_url_provider ScannableURLProvider instance.
-	 * @param URLValidationProvider     $url_validation_provider URLValidationProvider instance.
+	 * @param ScannableURLProvider      $scannable_url_provider      ScannableURLProvider instance.
 	 */
-	public function __construct( BackgroundTaskDeactivator $background_task_deactivator, ScannableURLProvider $scannable_url_provider, URLValidationProvider $url_validation_provider ) {
+	public function __construct( BackgroundTaskDeactivator $background_task_deactivator, ScannableURLProvider $scannable_url_provider ) {
 		parent::__construct( $background_task_deactivator );
-
-		$this->scannable_url_provider  = $scannable_url_provider;
-		$this->url_validation_provider = $url_validation_provider;
+		$this->scannable_url_provider = $scannable_url_provider;
 	}
 
 	/**
@@ -96,15 +61,49 @@ public function __construct( BackgroundTaskDeactivator $background_task_deactiva
 	 * @param mixed[] ...$args Unused callback arguments.
 	 */
 	public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
-		$urls       = $this->scannable_url_provider->get_urls();
-		$sleep_time = $this->get_sleep_time();
+		$url = $this->dequeue();
+		if ( $url ) {
+			AMP_Validation_Manager::validate_url_and_store( $url );
+		}
+	}
 
-		foreach ( $urls as $url ) {
-			$this->url_validation_provider->get_url_validation( $url['url'], $url['type'] );
-			if ( $sleep_time ) {
-				sleep( $sleep_time );
+	/**
+	 * Dequeue to obtain URL to validate.
+	 *
+	 * @return string|null URL to validate or null if there is nothing queued up.
+	 */
+	protected function dequeue() {
+		$data = get_option( self::OPTION_KEY, [] );
+		if ( ! is_array( $data ) ) {
+			$data = [];
+		}
+		$data = array_merge(
+			[
+				'timestamp' => 0,
+				'urls'      => [],
+			],
+			$data
+		);
+
+		// If there are no URLs queued, then obtain a new set.
+		if ( empty( $data['urls'] ) ) {
+
+			// If it has been less than a week since the last enqueueing, then do nothing.
+			if ( time() - $data['timestamp'] < WEEK_IN_SECONDS ) {
+				return null;
 			}
+
+			// @todo The URLs should be contextual based on the selected template mode, in particular only singular URLs should be included if using legacy Reader mode.
+			$data['urls']      = wp_list_pluck( $this->scannable_url_provider->get_urls(), 'url' );
+			$data['timestamp'] = time();
 		}
+
+		// If there is not a queued URL, then enqueue a new set of URLs.
+		$url = array_shift( $data['urls'] );
+
+		update_option( self::OPTION_KEY, $data );
+
+		return $url ?: null;
 	}
 
 	/**
@@ -126,22 +125,6 @@ protected function get_event_name() {
 	 * @return string An existing interval name.
 	 */
 	protected function get_interval() {
-		return self::DEFAULT_INTERVAL_DAILY;
-	}
-
-	/**
-	 * Provides the length of time, in seconds, to sleep between validating URLs.
-	 *
-	 * @return int
-	 */
-	private function get_sleep_time() {
-
-		/**
-		 * Filters the length of time to sleep between validating URLs.
-		 *
-		 * @since 2.1
-		 * @param int The number of seconds. Default 1. Setting to 0 or a negative numbers disables all throttling.
-		 */
-		return max( (int) apply_filters( 'amp_url_validation_sleep_time', self::DEFAULT_SLEEP_TIME ), 0 );
+		return self::DEFAULT_INTERVAL_HOURLY;
 	}
 }

tests/php/src/Validation/URLValidationCronTest.php

@@ -2,21 +2,17 @@
 
 namespace AmpProject\AmpWP\Tests\Validation;
 
-use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator;
 use AmpProject\AmpWP\BackgroundTask\CronBasedBackgroundTask;
-use AmpProject\AmpWP\Infrastructure\Conditional;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
+use AmpProject\AmpWP\Tests\DependencyInjectedTestCase;
 use AmpProject\AmpWP\Tests\Helpers\PrivateAccess;
 use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking;
-use AmpProject\AmpWP\Tests\TestCase;
 use AmpProject\AmpWP\Validation\ScannableURLProvider;
-use AmpProject\AmpWP\Validation\URLScanningContext;
 use AmpProject\AmpWP\Validation\URLValidationCron;
-use AmpProject\AmpWP\Validation\URLValidationProvider;
 
 /** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationCron */
-final class URLValidationCronTest extends TestCase {
+final class URLValidationCronTest extends DependencyInjectedTestCase {
 	use ValidationRequestMocking, PrivateAccess;
 
 	/**
@@ -26,15 +22,24 @@ final class URLValidationCronTest extends TestCase {
 	 */
 	private $test_instance;
 
+	/** @var int */
+	private $request_count = 0;
+
 	/**
 	 * Setup.
 	 *
 	 * @inheritdoc
 	 */
 	public function setUp() {
 		parent::setUp();
-		$this->test_instance = new URLValidationCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ), new URLValidationProvider() );
-		add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] );
+		$this->test_instance = $this->injector->make( URLValidationCron::class );
+		add_filter(
+			'pre_http_request',
+			function () {
+				$this->request_count++;
+				return $this->get_validate_response();
+			}
+		);
 	}
 
 	/**
@@ -46,24 +51,13 @@ public function test_register() {
 		$this->assertInstanceof( URLValidationCron::class, $this->test_instance );
 		$this->assertInstanceof( Service::class, $this->test_instance );
 		$this->assertInstanceof( Registerable::class, $this->test_instance );
-		$this->assertInstanceof( Conditional::class, $this->test_instance );
 
 		$this->test_instance->register();
 
 		$this->assertEquals( 10, has_action( 'admin_init', [ $this->test_instance, 'schedule_event' ] ) );
 		$this->assertEquals( 10, has_action( URLValidationCron::BACKGROUND_TASK_NAME, [ $this->test_instance, 'process' ] ) );
 	}
 
-	/** @covers ::is_needed() */
-	public function test_is_needed() {
-		$this->assertFalse( URLValidationCron::is_needed() );
-
-		add_filter( 'amp_temp_validation_cron_tasks_enabled', '__return_true' );
-		$this->assertTrue( URLValidationCron::is_needed() );
-
-		remove_filter( 'amp_temp_validation_cron_tasks_enabled', '__return_true' );
-	}
-
 	/** @covers ::schedule_event() */
 	public function test_schedule_event_with_no_user() {
 		$event_name = $this->call_private_method( $this->test_instance, 'get_event_name' );
@@ -102,15 +96,48 @@ public function test_schedule_event_with_user_with_permission() {
 	 * Test validate_urls.
 	 *
 	 * @covers ::process()
-	 * @covers ::get_sleep_time()
+	 * @covers ::dequeue()
 	 */
 	public function test_validate_urls() {
-		$this->factory()->post->create_many( 5 );
-
-		add_filter( 'amp_url_validation_sleep_time', '__return_false' );
+		/** @var ScannableURLProvider $scannable_url_provider */
+		$scannable_url_provider = $this->get_private_property( $this->test_instance, 'scannable_url_provider' );
+
+		$initial_urls      = wp_list_pluck( $scannable_url_provider->get_urls(), 'url' );
+		$initial_url_count = count( $initial_urls );
+		$this->assertGreaterThan( 0, $initial_url_count );
+
+		delete_option( URLValidationCron::OPTION_KEY );
+
+		// Verify that processing will enqueue URLs (if none are queued) and process one.
+		for ( $i = 1; $i <= $initial_url_count; $i++ ) {
+			$this->test_instance->process();
+			$this->assertEquals( $i, $this->request_count );
+			$data = get_option( URLValidationCron::OPTION_KEY );
+			$this->assertArrayHasKey( 'urls', $data );
+			$this->assertArrayHasKey( 'timestamp', $data );
+			$this->assertLessThanOrEqual( time(), $data['timestamp'] );
+			$this->assertEquals(
+				array_slice( $initial_urls, $i ),
+				$data['urls']
+			);
+		}
+
+		// Ensure no URLs are queued or processed if timestamp is less than a week.
+		$data = get_option( URLValidationCron::OPTION_KEY );
+		$this->assertCount( 0, $data['urls'] );
+		$before_request_count = $this->request_count;
+		$this->test_instance->process();
+		$this->assertEquals( $before_request_count, $this->request_count );
+		$data = get_option( URLValidationCron::OPTION_KEY );
+		$this->assertCount( 0, $data['urls'] );
 
+		// Now test that after a week has transpired, the next set of URLs are re-queued and one is processed.
+		$data['timestamp'] = time() - WEEK_IN_SECONDS - MINUTE_IN_SECONDS;
+		update_option( URLValidationCron::OPTION_KEY, $data );
 		$this->test_instance->process();
-		$this->assertCount( 10, $this->get_validated_urls() );
+		$this->assertEquals( $before_request_count + 1, $this->request_count );
+		$data = get_option( URLValidationCron::OPTION_KEY );
+		$this->assertCount( $initial_url_count - 1, $data['urls'] );
 	}
 
 	/** @covers ::get_event_name() */
@@ -124,7 +151,7 @@ public function test_get_event_name() {
 	/** @covers ::get_interval() */
 	public function test_get_interval() {
 		$this->assertEquals(
-			URLValidationCron::DEFAULT_INTERVAL_DAILY,
+			URLValidationCron::DEFAULT_INTERVAL_HOURLY,
 			$this->call_private_method( $this->test_instance, 'get_interval' )
 		);
 	}