From 69cb26f986375c3a3eea8ce28e4e93058bc4672c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Feb 2021 15:58:13 -0800 Subject: [PATCH 1/3] Put SavePostValidationEvent and URLValidationCron behind a feature flag filter --- src/Validation/SavePostValidationEvent.php | 12 ++++++++- src/Validation/URLValidationCron.php | 30 +++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/Validation/SavePostValidationEvent.php b/src/Validation/SavePostValidationEvent.php index ba9d18c192c..f096b7a31a7 100644 --- a/src/Validation/SavePostValidationEvent.php +++ b/src/Validation/SavePostValidationEvent.php @@ -11,6 +11,7 @@ use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\SingleScheduledBackgroundTask; use AmpProject\AmpWP\DevTools\UserAccess; +use AmpProject\AmpWP\Infrastructure\Conditional; /** * SavePostValidationEvent class. @@ -19,7 +20,7 @@ * * @internal */ -final class SavePostValidationEvent extends SingleScheduledBackgroundTask { +final class SavePostValidationEvent extends SingleScheduledBackgroundTask implements Conditional { /** * Instance of URLValidationProvider @@ -42,6 +43,15 @@ final class SavePostValidationEvent extends SingleScheduledBackgroundTask { */ const BACKGROUND_TASK_NAME = 'amp_single_post_validate'; + /** + * Check whether the service is currently needed. + * + * @return bool Whether needed. + */ + public static function is_needed() { + return URLValidationCron::is_needed(); + } + /** * Class constructor. * diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index e2784b7512b..1dddd77f64b 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -10,6 +10,7 @@ use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\RecurringBackgroundTask; +use AmpProject\AmpWP\Infrastructure\Conditional; /** * URLValidationCron class. @@ -18,7 +19,7 @@ * * @internal */ -final class URLValidationCron extends RecurringBackgroundTask { +final class URLValidationCron extends RecurringBackgroundTask implements Conditional { /** * ScannableURLProvider instance. @@ -48,6 +49,33 @@ final class URLValidationCron extends RecurringBackgroundTask { */ const DEFAULT_SLEEP_TIME = 1; + /** + * Check whether the service is currently needed. + * + * @return bool Whether needed. + */ + 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 ); + } + /** * Class constructor. * From 1dcd3f6c19e62559a1a036967bf161ca0636f547 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Feb 2021 15:59:17 -0800 Subject: [PATCH 2/3] Add comment for needed change to should_schedule_event --- src/Validation/SavePostValidationEvent.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Validation/SavePostValidationEvent.php b/src/Validation/SavePostValidationEvent.php index f096b7a31a7..258c5d760c5 100644 --- a/src/Validation/SavePostValidationEvent.php +++ b/src/Validation/SavePostValidationEvent.php @@ -121,6 +121,7 @@ protected function should_schedule_event( $args ) { $post = get_post( $id ); + // @todo This needs to be limited to when the status is publish because otherwise the validation request will fail to be able to access the post, as the request is not authenticated. if ( ! $post || wp_is_post_revision( $post ) From f12c5d555ac3c56dc6be6152dea8497253dae8a7 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 18 Feb 2021 11:20:57 -0500 Subject: [PATCH 3/3] Test that cron services are conditional --- .../src/Validation/SavePostValidationEventTest.php | 12 ++++++++++++ tests/php/src/Validation/URLValidationCronTest.php | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tests/php/src/Validation/SavePostValidationEventTest.php b/tests/php/src/Validation/SavePostValidationEventTest.php index 2e42f6d272a..a9b459057a0 100644 --- a/tests/php/src/Validation/SavePostValidationEventTest.php +++ b/tests/php/src/Validation/SavePostValidationEventTest.php @@ -8,6 +8,7 @@ use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\SingleScheduledBackgroundTask; use AmpProject\AmpWP\DevTools\UserAccess; +use AmpProject\AmpWP\Infrastructure\Conditional; use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; @@ -49,6 +50,17 @@ public function test__construct() { $this->assertInstanceof( SavePostValidationEvent::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 ); + } + + /** @covers ::is_needed() */ + public function test_is_needed() { + $this->assertFalse( SavePostValidationEvent::is_needed() ); + + add_filter( 'amp_temp_validation_cron_tasks_enabled', '__return_true' ); + $this->assertTrue( SavePostValidationEvent::is_needed() ); + + remove_filter( 'amp_temp_validation_cron_tasks_enabled', '__return_true' ); } /** diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index b6d16abe5ae..654a60f1d4a 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -4,6 +4,7 @@ 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\Helpers\PrivateAccess; @@ -45,6 +46,7 @@ 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(); @@ -52,6 +54,16 @@ public function test_register() { $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' );