From 6feb425b8763545c5a6e221a5cf142cf058f386d Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Mon, 9 Aug 2021 17:58:19 +0530 Subject: [PATCH 01/44] Update URL validation cron jobs --- src/AmpWpPlugin.php | 2 + .../CronBasedBackgroundTask.php | 27 ++++- src/Validation/SavePostValidationEvent.php | 21 +++- src/Validation/URLValidationCron.php | 100 ++++++------------ src/Validation/URLValidationQueueCron.php | 95 +++++++++++++++++ 5 files changed, 171 insertions(+), 74 deletions(-) create mode 100644 src/Validation/URLValidationQueueCron.php diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index 776cf25b6e5..bab787a53ca 100644 --- a/src/AmpWpPlugin.php +++ b/src/AmpWpPlugin.php @@ -22,6 +22,7 @@ use AmpProject\AmpWP\Validation\SavePostValidationEvent; use AmpProject\AmpWP\Validation\URLValidationCron; use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; +use AmpProject\AmpWP\Validation\URLValidationQueueCron; use AmpProject\Optimizer; use AmpProject\RemoteGetRequest; @@ -118,6 +119,7 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'support' => SupportCliCommand::class, 'support_rest_controller' => SupportRESTController::class, 'url_validation_cron' => URLValidationCron::class, + 'url_validation_queue_cron' => URLValidationQueueCron::class, 'url_validation_rest_controller' => Validation\URLValidationRESTController::class, 'validated_url_stylesheet_gc' => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class, ]; diff --git a/src/BackgroundTask/CronBasedBackgroundTask.php b/src/BackgroundTask/CronBasedBackgroundTask.php index 925e81ed4a6..b25cbbbf5bd 100644 --- a/src/BackgroundTask/CronBasedBackgroundTask.php +++ b/src/BackgroundTask/CronBasedBackgroundTask.php @@ -19,9 +19,10 @@ */ 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_HOURLY = 'hourly'; + const DEFAULT_INTERVAL_TWICE_DAILY = 'twicedaily'; + const DEFAULT_INTERVAL_DAILY = 'daily'; + const DEFAULT_INTERVAL_EVERY_TEN_MINUTES = 'every_ten_minutes'; /** * BackgroundTaskDeactivator instance. @@ -46,6 +47,7 @@ public function __construct( BackgroundTaskDeactivator $background_task_deactiva */ public function register() { $this->background_task_deactivator->add_event( $this->get_event_name() ); + add_filter( 'cron_schedules', [ $this, 'add_cron_schedules' ] ); // phpcs:ignore WordPress.WP.CronInterval.ChangeDetected } /** @@ -72,4 +74,23 @@ abstract protected function get_event_name(); * @param mixed[] ...$args Args to pass to the process callback. */ abstract public function process( ...$args ); + + /** + * Add new interval for cron schedules. + * + * @param array $cron_schedules List of cron schedules. + * + * @return array List of cron schedules. + */ + public function add_cron_schedules( $cron_schedules ) { + + $cron_schedules = ( ! empty( $cron_schedules ) && is_array( $cron_schedules ) ) ? $cron_schedules : []; + + $cron_schedules[ self::DEFAULT_INTERVAL_EVERY_TEN_MINUTES ] = [ + 'interval' => ( MINUTE_IN_SECONDS * 10 ), + 'display' => esc_html__( 'Every 10 minutes', 'amp' ), + ]; + + return $cron_schedules; + } } diff --git a/src/Validation/SavePostValidationEvent.php b/src/Validation/SavePostValidationEvent.php index 4041ddbc9aa..3aaa070253a 100644 --- a/src/Validation/SavePostValidationEvent.php +++ b/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 ); } /** diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index f023797e1a4..4edc1fbcb49 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -3,7 +3,7 @@ * WP cron process to validate URLs in the background. * * @package AMP - * @since 2.1 + * @since 2.1 */ namespace AmpProject\AmpWP\Validation; @@ -19,14 +19,7 @@ * * @internal */ -final class URLValidationCron extends RecurringBackgroundTask implements Conditional { - - /** - * ScannableURLProvider instance. - * - * @var ScannableURLProvider - */ - private $scannable_url_provider; +final class URLValidationCron extends RecurringBackgroundTask { /** * URLValidationProvider instance. @@ -42,51 +35,15 @@ final class URLValidationCron extends RecurringBackgroundTask implements Conditi */ const BACKGROUND_TASK_NAME = 'amp_validate_urls'; - /** - * The length of time, in seconds, to sleep between each URL validation. - * - * @var int - */ - 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. * * @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 URLValidationProvider $url_validation_provider URLValidationProvider instance. */ - public function __construct( BackgroundTaskDeactivator $background_task_deactivator, ScannableURLProvider $scannable_url_provider, URLValidationProvider $url_validation_provider ) { + public function __construct( BackgroundTaskDeactivator $background_task_deactivator, URLValidationProvider $url_validation_provider ) { parent::__construct( $background_task_deactivator ); - $this->scannable_url_provider = $scannable_url_provider; $this->url_validation_provider = $url_validation_provider; } @@ -96,15 +53,34 @@ 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(); + $validation_queue_key = 'amp_url_validation_queue'; + $validation_queue = get_option( $validation_queue_key, [] ); - foreach ( $urls as $url ) { - $this->url_validation_provider->get_url_validation( $url['url'], $url['type'] ); - if ( $sleep_time ) { - sleep( $sleep_time ); + if ( empty( $validation_queue ) || ! is_array( $validation_queue ) ) { + return []; + } + + $limit = 5; + $count = 1; + + foreach ( $validation_queue as $hash => $url ) { + if ( empty( $url['url'] ) || empty( $url['type'] ) ) { + continue; + } + + $response = $this->url_validation_provider->get_url_validation( $url['url'], $url['type'] ); + if ( empty( $response ) || is_wp_error( $response ) ) { + continue; + } + + unset( $validation_queue[ $hash ] ); + $count ++; + if ( $limit < $count ) { + break; } } + + update_option( $validation_queue_key, $validation_queue ); } /** @@ -126,22 +102,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_EVERY_TEN_MINUTES; } } diff --git a/src/Validation/URLValidationQueueCron.php b/src/Validation/URLValidationQueueCron.php new file mode 100644 index 00000000000..9eee400373b --- /dev/null +++ b/src/Validation/URLValidationQueueCron.php @@ -0,0 +1,95 @@ +scannable_url_provider = $scannable_url_provider; + } + + /** + * Callback for the cron action. + * + * @param mixed[] ...$args Unused callback arguments. + */ + public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable + $urls = $this->scannable_url_provider->get_urls(); + $validation_queue_key = 'amp_url_validation_queue'; + $validation_queue = get_option( $validation_queue_key, [] ); + + foreach ( $urls as $url ) { + + if ( empty( $url['url'] ) || empty( $url['type'] ) ) { + continue; + } + + $url_hash = md5( trim( $url['url'] ) ); + $validation_queue[ $url_hash ] = $url; + } + + update_option( $validation_queue_key, $validation_queue ); + } + + /** + * Get the event name. + * + * This is the "slug" of the event, not the display name. + * + * Note: the event name should be prefixed to prevent naming collisions. + * + * @return string Name of the event. + */ + protected function get_event_name() { + return self::BACKGROUND_TASK_NAME; + } + + /** + * Get the interval to use for the event. + * + * @return string An existing interval name. + */ + protected function get_interval() { + return self::DEFAULT_INTERVAL_DAILY; + } +} From 2ce519a945571364428a49122bc2c6de1dc23a5b Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Mon, 9 Aug 2021 19:52:07 +0530 Subject: [PATCH 02/44] Add unit test cases --- .../src/Validation/URLValidationCronTest.php | 35 +++--- .../Validation/URLValidationQueueCronTest.php | 100 ++++++++++++++++++ 2 files changed, 120 insertions(+), 15 deletions(-) create mode 100644 tests/php/src/Validation/URLValidationQueueCronTest.php diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 12b716c5e6f..df3a85feae8 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -4,7 +4,6 @@ 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; @@ -14,6 +13,7 @@ use AmpProject\AmpWP\Validation\URLScanningContext; use AmpProject\AmpWP\Validation\URLValidationCron; use AmpProject\AmpWP\Validation\URLValidationProvider; +use AmpProject\AmpWP\Validation\URLValidationQueueCron; /** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationCron */ final class URLValidationCronTest extends TestCase { @@ -33,7 +33,7 @@ final class URLValidationCronTest extends TestCase { */ public function setUp() { parent::setUp(); - $this->test_instance = new URLValidationCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ), new URLValidationProvider() ); + $this->test_instance = new URLValidationCron( new BackgroundTaskDeactivator(), new URLValidationProvider() ); add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } @@ -46,7 +46,6 @@ 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(); @@ -54,16 +53,6 @@ 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' ); @@ -102,15 +91,31 @@ public function test_schedule_event_with_user_with_permission() { * Test validate_urls. * * @covers ::process() - * @covers ::get_sleep_time() */ public function test_validate_urls() { + $this->factory()->post->create_many( 5 ); add_filter( 'amp_url_validation_sleep_time', '__return_false' ); + $url_validation_queue_instance = new URLValidationQueueCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ) ); + $url_validation_queue_instance->process(); + + $validation_queue_key = 'amp_url_validation_queue'; + $validation_queue = get_option( $validation_queue_key, [] ); + + $this->assertCount( 10, $validation_queue ); + + $this->test_instance->process(); + $validation_queue = get_option( $validation_queue_key, [] ); + + $this->assertCount( 5, $this->get_validated_urls() ); + $this->assertCount( 5, $validation_queue ); + $this->test_instance->process(); + $validation_queue = get_option( $validation_queue_key, [] ); $this->assertCount( 10, $this->get_validated_urls() ); + $this->assertCount( 0, $validation_queue ); } /** @covers ::get_event_name() */ @@ -124,7 +129,7 @@ public function test_get_event_name() { /** @covers ::get_interval() */ public function test_get_interval() { $this->assertEquals( - URLValidationCron::DEFAULT_INTERVAL_DAILY, + URLValidationCron::DEFAULT_INTERVAL_EVERY_TEN_MINUTES, $this->call_private_method( $this->test_instance, 'get_interval' ) ); } diff --git a/tests/php/src/Validation/URLValidationQueueCronTest.php b/tests/php/src/Validation/URLValidationQueueCronTest.php new file mode 100644 index 00000000000..0264e035539 --- /dev/null +++ b/tests/php/src/Validation/URLValidationQueueCronTest.php @@ -0,0 +1,100 @@ +test_instance = new URLValidationQueueCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ) ); + + add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); + } + + /** + * @covers ::register() + * @covers ::get_event_name() + */ + public function test_register() { + + $this->assertInstanceof( CronBasedBackgroundTask::class, $this->test_instance ); + $this->assertInstanceof( URLValidationQueueCron::class, $this->test_instance ); + $this->assertInstanceof( Service::class, $this->test_instance ); + $this->assertInstanceof( Registerable::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( URLValidationQueueCron::BACKGROUND_TASK_NAME, [ $this->test_instance, 'process' ] ) + ); + } + + /** + * @covers ::process + */ + public function test_process() { + + $this->factory()->post->create_many( 5 ); + $this->test_instance->process(); + + $validation_queue_key = 'amp_url_validation_queue'; + $validation_queue = get_option( $validation_queue_key, [] ); + $this->assertCount( 10, $validation_queue ); + } + + /** + * @covers ::get_event_name + */ + public function test_get_event_name() { + + $this->assertEquals( + URLValidationQueueCron::BACKGROUND_TASK_NAME, + $this->call_private_method( $this->test_instance, 'get_event_name' ) + ); + } + + /** + * @covers ::get_interval + */ + public function test_get_interval() { + + $this->assertEquals( + URLValidationQueueCron::DEFAULT_INTERVAL_DAILY, + $this->call_private_method( $this->test_instance, 'get_interval' ) + ); + } +} From 0e8fa7366a4313cb65d420c9f166621dcf7f246c Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Fri, 1 Oct 2021 18:01:56 +0530 Subject: [PATCH 03/44] Validate single URL on URL validation cron, Update test cases --- includes/uninstall-functions.php | 3 ++ .../CronBasedBackgroundTask.php | 28 +++-------------- src/Validation/URLValidationCron.php | 30 +++++-------------- src/Validation/URLValidationQueueCron.php | 26 ++++++++-------- .../src/Validation/URLValidationCronTest.php | 24 +++++++++------ .../Validation/URLValidationQueueCronTest.php | 2 +- 6 files changed, 44 insertions(+), 69 deletions(-) diff --git a/includes/uninstall-functions.php b/includes/uninstall-functions.php index 2b8ff94ca6a..64754a1243e 100644 --- a/includes/uninstall-functions.php +++ b/includes/uninstall-functions.php @@ -7,6 +7,8 @@ namespace AmpProject\AmpWP; +use AmpProject\AmpWP\Validation\URLValidationQueueCron; + /** * Delete data from option table. * @@ -19,6 +21,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( URLValidationQueueCron::OPTION_KEY ); $theme_mod_name = 'amp_customize_setting_modified_timestamps'; remove_theme_mod( $theme_mod_name ); diff --git a/src/BackgroundTask/CronBasedBackgroundTask.php b/src/BackgroundTask/CronBasedBackgroundTask.php index b25cbbbf5bd..8a9ff58a372 100644 --- a/src/BackgroundTask/CronBasedBackgroundTask.php +++ b/src/BackgroundTask/CronBasedBackgroundTask.php @@ -19,10 +19,10 @@ */ 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_EVERY_TEN_MINUTES = 'every_ten_minutes'; + const DEFAULT_INTERVAL_HOURLY = 'hourly'; + const DEFAULT_INTERVAL_TWICE_DAILY = 'twicedaily'; + const DEFAULT_INTERVAL_DAILY = 'daily'; + const DEFAULT_INTERVAL_WEEKLY = 'weekly'; /** * BackgroundTaskDeactivator instance. @@ -47,7 +47,6 @@ public function __construct( BackgroundTaskDeactivator $background_task_deactiva */ public function register() { $this->background_task_deactivator->add_event( $this->get_event_name() ); - add_filter( 'cron_schedules', [ $this, 'add_cron_schedules' ] ); // phpcs:ignore WordPress.WP.CronInterval.ChangeDetected } /** @@ -74,23 +73,4 @@ abstract protected function get_event_name(); * @param mixed[] ...$args Args to pass to the process callback. */ abstract public function process( ...$args ); - - /** - * Add new interval for cron schedules. - * - * @param array $cron_schedules List of cron schedules. - * - * @return array List of cron schedules. - */ - public function add_cron_schedules( $cron_schedules ) { - - $cron_schedules = ( ! empty( $cron_schedules ) && is_array( $cron_schedules ) ) ? $cron_schedules : []; - - $cron_schedules[ self::DEFAULT_INTERVAL_EVERY_TEN_MINUTES ] = [ - 'interval' => ( MINUTE_IN_SECONDS * 10 ), - 'display' => esc_html__( 'Every 10 minutes', 'amp' ), - ]; - - return $cron_schedules; - } } diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index 4edc1fbcb49..c2413a8109e 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -15,7 +15,7 @@ /** * URLValidationCron class. * - * @since 2.1 + * @since 2.2 * * @internal */ @@ -53,34 +53,18 @@ public function __construct( BackgroundTaskDeactivator $background_task_deactiva * @param mixed[] ...$args Unused callback arguments. */ public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - $validation_queue_key = 'amp_url_validation_queue'; - $validation_queue = get_option( $validation_queue_key, [] ); + $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); if ( empty( $validation_queue ) || ! is_array( $validation_queue ) ) { return []; } - $limit = 5; - $count = 1; - - foreach ( $validation_queue as $hash => $url ) { - if ( empty( $url['url'] ) || empty( $url['type'] ) ) { - continue; - } - - $response = $this->url_validation_provider->get_url_validation( $url['url'], $url['type'] ); - if ( empty( $response ) || is_wp_error( $response ) ) { - continue; - } - - unset( $validation_queue[ $hash ] ); - $count ++; - if ( $limit < $count ) { - break; - } + $entry = array_shift( $validation_queue ); + if ( isset( $entry['url'], $entry['type'] ) ) { + $this->url_validation_provider->get_url_validation( $entry['url'], $entry['type'] ); } - update_option( $validation_queue_key, $validation_queue ); + update_option( URLValidationQueueCron::OPTION_KEY, $validation_queue ); } /** @@ -102,6 +86,6 @@ protected function get_event_name() { * @return string An existing interval name. */ protected function get_interval() { - return self::DEFAULT_INTERVAL_EVERY_TEN_MINUTES; + return self::DEFAULT_INTERVAL_HOURLY; } } diff --git a/src/Validation/URLValidationQueueCron.php b/src/Validation/URLValidationQueueCron.php index 9eee400373b..93270e023fb 100644 --- a/src/Validation/URLValidationQueueCron.php +++ b/src/Validation/URLValidationQueueCron.php @@ -15,7 +15,7 @@ /** * URLValidationCron class. * - * @since 2.1 + * @since 2.2 * * @internal */ @@ -35,6 +35,13 @@ final class URLValidationQueueCron extends RecurringBackgroundTask { */ const BACKGROUND_TASK_NAME = 'amp_validate_url_queue'; + /** + * Option key to store queue for URL validation. + * + * @var string + */ + const OPTION_KEY = 'amp_url_validation_queue'; + /** * Class constructor. * @@ -54,21 +61,16 @@ 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(); - $validation_queue_key = 'amp_url_validation_queue'; - $validation_queue = get_option( $validation_queue_key, [] ); + $urls = $this->scannable_url_provider->get_urls(); + $validation_queue = get_option( self::OPTION_KEY, [] ); foreach ( $urls as $url ) { - - if ( empty( $url['url'] ) || empty( $url['type'] ) ) { - continue; + if ( ! in_array( $url, $validation_queue, true ) ) { + $validation_queue[] = $url; } - - $url_hash = md5( trim( $url['url'] ) ); - $validation_queue[ $url_hash ] = $url; } - update_option( $validation_queue_key, $validation_queue ); + update_option( self::OPTION_KEY, $validation_queue ); } /** @@ -90,6 +92,6 @@ protected function get_event_name() { * @return string An existing interval name. */ protected function get_interval() { - return self::DEFAULT_INTERVAL_DAILY; + return self::DEFAULT_INTERVAL_WEEKLY; } } diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index df3a85feae8..1f2fa6e82f5 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -101,21 +101,27 @@ public function test_validate_urls() { $url_validation_queue_instance = new URLValidationQueueCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ) ); $url_validation_queue_instance->process(); - $validation_queue_key = 'amp_url_validation_queue'; - $validation_queue = get_option( $validation_queue_key, [] ); + $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); $this->assertCount( 10, $validation_queue ); $this->test_instance->process(); - $validation_queue = get_option( $validation_queue_key, [] ); + $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); - $this->assertCount( 5, $this->get_validated_urls() ); - $this->assertCount( 5, $validation_queue ); + $this->assertCount( 1, $this->get_validated_urls() ); + $this->assertCount( 9, $validation_queue ); + + + $this->test_instance->process(); + $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); + + $this->assertCount( 2, $this->get_validated_urls() ); + $this->assertCount( 8, $validation_queue ); $this->test_instance->process(); - $validation_queue = get_option( $validation_queue_key, [] ); - $this->assertCount( 10, $this->get_validated_urls() ); - $this->assertCount( 0, $validation_queue ); + $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); + $this->assertCount( 3, $this->get_validated_urls() ); + $this->assertCount( 7, $validation_queue ); } /** @covers ::get_event_name() */ @@ -129,7 +135,7 @@ public function test_get_event_name() { /** @covers ::get_interval() */ public function test_get_interval() { $this->assertEquals( - URLValidationCron::DEFAULT_INTERVAL_EVERY_TEN_MINUTES, + URLValidationCron::DEFAULT_INTERVAL_HOURLY, $this->call_private_method( $this->test_instance, 'get_interval' ) ); } diff --git a/tests/php/src/Validation/URLValidationQueueCronTest.php b/tests/php/src/Validation/URLValidationQueueCronTest.php index 0264e035539..f59e64fd2ad 100644 --- a/tests/php/src/Validation/URLValidationQueueCronTest.php +++ b/tests/php/src/Validation/URLValidationQueueCronTest.php @@ -93,7 +93,7 @@ public function test_get_event_name() { public function test_get_interval() { $this->assertEquals( - URLValidationQueueCron::DEFAULT_INTERVAL_DAILY, + URLValidationQueueCron::DEFAULT_INTERVAL_WEEKLY, $this->call_private_method( $this->test_instance, 'get_interval' ) ); } From a7aeb612e8cb31e2b993217a3a990d1b3df94b19 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 7 Oct 2021 20:17:33 +0530 Subject: [PATCH 04/44] Hard core the site option key that need to delete --- includes/uninstall-functions.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/includes/uninstall-functions.php b/includes/uninstall-functions.php index 64754a1243e..3f9f345cc83 100644 --- a/includes/uninstall-functions.php +++ b/includes/uninstall-functions.php @@ -7,8 +7,6 @@ namespace AmpProject\AmpWP; -use AmpProject\AmpWP\Validation\URLValidationQueueCron; - /** * Delete data from option table. * @@ -21,7 +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( URLValidationQueueCron::OPTION_KEY ); + delete_option( 'amp_url_validation_queue' ); $theme_mod_name = 'amp_customize_setting_modified_timestamps'; remove_theme_mod( $theme_mod_name ); From 9dcb1390f30301836081cc53f9e9e29b538566ed Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 7 Oct 2021 20:33:38 +0530 Subject: [PATCH 05/44] Update comments --- src/Validation/URLValidationCron.php | 2 +- src/Validation/URLValidationQueueCron.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index c2413a8109e..28299b0a871 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -15,7 +15,7 @@ /** * URLValidationCron class. * - * @since 2.2 + * @since 2.1 * * @internal */ diff --git a/src/Validation/URLValidationQueueCron.php b/src/Validation/URLValidationQueueCron.php index 93270e023fb..d7075fed17c 100644 --- a/src/Validation/URLValidationQueueCron.php +++ b/src/Validation/URLValidationQueueCron.php @@ -3,7 +3,7 @@ * WP cron process to validate URLs in the background. * * @package AMP - * @since 2.1 + * @since 2.2 */ namespace AmpProject\AmpWP\Validation; @@ -13,7 +13,7 @@ use AmpProject\AmpWP\Infrastructure\Conditional; /** - * URLValidationCron class. + * URLValidationQueueCron class. * * @since 2.2 * From dc72b8a4fb39b787cdf382615305ef3170e65226 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 7 Oct 2021 20:43:05 +0530 Subject: [PATCH 06/44] Remove unused references --- src/Validation/URLValidationCron.php | 1 - src/Validation/URLValidationQueueCron.php | 1 - 2 files changed, 2 deletions(-) diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index 28299b0a871..6527119f0d5 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -10,7 +10,6 @@ use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\RecurringBackgroundTask; -use AmpProject\AmpWP\Infrastructure\Conditional; /** * URLValidationCron class. diff --git a/src/Validation/URLValidationQueueCron.php b/src/Validation/URLValidationQueueCron.php index d7075fed17c..4e887263472 100644 --- a/src/Validation/URLValidationQueueCron.php +++ b/src/Validation/URLValidationQueueCron.php @@ -10,7 +10,6 @@ use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\RecurringBackgroundTask; -use AmpProject\AmpWP\Infrastructure\Conditional; /** * URLValidationQueueCron class. From 4f1630e5e8fdfd41a1b415ecc8f585e81abca4c4 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 11 Oct 2021 17:25:14 -0700 Subject: [PATCH 07/44] Remove return of empty array --- src/Validation/URLValidationCron.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index 6527119f0d5..67ffa344187 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -55,7 +55,7 @@ public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnaly $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); if ( empty( $validation_queue ) || ! is_array( $validation_queue ) ) { - return []; + return; } $entry = array_shift( $validation_queue ); From c0853884089c9cc2f9c9b16df896d42c7a7da3b9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 11 Oct 2021 18:06:22 -0700 Subject: [PATCH 08/44] Replace extraneous URLValidationProvider with direct AMP_Validation_Manager call --- src/Validation/URLValidationCron.php | 30 ++++++----------------- src/Validation/URLValidationQueueCron.php | 12 +++++---- 2 files changed, 14 insertions(+), 28 deletions(-) diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index 67ffa344187..e9e1a00372e 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -8,44 +8,28 @@ namespace AmpProject\AmpWP\Validation; -use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\RecurringBackgroundTask; +use AMP_Validation_Manager; /** * URLValidationCron class. * * @since 2.1 * + * @todo This should be renamed something to make it distinct from URLValidationQueueCron. * @internal */ final class URLValidationCron extends RecurringBackgroundTask { - /** - * URLValidationProvider instance. - * - * @var URLValidationProvider - */ - private $url_validation_provider; - /** * The cron action name. * + * @todo Rename this to amp_validate_dequeued_url or something. + * * @var string */ const BACKGROUND_TASK_NAME = 'amp_validate_urls'; - /** - * Class constructor. - * - * @param BackgroundTaskDeactivator $background_task_deactivator Service that deactivates background events. - * @param URLValidationProvider $url_validation_provider URLValidationProvider instance. - */ - public function __construct( BackgroundTaskDeactivator $background_task_deactivator, URLValidationProvider $url_validation_provider ) { - parent::__construct( $background_task_deactivator ); - - $this->url_validation_provider = $url_validation_provider; - } - /** * Callback for the cron action. * @@ -58,9 +42,9 @@ public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnaly return; } - $entry = array_shift( $validation_queue ); - if ( isset( $entry['url'], $entry['type'] ) ) { - $this->url_validation_provider->get_url_validation( $entry['url'], $entry['type'] ); + $url = array_shift( $validation_queue ); + if ( is_string( $url ) ) { + AMP_Validation_Manager::validate_url_and_store( $url ); } update_option( URLValidationQueueCron::OPTION_KEY, $validation_queue ); diff --git a/src/Validation/URLValidationQueueCron.php b/src/Validation/URLValidationQueueCron.php index 4e887263472..e40a113e1e7 100644 --- a/src/Validation/URLValidationQueueCron.php +++ b/src/Validation/URLValidationQueueCron.php @@ -48,7 +48,6 @@ final class URLValidationQueueCron extends RecurringBackgroundTask { * @param ScannableURLProvider $scannable_url_provider ScannableURLProvider instance. */ public function __construct( BackgroundTaskDeactivator $background_task_deactivator, ScannableURLProvider $scannable_url_provider ) { - parent::__construct( $background_task_deactivator ); $this->scannable_url_provider = $scannable_url_provider; @@ -60,12 +59,15 @@ 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(); $validation_queue = get_option( self::OPTION_KEY, [] ); + if ( ! is_array( $validation_queue ) ) { + $validation_queue = []; + } - foreach ( $urls as $url ) { - if ( ! in_array( $url, $validation_queue, true ) ) { - $validation_queue[] = $url; + $entries = $this->scannable_url_provider->get_urls(); + foreach ( $entries as $entry ) { + if ( ! in_array( $entry['url'], $validation_queue, true ) ) { + $validation_queue[] = $entry['url']; } } From 2a05e8f8ff83e7de9f8a42893cb2960344c1773c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 11 Oct 2021 18:12:17 -0700 Subject: [PATCH 09/44] Update tests to use DependencyInjectedTestCase --- .../src/Validation/URLValidationCronTest.php | 7 +++---- .../Validation/URLValidationQueueCronTest.php | 20 +++++++++---------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 1f2fa6e82f5..df7e1620d58 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -6,17 +6,16 @@ use AmpProject\AmpWP\BackgroundTask\CronBasedBackgroundTask; 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; use AmpProject\AmpWP\Validation\URLValidationQueueCron; /** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationCron */ -final class URLValidationCronTest extends TestCase { +final class URLValidationCronTest extends DependencyInjectedTestCase { use ValidationRequestMocking, PrivateAccess; /** @@ -33,7 +32,7 @@ final class URLValidationCronTest extends TestCase { */ public function setUp() { parent::setUp(); - $this->test_instance = new URLValidationCron( new BackgroundTaskDeactivator(), new URLValidationProvider() ); + $this->test_instance = $this->injector->make( URLValidationCron::class ); add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } diff --git a/tests/php/src/Validation/URLValidationQueueCronTest.php b/tests/php/src/Validation/URLValidationQueueCronTest.php index f59e64fd2ad..28566cc0356 100644 --- a/tests/php/src/Validation/URLValidationQueueCronTest.php +++ b/tests/php/src/Validation/URLValidationQueueCronTest.php @@ -1,25 +1,20 @@ test_instance = new URLValidationQueueCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ) ); + $this->test_instance = $this->injector->make( URLValidationQueueCron::class ); add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } @@ -71,9 +66,12 @@ public function test_process() { $this->factory()->post->create_many( 5 ); $this->test_instance->process(); - $validation_queue_key = 'amp_url_validation_queue'; - $validation_queue = get_option( $validation_queue_key, [] ); - $this->assertCount( 10, $validation_queue ); + $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); + $this->assertIsArray( $validation_queue ); + $this->assertCount( 6, $validation_queue ); + foreach ( $validation_queue as $url ) { + $this->assertStringStartsWith( home_url( '/' ), $url ); + } } /** From f41681fdc48a2c38658ecdd8bd51112387ecd828 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 12 Oct 2021 15:01:10 -0700 Subject: [PATCH 10/44] Simplify URL validation scheduling by putting the queue in URLValidationCron --- includes/uninstall-functions.php | 2 +- src/AmpWpPlugin.php | 2 - src/Validation/URLValidationCron.php | 74 ++++++++++++-- src/Validation/URLValidationQueueCron.php | 98 ------------------- .../src/Validation/URLValidationCronTest.php | 75 ++++++++------ .../Validation/URLValidationQueueCronTest.php | 98 ------------------- 6 files changed, 113 insertions(+), 236 deletions(-) delete mode 100644 src/Validation/URLValidationQueueCron.php delete mode 100644 tests/php/src/Validation/URLValidationQueueCronTest.php diff --git a/includes/uninstall-functions.php b/includes/uninstall-functions.php index d7ed6bf45fa..7bf7bba15bd 100644 --- a/includes/uninstall-functions.php +++ b/includes/uninstall-functions.php @@ -19,7 +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' ); + 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 ); diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index bab787a53ca..776cf25b6e5 100644 --- a/src/AmpWpPlugin.php +++ b/src/AmpWpPlugin.php @@ -22,7 +22,6 @@ use AmpProject\AmpWP\Validation\SavePostValidationEvent; use AmpProject\AmpWP\Validation\URLValidationCron; use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; -use AmpProject\AmpWP\Validation\URLValidationQueueCron; use AmpProject\Optimizer; use AmpProject\RemoteGetRequest; @@ -119,7 +118,6 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'support' => SupportCliCommand::class, 'support_rest_controller' => SupportRESTController::class, 'url_validation_cron' => URLValidationCron::class, - 'url_validation_queue_cron' => URLValidationQueueCron::class, 'url_validation_rest_controller' => Validation\URLValidationRESTController::class, 'validated_url_stylesheet_gc' => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class, ]; diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index e9e1a00372e..016afe85b65 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -8,6 +8,7 @@ namespace AmpProject\AmpWP\Validation; +use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\RecurringBackgroundTask; use AMP_Validation_Manager; @@ -16,7 +17,6 @@ * * @since 2.1 * - * @todo This should be renamed something to make it distinct from URLValidationQueueCron. * @internal */ final class URLValidationCron extends RecurringBackgroundTask { @@ -24,30 +24,86 @@ final class URLValidationCron extends RecurringBackgroundTask { /** * The cron action name. * - * @todo Rename this to amp_validate_dequeued_url or something. + * Note that only one queued URL is currently validated at a time. * * @var string */ const BACKGROUND_TASK_NAME = 'amp_validate_urls'; + /** + * Option key to store queue for URL validation. + * + * @var string + */ + const OPTION_KEY = 'amp_url_validation_queue'; + + /** + * ScannableURLProvider instance. + * + * @var ScannableURLProvider + */ + private $scannable_url_provider; + + /** + * Constructor. + * + * @param BackgroundTaskDeactivator $background_task_deactivator Service that deactivates background events. + * @param ScannableURLProvider $scannable_url_provider ScannableURLProvider instance. + */ + public function __construct( BackgroundTaskDeactivator $background_task_deactivator, ScannableURLProvider $scannable_url_provider ) { + parent::__construct( $background_task_deactivator ); + $this->scannable_url_provider = $scannable_url_provider; + } + /** * Callback for the cron action. * * @param mixed[] ...$args Unused callback arguments. */ public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); + $url = $this->dequeue(); + if ( $url ) { + AMP_Validation_Manager::validate_url_and_store( $url ); + } + } - if ( empty( $validation_queue ) || ! is_array( $validation_queue ) ) { - return; + /** + * 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 + ); - $url = array_shift( $validation_queue ); - if ( is_string( $url ) ) { - AMP_Validation_Manager::validate_url_and_store( $url ); + // 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(); } - update_option( URLValidationQueueCron::OPTION_KEY, $validation_queue ); + // 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; } /** diff --git a/src/Validation/URLValidationQueueCron.php b/src/Validation/URLValidationQueueCron.php deleted file mode 100644 index e40a113e1e7..00000000000 --- a/src/Validation/URLValidationQueueCron.php +++ /dev/null @@ -1,98 +0,0 @@ -scannable_url_provider = $scannable_url_provider; - } - - /** - * Callback for the cron action. - * - * @param mixed[] ...$args Unused callback arguments. - */ - public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - $validation_queue = get_option( self::OPTION_KEY, [] ); - if ( ! is_array( $validation_queue ) ) { - $validation_queue = []; - } - - $entries = $this->scannable_url_provider->get_urls(); - foreach ( $entries as $entry ) { - if ( ! in_array( $entry['url'], $validation_queue, true ) ) { - $validation_queue[] = $entry['url']; - } - } - - update_option( self::OPTION_KEY, $validation_queue ); - } - - /** - * Get the event name. - * - * This is the "slug" of the event, not the display name. - * - * Note: the event name should be prefixed to prevent naming collisions. - * - * @return string Name of the event. - */ - protected function get_event_name() { - return self::BACKGROUND_TASK_NAME; - } - - /** - * Get the interval to use for the event. - * - * @return string An existing interval name. - */ - protected function get_interval() { - return self::DEFAULT_INTERVAL_WEEKLY; - } -} diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index df7e1620d58..fe1987fc91a 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -12,7 +12,6 @@ use AmpProject\AmpWP\Validation\ScannableURLProvider; use AmpProject\AmpWP\Validation\URLScanningContext; use AmpProject\AmpWP\Validation\URLValidationCron; -use AmpProject\AmpWP\Validation\URLValidationQueueCron; /** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationCron */ final class URLValidationCronTest extends DependencyInjectedTestCase { @@ -25,6 +24,9 @@ final class URLValidationCronTest extends DependencyInjectedTestCase { */ private $test_instance; + /** @var int */ + private $request_count = 0; + /** * Setup. * @@ -33,7 +35,13 @@ final class URLValidationCronTest extends DependencyInjectedTestCase { public function setUp() { parent::setUp(); $this->test_instance = $this->injector->make( URLValidationCron::class ); - add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); + add_filter( + 'pre_http_request', + function () { + $this->request_count++; + return $this->get_validate_response(); + } + ); } /** @@ -90,37 +98,48 @@ public function test_schedule_event_with_user_with_permission() { * Test validate_urls. * * @covers ::process() + * @covers ::dequeue() */ public function test_validate_urls() { - - $this->factory()->post->create_many( 5 ); - - add_filter( 'amp_url_validation_sleep_time', '__return_false' ); - - $url_validation_queue_instance = new URLValidationQueueCron( new BackgroundTaskDeactivator(), new ScannableURLProvider( new URLScanningContext( 20 ) ) ); - $url_validation_queue_instance->process(); - - $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); - - $this->assertCount( 10, $validation_queue ); - - $this->test_instance->process(); - $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); - - $this->assertCount( 1, $this->get_validated_urls() ); - $this->assertCount( 9, $validation_queue ); - - + /** @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(); - $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); - - $this->assertCount( 2, $this->get_validated_urls() ); - $this->assertCount( 8, $validation_queue ); + $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(); - $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); - $this->assertCount( 3, $this->get_validated_urls() ); - $this->assertCount( 7, $validation_queue ); + $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() */ diff --git a/tests/php/src/Validation/URLValidationQueueCronTest.php b/tests/php/src/Validation/URLValidationQueueCronTest.php deleted file mode 100644 index 28566cc0356..00000000000 --- a/tests/php/src/Validation/URLValidationQueueCronTest.php +++ /dev/null @@ -1,98 +0,0 @@ -test_instance = $this->injector->make( URLValidationQueueCron::class ); - - add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); - } - - /** - * @covers ::register() - * @covers ::get_event_name() - */ - public function test_register() { - - $this->assertInstanceof( CronBasedBackgroundTask::class, $this->test_instance ); - $this->assertInstanceof( URLValidationQueueCron::class, $this->test_instance ); - $this->assertInstanceof( Service::class, $this->test_instance ); - $this->assertInstanceof( Registerable::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( URLValidationQueueCron::BACKGROUND_TASK_NAME, [ $this->test_instance, 'process' ] ) - ); - } - - /** - * @covers ::process - */ - public function test_process() { - - $this->factory()->post->create_many( 5 ); - $this->test_instance->process(); - - $validation_queue = get_option( URLValidationQueueCron::OPTION_KEY, [] ); - $this->assertIsArray( $validation_queue ); - $this->assertCount( 6, $validation_queue ); - foreach ( $validation_queue as $url ) { - $this->assertStringStartsWith( home_url( '/' ), $url ); - } - } - - /** - * @covers ::get_event_name - */ - public function test_get_event_name() { - - $this->assertEquals( - URLValidationQueueCron::BACKGROUND_TASK_NAME, - $this->call_private_method( $this->test_instance, 'get_event_name' ) - ); - } - - /** - * @covers ::get_interval - */ - public function test_get_interval() { - - $this->assertEquals( - URLValidationQueueCron::DEFAULT_INTERVAL_WEEKLY, - $this->call_private_method( $this->test_instance, 'get_interval' ) - ); - } -} From 422d0f2cc224d848eb014a4a390d242f0594d2fc Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 12 Oct 2021 20:47:06 -0700 Subject: [PATCH 11/44] Re-schedule with new recurrence when schedule changes --- src/BackgroundTask/RecurringBackgroundTask.php | 12 +++++++----- tests/php/src/Validation/URLValidationCronTest.php | 2 -- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/BackgroundTask/RecurringBackgroundTask.php b/src/BackgroundTask/RecurringBackgroundTask.php index 20dd98ae0cd..84b0901965e 100644 --- a/src/BackgroundTask/RecurringBackgroundTask.php +++ b/src/BackgroundTask/RecurringBackgroundTask.php @@ -37,13 +37,15 @@ 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 ); - wp_schedule_event( time(), $this->get_interval(), $event_name, $args ); + if ( $scheduled_event && $scheduled_event->schedule !== $recurrence ) { + wp_schedule_event( $scheduled_event->timestamp, $recurrence, $event_name, $args ); + } elseif ( ! $scheduled_event ) { + wp_schedule_event( time(), $recurrence, $event_name, $args ); + } } /** diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index fe1987fc91a..b950c455c78 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -2,7 +2,6 @@ namespace AmpProject\AmpWP\Tests\Validation; -use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; use AmpProject\AmpWP\BackgroundTask\CronBasedBackgroundTask; use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; @@ -10,7 +9,6 @@ use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; use AmpProject\AmpWP\Validation\ScannableURLProvider; -use AmpProject\AmpWP\Validation\URLScanningContext; use AmpProject\AmpWP\Validation\URLValidationCron; /** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationCron */ From bf25b0cb4999c2c4e9d70a1923082cec2dab0aaa Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 12 Oct 2021 21:30:45 -0700 Subject: [PATCH 12/44] Unschedule event with wrong recurrence instead of rescheduling it --- src/BackgroundTask/RecurringBackgroundTask.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/BackgroundTask/RecurringBackgroundTask.php b/src/BackgroundTask/RecurringBackgroundTask.php index 84b0901965e..a8c50ebc75a 100644 --- a/src/BackgroundTask/RecurringBackgroundTask.php +++ b/src/BackgroundTask/RecurringBackgroundTask.php @@ -41,9 +41,13 @@ final public function schedule_event( ...$args ) { $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_schedule_event( $scheduled_event->timestamp, $recurrence, $event_name, $args ); - } elseif ( ! $scheduled_event ) { + wp_unschedule_event( $scheduled_event->timestamp, $event_name, $args ); + $scheduled_event = null; + } + + if ( ! $scheduled_event ) { wp_schedule_event( time(), $recurrence, $event_name, $args ); } } From e7f51ea22c2ce39f6d61fccd374e1bb336093655 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 13 Oct 2021 19:32:05 +0530 Subject: [PATCH 13/44] Fix unit test case --- src/BackgroundTask/RecurringBackgroundTask.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/BackgroundTask/RecurringBackgroundTask.php b/src/BackgroundTask/RecurringBackgroundTask.php index a8c50ebc75a..73a41e4e6f2 100644 --- a/src/BackgroundTask/RecurringBackgroundTask.php +++ b/src/BackgroundTask/RecurringBackgroundTask.php @@ -7,6 +7,8 @@ namespace AmpProject\AmpWP\BackgroundTask; +use function wp_get_scheduled_event; + /** * Abstract base class for using cron to execute a background task that runs on a schedule. * From 5f56cc140c01957b80ebfa00dae08e716bd5b28d Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 13 Oct 2021 19:40:00 +0530 Subject: [PATCH 14/44] Revert "Fix unit test case" This reverts commit e7f51ea22c2ce39f6d61fccd374e1bb336093655. --- src/BackgroundTask/RecurringBackgroundTask.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/BackgroundTask/RecurringBackgroundTask.php b/src/BackgroundTask/RecurringBackgroundTask.php index 73a41e4e6f2..a8c50ebc75a 100644 --- a/src/BackgroundTask/RecurringBackgroundTask.php +++ b/src/BackgroundTask/RecurringBackgroundTask.php @@ -7,8 +7,6 @@ namespace AmpProject\AmpWP\BackgroundTask; -use function wp_get_scheduled_event; - /** * Abstract base class for using cron to execute a background task that runs on a schedule. * From 3f803b0ba56e3ac8ae69f0b4ee30dd04fc9f08e0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 13 Oct 2021 18:08:08 -0700 Subject: [PATCH 15/44] Ensure that AMP-enabled posts are scanned --- includes/admin/functions.php | 3 +++ src/Validation/ScannableURLProvider.php | 30 +++++++++++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/includes/admin/functions.php b/includes/admin/functions.php index 512358df4f9..e2d53bfac64 100644 --- a/includes/admin/functions.php +++ b/includes/admin/functions.php @@ -32,6 +32,9 @@ function amp_init_customizer() { /** * Get permalink for the first AMP-eligible post. * + * @todo Eliminate this in favor of ScannableURLProvider::get_posts_by_type(). + * @see \AmpProject\AmpWP\Validation\ScannableURLProvider::get_posts_by_type() + * * @internal * @return string|null URL on success, null if none found. */ diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 1e3232b8c5f..a5eeb6d71a4 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -68,11 +68,18 @@ public function get_urls( $offset = 0 ) { for ( $i = $offset; $i < $limit_per_type + $offset; $i++ ) { // Include all public, published posts. foreach ( $public_post_types as $post_type ) { - $post_ids = $this->get_posts_that_support_amp( $this->get_posts_by_type( $post_type, $i, 1 ) ); - if ( ! empty( $post_ids[0] ) ) { + // Note that we get 100 posts because it may be that some of them have AMP disabled. It is more + // efficient to do it this way than to try to do a meta query that looks for posts that have the + // amp_status meta equal to 'enabled' or else for posts that lack the meta key altogether. In the latter + // case, the absence of the meta may not mean AMP is enabled since the default-enabled state can be + // overridden with the `amp_post_status_default_enabled` filter. So in this case, we grab 100 post IDs + // and then just use the first one. + $post_ids = $this->get_posts_that_support_amp( $this->get_posts_by_type( $post_type, $i, 100 ) ); + $post_id = reset( $post_ids ); + if ( $post_id ) { $post_type_object = get_post_type_object( $post_type ); $urls[] = [ - 'url' => get_permalink( $post_ids[0] ), + 'url' => get_permalink( $post_id ), 'type' => $post_type, 'label' => $post_type_object->labels->singular_name ?: $post_type, ]; @@ -82,7 +89,7 @@ public function get_urls( $offset = 0 ) { foreach ( $amp_enabled_taxonomies as $taxonomy ) { $taxonomy_links = $this->get_taxonomy_links( $taxonomy, $i, 1 ); $link = reset( $taxonomy_links ); - if ( ! empty( $link ) ) { + if ( $link ) { $taxonomy_object = get_taxonomy( $taxonomy ); $urls[] = [ 'url' => $link, @@ -93,9 +100,10 @@ public function get_urls( $offset = 0 ) { } $author_page_urls = $this->get_author_page_urls( $i, 1 ); - if ( ! empty( $author_page_urls[0] ) ) { + $author_page_url = reset( $author_page_urls ); + if ( $author_page_url ) { $urls[] = [ - 'url' => $author_page_urls[0], + 'url' => $author_page_url, 'type' => 'author', 'label' => __( 'Author Archive', 'amp' ), ]; @@ -165,15 +173,19 @@ private function get_posts_that_support_amp( $ids ) { return $ids; } - return array_filter( - $ids, - 'amp_is_post_supported' + return array_values( + array_filter( + $ids, + 'amp_is_post_supported' + ) ); } /** * Gets the IDs of public, published posts. * + * @see \amp_admin_get_preview_permalink() + * * @param string $post_type The post type. * @param int|null $offset The offset of the query (optional). * @param int|null $number The number of posts to query for (optional). From fc2c727bdb687fcf471a7853fce59b4e88ec2c52 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 13 Oct 2021 18:22:33 -0700 Subject: [PATCH 16/44] Remove obsolete force argument for amp validation CLI command --- src/Cli/ValidationCommand.php | 37 +++++++------------ src/Validation/ScannableURLProvider.php | 7 ---- src/Validation/URLScanningContext.php | 21 +---------- .../Validation/ScannableURLProviderTest.php | 32 ++++++---------- .../src/Validation/URLScanningContextTest.php | 2 - 5 files changed, 26 insertions(+), 73 deletions(-) diff --git a/src/Cli/ValidationCommand.php b/src/Cli/ValidationCommand.php index 0bc77433598..ea744b34a1b 100644 --- a/src/Cli/ValidationCommand.php +++ b/src/Cli/ValidationCommand.php @@ -43,6 +43,7 @@ final class ValidationCommand implements Service, CliCommand { * For example, by unchecking 'Categories' in 'AMP Settings' > 'Supported Templates'. * But with this flag, validation will ignore these options. * + * @since 2.2 This is no longer used. * @var string */ const FLAG_NAME_FORCE_VALIDATION = 'force'; @@ -121,7 +122,7 @@ public static function get_command_name() { * : Only validates a URL if one of the conditionals is true. * * [--force] - * : Force validation of URLs even if their associated templates or object types do not have AMP enabled. + * : (Obsolete) Force validation of URLs even if their associated templates or object types do not have AMP enabled. * * ## EXAMPLES * @@ -138,6 +139,10 @@ public function run( /** @noinspection PhpUnusedParameterInspection */ $args, $a $url_validation_provider = $this->get_validation_provider(); $urls = $scannable_url_provider->get_urls(); + if ( Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ) ) { + WP_CLI::warning( sprintf( 'The --%s argument is obsolete.', self::FLAG_NAME_FORCE_VALIDATION ) ); + } + $number_urls_to_crawl = count( $urls ); if ( ! $number_urls_to_crawl ) { if ( ! empty( Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ) ) ) { @@ -148,12 +153,7 @@ public function run( /** @noinspection PhpUnusedParameterInspection */ $args, $a ) ); } else { - WP_CLI::error( - sprintf( - 'All of your templates might be unchecked in AMP Settings > Supported Templates. You might pass --%s to this command.', - self::FLAG_NAME_FORCE_VALIDATION - ) - ); + WP_CLI::error( 'All of your templates might be unchecked in AMP Settings > Supported Templates.' ); } } @@ -222,23 +222,13 @@ private function get_validation_url_provider() { return $this->scannable_url_provider; } - $include_conditionals = Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); - $force_crawl_urls = Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); - $limit_type_validate_count = Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); - - /* - * Handle the argument and flag passed to the command: --include and --force. - * If the self::INCLUDE_ARGUMENT is present, force crawling of URLs. - * The WP-CLI command should indicate which templates are crawled, not the /wp-admin options. - */ - if ( ! empty( $include_conditionals ) ) { - if ( is_string( $include_conditionals ) ) { - $include_conditionals = explode( ',', $include_conditionals ); - } - - $force_crawl_urls = true; + $include_conditionals = Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); + if ( is_string( $include_conditionals ) ) { + $include_conditionals = explode( ',', $include_conditionals ); } + $limit_type_validate_count = Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); + // Handle special case for Legacy Reader mode. if ( AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) @@ -271,8 +261,7 @@ private function get_validation_url_provider() { $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( $limit_type_validate_count, - $include_conditionals, - $force_crawl_urls + $include_conditionals ) ); diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index a5eeb6d71a4..168b5d3da67 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -144,9 +144,6 @@ private function is_template_supported( $template ) { if ( ! empty( $include_conditionals ) ) { return in_array( $template, $include_conditionals, true ); } - if ( $this->context->get_include_unsupported() ) { - return true; - } $supportable_templates = AMP_Theme_Support::get_supportable_templates(); @@ -169,10 +166,6 @@ private function get_posts_that_support_amp( $ids ) { return []; } - if ( $this->context->get_include_unsupported() ) { - return $ids; - } - return array_values( array_filter( $ids, diff --git a/src/Validation/URLScanningContext.php b/src/Validation/URLScanningContext.php index 9b6c203bc65..11362a3ffee 100644 --- a/src/Validation/URLScanningContext.php +++ b/src/Validation/URLScanningContext.php @@ -24,13 +24,6 @@ final class URLScanningContext { */ const DEFAULT_LIMIT_PER_TYPE = 1; - /** - * Whether to include URLs that don't support AMP. - * - * @var bool - */ - private $include_unsupported; - /** * An allowlist of conditionals to use for querying URLs. * @@ -54,16 +47,13 @@ final class URLScanningContext { * * @param int $limit_per_type The maximum number of URLs to validate for each type. * @param array $include_conditionals An allowlist of conditionals to use for validation. - * @param bool $include_unsupported Whether to include URLs that don't support AMP. */ public function __construct( $limit_per_type = self::DEFAULT_LIMIT_PER_TYPE, - $include_conditionals = [], - $include_unsupported = false + $include_conditionals = [] ) { $this->limit_per_type = $limit_per_type; $this->include_conditionals = $include_conditionals; - $this->include_unsupported = $include_unsupported; } /** @@ -91,13 +81,4 @@ public function get_limit_per_type() { public function get_include_conditionals() { return $this->include_conditionals; } - - /** - * Provides the include_unsupported setting. - * - * @return bool - */ - public function get_include_unsupported() { - return $this->include_unsupported; - } } diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index eaea484d6c0..f83fe812edc 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -31,7 +31,7 @@ final class ScannableURLProviderTest extends TestCase { */ public function setUp() { parent::setUp(); - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [] ) ); add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } @@ -125,10 +125,7 @@ public function test_get_posts_that_support_amp() { AMP_Post_Meta_Box::ENABLED_STATUS ); - // When the second $force_count_all_urls argument is true, all of the newly-created posts should be part of the URL count. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [], true ) ); - $this->assertEquals( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [] ) ); // In AMP-first, the IDs should include all of the newly-created posts. AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); @@ -147,14 +144,14 @@ public function test_get_posts_that_support_amp() { * If the WP-CLI command has an include argument, and is_singular isn't in it, no posts will have AMP enabled. * For example, wp amp validate-site --include=is_tag,is_category */ - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_tag', 'is_category' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_tag', 'is_category' ] ) ); $this->assertEquals( [], $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); /* * If is_singular is in the WP-CLI argument, it should return these posts as being AMP-enabled. * For example, wp amp validate-site include=is_singular,is_category */ - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_singular', 'is_category' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_singular', 'is_category' ] ) ); $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ) ); } @@ -182,11 +179,11 @@ public function test_get_author_page_urls() { $this->assertEquals( [ $second_author_url ], $actual_urls ); // If $include_conditionals is set and does not have is_author, this should not return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_category' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_category' ] ) ); $this->assertEquals( [], $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls' ) ); // If $include_conditionals is set and has is_author, this should return URLs. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_author' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_author' ] ) ); $this->assertEquals( [ $first_author_url, $second_author_url ], $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls' ) @@ -216,12 +213,7 @@ public function test_does_taxonomy_support_amp() { $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); } - // When $include_unsupported is true, all taxonomies should be supported. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [], true ) ); - foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); - } - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [] ) ); // When the user has checked the Option::ALL_TEMPLATES_SUPPORTED box, this should always be true. AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, true ); @@ -234,7 +226,7 @@ public function test_does_taxonomy_support_amp() { * If the user passed allowed conditionals to the WP-CLI command like wp amp validate-site --include=is_category,is_tag * these should be supported taxonomies. */ - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_category', 'is_tag' ], true ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_category', 'is_tag' ] ) ); $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'category' ] ) ); $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'tag' ] ) ); $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'author' ] ) ); @@ -362,11 +354,11 @@ public function test_get_search_page() { $this->assertTrue( is_string( $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ) ); // If $include_conditionals is set and does not have is_search, this should not return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_author' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_author' ] ) ); $this->assertEquals( null, $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ); // If $include_conditionals has is_search, this should return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_search' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_search' ] ) ); $this->assertTrue( is_string( $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ) ); } @@ -382,11 +374,11 @@ public function test_get_date_page() { $this->assertStringContainsString( $year, $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); // If $include_conditionals is set and does not have is_date, this should not return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_search' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_search' ] ) ); $this->assertEquals( null, $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); // If $include_conditionals has is_date, this should return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_date' ], false ) ); + $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_date' ] ) ); $parsed_page_url = wp_parse_url( $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); $this->assertStringContainsString( $year, $parsed_page_url['query'] ); } diff --git a/tests/php/src/Validation/URLScanningContextTest.php b/tests/php/src/Validation/URLScanningContextTest.php index f467f405ba5..ef6f990df93 100644 --- a/tests/php/src/Validation/URLScanningContextTest.php +++ b/tests/php/src/Validation/URLScanningContextTest.php @@ -66,7 +66,6 @@ static function() { /** * @covers ::get_limit_per_type() * @covers ::get_include_conditionals() - * @covers ::get_include_unsupported() */ public function test_getters() { $this->test_instance = new URLScanningContext( 99, [ 'is_date', 'is_search' ], true ); @@ -76,6 +75,5 @@ public function test_getters() { [ 'is_date', 'is_search' ], $this->test_instance->get_include_conditionals() ); - $this->assertTrue( $this->test_instance->get_include_unsupported() ); } } From 72ffb2d72161b9947e48ebc79a03019bfb82ab4e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 12:13:12 -0700 Subject: [PATCH 17/44] Provide wp_get_scheduled_event() implementation for WP<5.1 and add tests --- .../RecurringBackgroundTask.php | 69 ++++++++++++++++++- .../src/Validation/URLValidationCronTest.php | 45 +++++++++++- 2 files changed, 110 insertions(+), 4 deletions(-) diff --git a/src/BackgroundTask/RecurringBackgroundTask.php b/src/BackgroundTask/RecurringBackgroundTask.php index a8c50ebc75a..7291be8e451 100644 --- a/src/BackgroundTask/RecurringBackgroundTask.php +++ b/src/BackgroundTask/RecurringBackgroundTask.php @@ -39,7 +39,7 @@ final public function schedule_event( ...$args ) { $event_name = $this->get_event_name(); $recurrence = $this->get_interval(); - $scheduled_event = wp_get_scheduled_event( $event_name, $args ); + $scheduled_event = $this->get_scheduled_event( $event_name, $args ); // Unschedule any existing event which had a differing recurrence. if ( $scheduled_event && $scheduled_event->schedule !== $recurrence ) { @@ -52,6 +52,73 @@ final public function schedule_event( ...$args ) { } } + /** + * Retrieve a scheduled event. + * + * This uses a copied implementation from WordPress core if `wp_get_scheduled_event()` does not exist, as it was + * introduced in WordPress 5.1. + * + * @link https://github.com/WordPress/wordpress-develop/blob/ba943e113d3b31b121f7/src/wp-includes/cron.php#L753-L793 + * @see \wp_get_scheduled_event() + * @codeCoverageIgnore + * + * @param string $hook Action hook of the event. + * @param array $args Optional. Array containing each separate argument to pass to the hook's callback function. + * Although not passed to a callback, these arguments are used to uniquely identify the + * event, so they should be the same as those used when originally scheduling the event. + * Default empty array. + * @param int|null $timestamp Optional. Unix timestamp (UTC) of the event. If not specified, the next scheduled event + * is returned. Default null. + * @return object|false The event object. False if the event does not exist. + */ + final public function get_scheduled_event( $hook, $args = [], $timestamp = null ) { + if ( function_exists( 'wp_get_scheduled_event' ) ) { + return wp_get_scheduled_event( $hook, $args, $timestamp ); + } + + if ( null !== $timestamp && ! is_numeric( $timestamp ) ) { + return false; + } + + $crons = _get_cron_array(); + if ( empty( $crons ) ) { + return false; + } + + $key = md5( serialize( $args ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize -- This is copied from WP core. + + if ( ! $timestamp ) { + // Get next event. + $next = false; + foreach ( $crons as $timestamp => $cron ) { + if ( isset( $cron[ $hook ][ $key ] ) ) { + $next = $timestamp; + break; + } + } + if ( ! $next ) { + return false; + } + + $timestamp = $next; + } elseif ( ! isset( $crons[ $timestamp ][ $hook ][ $key ] ) ) { + return false; + } + + $event = (object) [ + 'hook' => $hook, + 'timestamp' => $timestamp, + 'schedule' => $crons[ $timestamp ][ $hook ][ $key ]['schedule'], + 'args' => $args, + ]; + + if ( isset( $crons[ $timestamp ][ $hook ][ $key ]['interval'] ) ) { + $event->interval = $crons[ $timestamp ][ $hook ][ $key ]['interval']; + } + + return $event; + } + /** * Get the interval to use for the event. * diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index b950c455c78..31fbfc0e18e 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -58,7 +58,7 @@ public function test_register() { $this->assertEquals( 10, has_action( URLValidationCron::BACKGROUND_TASK_NAME, [ $this->test_instance, 'process' ] ) ); } - /** @covers ::schedule_event() */ + /** @covers RecurringBackgroundTask::schedule_event() */ public function test_schedule_event_with_no_user() { $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); @@ -68,7 +68,7 @@ public function test_schedule_event_with_no_user() { $this->assertFalse( wp_next_scheduled( $event_name ) ); } - /** @covers ::schedule_event() */ + /** @covers RecurringBackgroundTask::schedule_event() */ public function test_schedule_event_with_user_without_permission() { $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); @@ -81,7 +81,7 @@ public function test_schedule_event_with_user_without_permission() { $this->assertTrue( is_numeric( wp_next_scheduled( $event_name ) ) ); } - /** @covers ::schedule_event() */ + /** @covers RecurringBackgroundTask::schedule_event() */ public function test_schedule_event_with_user_with_permission() { $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); @@ -92,6 +92,45 @@ public function test_schedule_event_with_user_with_permission() { $this->assertNotFalse( wp_next_scheduled( $event_name ) ); } + /** @covers RecurringBackgroundTask::schedule_event() */ + public function test_schedule_event_with_different_recurrence() { + wp_set_current_user( $this->factory()->user->create( [ 'role' => 'administrator' ] ) ); + $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); + $args = []; + $old_interval = 'weekly'; + $interval = $this->call_private_method( $this->test_instance, 'get_interval' ); + + $this->assertFalse( wp_next_scheduled( $event_name, $args ) ); + + $count_events = static function ( $hook ) { + $count = 0; + foreach ( _get_cron_array() as $cron ) { + if ( isset( $cron[ $hook ] ) ) { + $count += count( $cron[ $hook ] ); + } + } + return $count; + }; + $this->assertEquals( 0, $count_events( $event_name ) ); + + // First schedule with an old interval. + $this->assertNotEquals( $old_interval, $interval ); + $old_time = time() - HOUR_IN_SECONDS; + wp_schedule_event( $old_time, $old_interval, $event_name, $args ); + $this->assertEquals( $old_time, wp_next_scheduled( $event_name, $args ) ); + $this->assertEquals( 1, $count_events( $event_name ) ); + + // Now try scheduling with the new interval. + $this->test_instance->schedule_event(); + $event = $this->test_instance->get_scheduled_event( $event_name, $args ); + $this->assertIsObject( $event ); + + $this->assertEquals( 1, $count_events( $event_name ) ); + $this->assertNotEquals( $old_time, $event->timestamp, 'Expected old event to no longer be scheduled at the old time.' ); + $this->assertGreaterThanOrEqual( time(), $event->timestamp ); + $this->assertEquals( $interval, $event->schedule, 'Expected event to have the new interval.' ); + } + /** * Test validate_urls. * From b5afb54ace69ada293946771388bafd4b087de90 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 14:03:16 -0700 Subject: [PATCH 18/44] Try not using past time for scheduling event --- tests/php/src/Validation/URLValidationCronTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 31fbfc0e18e..42942a70751 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -115,7 +115,7 @@ public function test_schedule_event_with_different_recurrence() { // First schedule with an old interval. $this->assertNotEquals( $old_interval, $interval ); - $old_time = time() - HOUR_IN_SECONDS; + $old_time = time() + HOUR_IN_SECONDS; wp_schedule_event( $old_time, $old_interval, $event_name, $args ); $this->assertEquals( $old_time, wp_next_scheduled( $event_name, $args ) ); $this->assertEquals( 1, $count_events( $event_name ) ); From 7371fd4e11bcb59b47aecbbff81945c81620f197 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 14:50:30 -0700 Subject: [PATCH 19/44] Use daily schedule to test since available in older WP versions --- tests/php/src/Validation/URLValidationCronTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 42942a70751..12143b38203 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -97,7 +97,7 @@ public function test_schedule_event_with_different_recurrence() { wp_set_current_user( $this->factory()->user->create( [ 'role' => 'administrator' ] ) ); $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); $args = []; - $old_interval = 'weekly'; + $old_interval = 'daily'; $interval = $this->call_private_method( $this->test_instance, 'get_interval' ); $this->assertFalse( wp_next_scheduled( $event_name, $args ) ); @@ -116,7 +116,7 @@ public function test_schedule_event_with_different_recurrence() { // First schedule with an old interval. $this->assertNotEquals( $old_interval, $interval ); $old_time = time() + HOUR_IN_SECONDS; - wp_schedule_event( $old_time, $old_interval, $event_name, $args ); + $this->assertTrue( wp_schedule_event( $old_time, $old_interval, $event_name, $args ) ); $this->assertEquals( $old_time, wp_next_scheduled( $event_name, $args ) ); $this->assertEquals( 1, $count_events( $event_name ) ); From afcbcce1b39bac1c5267237a0e33f5dea92643ca Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 15:05:55 -0700 Subject: [PATCH 20/44] Fix covers phpdoc tags --- tests/php/src/Validation/URLValidationCronTest.php | 8 ++++---- tests/php/test-amp-script-sanitizer.php | 6 +++--- .../php/validation/test-class-amp-validation-manager.php | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 12143b38203..b4147e430f4 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -58,7 +58,7 @@ public function test_register() { $this->assertEquals( 10, has_action( URLValidationCron::BACKGROUND_TASK_NAME, [ $this->test_instance, 'process' ] ) ); } - /** @covers RecurringBackgroundTask::schedule_event() */ + /** @covers ::schedule_event() */ public function test_schedule_event_with_no_user() { $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); @@ -68,7 +68,7 @@ public function test_schedule_event_with_no_user() { $this->assertFalse( wp_next_scheduled( $event_name ) ); } - /** @covers RecurringBackgroundTask::schedule_event() */ + /** @covers ::schedule_event() */ public function test_schedule_event_with_user_without_permission() { $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); @@ -81,7 +81,7 @@ public function test_schedule_event_with_user_without_permission() { $this->assertTrue( is_numeric( wp_next_scheduled( $event_name ) ) ); } - /** @covers RecurringBackgroundTask::schedule_event() */ + /** @covers ::schedule_event() */ public function test_schedule_event_with_user_with_permission() { $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); @@ -92,7 +92,7 @@ public function test_schedule_event_with_user_with_permission() { $this->assertNotFalse( wp_next_scheduled( $event_name ) ); } - /** @covers RecurringBackgroundTask::schedule_event() */ + /** @covers ::schedule_event() */ public function test_schedule_event_with_different_recurrence() { wp_set_current_user( $this->factory()->user->create( [ 'role' => 'administrator' ] ) ); $event_name = $this->call_private_method( $this->test_instance, 'get_event_name' ); diff --git a/tests/php/test-amp-script-sanitizer.php b/tests/php/test-amp-script-sanitizer.php index 0e72637cf54..539e2149916 100644 --- a/tests/php/test-amp-script-sanitizer.php +++ b/tests/php/test-amp-script-sanitizer.php @@ -19,7 +19,7 @@ /** * Test AMP_Script_Sanitizer. * - * @covers AMP_Script_Sanitizer + * @coversDefaultClass AMP_Script_Sanitizer */ class AMP_Script_Sanitizer_Test extends TestCase { @@ -320,7 +320,7 @@ public function get_sanitizer_data() { * @param string $source Source. * @param string $expected Expected. * @param array $sanitizer_args Sanitizer args. - * @covers AMP_Script_Sanitizer::sanitize() + * @covers ::sanitize() */ public function test_sanitize( $source, $expected = null, $sanitizer_args = [], $expected_error_codes = [] ) { if ( null === $expected ) { @@ -365,7 +365,7 @@ public function get_data_to_test_cascading_sanitizer_argument_changes_with_custo /** * @dataProvider get_data_to_test_cascading_sanitizer_argument_changes_with_custom_scripts * - * @covers AMP_Script_Sanitizer::init() + * @covers ::init() * * @param int $level Level. */ diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 6fa87739915..9c0704fd05f 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1459,7 +1459,7 @@ public function test_callback_wrappers() { } /** - * @covers ::wrap_block_callbacks() + * @covers AMP_Validation_Callback_Wrapper::wrap_block_callbacks() * @covers AMP_Validation_Callback_Wrapper::get_callback_function() */ public function test_wrap_block_callbacks() { From ca7b70d8d4c93f1f1157c99bc72e0fddedb27919 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 15:08:50 -0700 Subject: [PATCH 21/44] Use assertLessThanOrEqual rather than assertGreaterThanOrEqual --- tests/php/src/Validation/URLValidationCronTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index b4147e430f4..d0a82afc5bd 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -127,7 +127,7 @@ public function test_schedule_event_with_different_recurrence() { $this->assertEquals( 1, $count_events( $event_name ) ); $this->assertNotEquals( $old_time, $event->timestamp, 'Expected old event to no longer be scheduled at the old time.' ); - $this->assertGreaterThanOrEqual( time(), $event->timestamp ); + $this->assertLessThanOrEqual( time(), $event->timestamp ); $this->assertEquals( $interval, $event->schedule, 'Expected event to have the new interval.' ); } From d1ccdaf4364e46117b8bb4d1ebec56c04c9cdf3a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 15:14:30 -0700 Subject: [PATCH 22/44] Fix covers phpdoc tag for wrap_block_callbacks --- tests/php/validation/test-class-amp-validation-manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php index 9c0704fd05f..52c4aa27b12 100644 --- a/tests/php/validation/test-class-amp-validation-manager.php +++ b/tests/php/validation/test-class-amp-validation-manager.php @@ -1459,7 +1459,7 @@ public function test_callback_wrappers() { } /** - * @covers AMP_Validation_Callback_Wrapper::wrap_block_callbacks() + * @covers AMP_Validation_Manager::wrap_block_callbacks() * @covers AMP_Validation_Callback_Wrapper::get_callback_function() */ public function test_wrap_block_callbacks() { From 2e2f3637ca214c9fbf977660a37ce42aeb63b095 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 15:16:09 -0700 Subject: [PATCH 23/44] Account for wp_schedule_event() returning bool as of WP 5.1 --- tests/php/src/Validation/URLValidationCronTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index d0a82afc5bd..3ed3c468874 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -116,7 +116,10 @@ public function test_schedule_event_with_different_recurrence() { // First schedule with an old interval. $this->assertNotEquals( $old_interval, $interval ); $old_time = time() + HOUR_IN_SECONDS; - $this->assertTrue( wp_schedule_event( $old_time, $old_interval, $event_name, $args ) ); + $success = wp_schedule_event( $old_time, $old_interval, $event_name, $args ); + if ( version_compare( get_bloginfo( 'version' ), '5.1', '>=' ) ) { + $this->assertTrue( $success ); + } $this->assertEquals( $old_time, wp_next_scheduled( $event_name, $args ) ); $this->assertEquals( 1, $count_events( $event_name ) ); From 41448300ca42cd9b4b4c2b39cb3273d488b0e43c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 15:27:44 -0700 Subject: [PATCH 24/44] Remove obsolete arg from URLScanningContext constructor --- tests/php/src/Validation/URLScanningContextTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/src/Validation/URLScanningContextTest.php b/tests/php/src/Validation/URLScanningContextTest.php index ef6f990df93..7bf463f8e1f 100644 --- a/tests/php/src/Validation/URLScanningContextTest.php +++ b/tests/php/src/Validation/URLScanningContextTest.php @@ -68,7 +68,7 @@ static function() { * @covers ::get_include_conditionals() */ public function test_getters() { - $this->test_instance = new URLScanningContext( 99, [ 'is_date', 'is_search' ], true ); + $this->test_instance = new URLScanningContext( 99, [ 'is_date', 'is_search' ] ); $this->assertEquals( 99, $this->test_instance->get_limit_per_type() ); $this->assertEquals( From 79d678fa56d4c4fab77fa3483849480880c3e728 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 14 Oct 2021 15:33:27 -0700 Subject: [PATCH 25/44] Remove unused amp_url_validation_limit_per_type filter --- src/Validation/URLScanningContext.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/Validation/URLScanningContext.php b/src/Validation/URLScanningContext.php index 11362a3ffee..eed3e6c215d 100644 --- a/src/Validation/URLScanningContext.php +++ b/src/Validation/URLScanningContext.php @@ -62,15 +62,7 @@ public function __construct( * @return int */ public function get_limit_per_type() { - /** - * Filters the number of URLs per content type to check during each run of the validation cron task. - * - * @since 2.1.0 - * @param int $url_validation_number_per_type The number of URLs. Defaults to 1. Filtering to -1 will result in all being returned. - */ - $url_validation_limit_per_type = (int) apply_filters( 'amp_url_validation_limit_per_type', $this->limit_per_type ); - - return max( $url_validation_limit_per_type, -1 ); + return $this->limit_per_type; } /** From da77a0ddedd2a26c5540fc7927e0f08adb9b0f12 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 11:38:49 -0700 Subject: [PATCH 26/44] Allow options to be overridden when calling get_supportable_templates --- includes/class-amp-theme-support.php | 12 +++++++++--- tests/php/test-class-amp-theme-support.php | 17 ++++++++++++++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 7e37d84a9c2..36351bdaa0d 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -686,9 +686,15 @@ public static function get_template_availability( $query = null ) { /** * Get the templates which can be supported. * + * @param array $options Optional AMP options to override what has been saved. * @return array Supportable templates. */ - public static function get_supportable_templates() { + public static function get_supportable_templates( $options = [] ) { + $options = array_merge( + AMP_Options_Manager::get_options(), + $options + ); + $templates = [ 'is_singular' => [ 'label' => __( 'Singular', 'amp' ), @@ -798,8 +804,8 @@ public static function get_supportable_templates() { */ $templates = apply_filters( 'amp_supportable_templates', $templates ); - $supported_templates = AMP_Options_Manager::get_option( Option::SUPPORTED_TEMPLATES ); - $are_all_supported = AMP_Options_Manager::get_option( Option::ALL_TEMPLATES_SUPPORTED ); + $supported_templates = $options[ Option::SUPPORTED_TEMPLATES ]; + $are_all_supported = $options[ Option::ALL_TEMPLATES_SUPPORTED ]; $did_filter_supply_supported = false; $did_filter_supply_immutable = false; diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index 844d09606ae..31b54764439 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -702,6 +702,22 @@ public function test_get_supportable_templates() { $this->assertArrayHasKey( 'is_home', $supportable_templates ); $this->assertArrayNotHasKey( 'parent', $supportable_templates['is_home'] ); + // Test that overrides work. + $this->assertTrue( $supportable_templates['is_singular']['supported'] ); + $this->assertTrue( $supportable_templates['is_home']['supported'] ); + $this->assertTrue( $supportable_templates['is_date']['supported'] ); + $this->assertTrue( $supportable_templates['is_404']['supported'] ); + $overridden_supportable_templates = AMP_Theme_Support::get_supportable_templates( + [ + Option::ALL_TEMPLATES_SUPPORTED => false, + Option::SUPPORTED_TEMPLATES => [ 'is_date', 'is_404' ], + ] + ); + $this->assertFalse( $overridden_supportable_templates['is_singular']['supported'] ); + $this->assertFalse( $overridden_supportable_templates['is_home']['supported'] ); + $this->assertTrue( $overridden_supportable_templates['is_date']['supported'] ); + $this->assertTrue( $overridden_supportable_templates['is_404']['supported'] ); + // Test common templates. $this->assertArrayHasKey( 'is_singular', $supportable_templates ); $this->assertArrayHasKey( 'is_archive', $supportable_templates ); @@ -747,7 +763,6 @@ static function( $templates ) { ); $supportable_templates = AMP_Theme_Support::get_supportable_templates(); $this->assertArrayHasKey( 'is_custom', $supportable_templates ); - remove_all_filters( 'amp_supportable_templates' ); } /** From 5bbadeaa6d5311090e3b61e0a5fc2f167bf0d760 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 12:03:16 -0700 Subject: [PATCH 27/44] Refactor ScannableURLProvider and URLValidationProvider into services * Eliminate extraneous URLScanningContext. * Use template conditionals as type labels in WP-CLI output. * Prepare ScannableURLProvider to return URLs that don't correspond to the currently-set template mode for Site Scan in Onboarding Wizard. --- .phpstorm.meta.php | 2 + src/AmpWpPlugin.php | 4 + src/Cli/ValidationCommand.php | 124 +++------- src/Validation/ScannableURLProvider.php | 229 +++++++++++++----- src/Validation/URLScanningContext.php | 76 ------ src/Validation/URLValidationProvider.php | 3 +- .../Validation/ScannableURLProviderTest.php | 53 ++-- .../src/Validation/URLScanningContextTest.php | 79 ------ 8 files changed, 228 insertions(+), 342 deletions(-) delete mode 100644 src/Validation/URLScanningContext.php delete mode 100644 tests/php/src/Validation/URLScanningContextTest.php diff --git a/.phpstorm.meta.php b/.phpstorm.meta.php index 84b228bdfdf..f9d561ad97f 100644 --- a/.phpstorm.meta.php +++ b/.phpstorm.meta.php @@ -60,6 +60,8 @@ 'url_validation_cron' => \AmpProject\AmpWP\Validation\URLValidationCron::class, 'url_validation_rest_controller' => \AmpProject\AmpWP\Validation\URLValidationRESTController::class, 'validated_url_stylesheet_gc' => \AmpProject\AmpWP\BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class, + 'validation.scannable_url_provider' => \AmpProject\AmpWP\Validation\ScannableURLProvider::class, + 'validation.url_validation_provider' => \AmpProject\AmpWP\Validation\URLValidationProvider::class, ] ) ); diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index 1ab1af1c5bf..4ca2da42317 100644 --- a/src/AmpWpPlugin.php +++ b/src/AmpWpPlugin.php @@ -20,8 +20,10 @@ use AmpProject\AmpWP\Support\SupportCliCommand; use AmpProject\AmpWP\Support\SupportRESTController; use AmpProject\AmpWP\Validation\SavePostValidationEvent; +use AmpProject\AmpWP\Validation\ScannableURLProvider; use AmpProject\AmpWP\Validation\URLValidationCron; use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator; +use AmpProject\AmpWP\Validation\URLValidationProvider; use AmpProject\Optimizer; use AmpProject\RemoteGetRequest; @@ -121,6 +123,8 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'url_validation_cron' => URLValidationCron::class, 'url_validation_rest_controller' => Validation\URLValidationRESTController::class, 'validated_url_stylesheet_gc' => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class, + 'validation.scannable_url_provider' => ScannableURLProvider::class, + 'validation.url_validation_provider' => URLValidationProvider::class, ]; /** diff --git a/src/Cli/ValidationCommand.php b/src/Cli/ValidationCommand.php index ea744b34a1b..d58c23149eb 100644 --- a/src/Cli/ValidationCommand.php +++ b/src/Cli/ValidationCommand.php @@ -9,18 +9,13 @@ namespace AmpProject\AmpWP\Cli; -use AMP_Options_Manager; -use AMP_Theme_Support; use AMP_Validated_URL_Post_Type; use AMP_Validation_Error_Taxonomy; use AMP_Validation_Manager; -use AmpProject\AmpWP\Admin\ReaderThemes; use AmpProject\AmpWP\Infrastructure\CliCommand; use AmpProject\AmpWP\Infrastructure\Service; -use AmpProject\AmpWP\Option; -use AmpProject\AmpWP\Validation\URLValidationProvider; use AmpProject\AmpWP\Validation\ScannableURLProvider; -use AmpProject\AmpWP\Validation\URLScanningContext; +use AmpProject\AmpWP\Validation\URLValidationProvider; use Exception; use WP_CLI; use WP_CLI\Utils; @@ -107,6 +102,17 @@ public static function get_command_name() { return 'amp validation'; } + /** + * Construct. + * + * @param URLValidationProvider $url_validation_provider URL validation provider. + * @param ScannableURLProvider $scannable_url_provider Scannable URL provider. + */ + public function __construct( URLValidationProvider $url_validation_provider, ScannableURLProvider $scannable_url_provider ) { + $this->url_validation_provider = $url_validation_provider; + $this->scannable_url_provider = $scannable_url_provider; + } + /** * Crawl the entire site to get AMP validation results. * @@ -135,14 +141,21 @@ public static function get_command_name() { public function run( /** @noinspection PhpUnusedParameterInspection */ $args, $assoc_args ) { $this->assoc_args = $assoc_args; - $scannable_url_provider = $this->get_validation_url_provider(); - $url_validation_provider = $this->get_validation_provider(); - $urls = $scannable_url_provider->get_urls(); - if ( Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ) ) { WP_CLI::warning( sprintf( 'The --%s argument is obsolete.', self::FLAG_NAME_FORCE_VALIDATION ) ); } + $include_conditionals = Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); + if ( is_string( $include_conditionals ) ) { + $include_conditionals = explode( ',', $include_conditionals ); + } + $this->scannable_url_provider->set_include_conditionals( $include_conditionals ); + + $limit_per_type = Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); + $this->scannable_url_provider->set_limit_per_type( $limit_per_type ); + + $urls = $this->scannable_url_provider->get_urls(); + $number_urls_to_crawl = count( $urls ); if ( ! $number_urls_to_crawl ) { if ( ! empty( Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ) ) ) { @@ -168,12 +181,12 @@ public function run( /** @noinspection PhpUnusedParameterInspection */ $args, $a $this->wp_cli_progress->finish(); - $key_template_type = 'Template or content type'; + $key_template_type = 'Template'; $key_url_count = 'URL Count'; $key_validity_rate = 'Validity Rate'; $table_validation_by_type = []; - foreach ( $url_validation_provider->get_validity_by_type() as $type_name => $validity ) { + foreach ( $this->url_validation_provider->get_validity_by_type() as $type_name => $validity ) { $table_validation_by_type[] = [ $key_template_type => $type_name, $key_url_count => $validity['total'], @@ -190,9 +203,9 @@ public function run( /** @noinspection PhpUnusedParameterInspection */ $args, $a WP_CLI::success( sprintf( '%3$d crawled URLs have invalid markup kept out of %2$d total with AMP validation issue(s); %1$d URLs were crawled.', - $url_validation_provider->get_number_validated(), - $url_validation_provider->get_total_errors(), - $url_validation_provider->get_unaccepted_errors() + $this->url_validation_provider->get_number_validated(), + $this->url_validation_provider->get_total_errors(), + $this->url_validation_provider->get_unaccepted_errors() ) ); @@ -211,93 +224,18 @@ public function run( /** @noinspection PhpUnusedParameterInspection */ $args, $a WP_CLI::line( sprintf( 'For more details, please see: %s', $url_more_details ) ); } - /** - * Provides the ScannableURLProvider instance. - * - * @return ScannableURLProvider - * @throws WP_CLI\ExitException If templates are disallowed by current config. - */ - private function get_validation_url_provider() { - if ( ! is_null( $this->scannable_url_provider ) ) { - return $this->scannable_url_provider; - } - - $include_conditionals = Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); - if ( is_string( $include_conditionals ) ) { - $include_conditionals = explode( ',', $include_conditionals ); - } - - $limit_type_validate_count = Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); - - // Handle special case for Legacy Reader mode. - if ( - AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) - && - ReaderThemes::DEFAULT_READER_THEME === AMP_Options_Manager::get_option( Option::READER_THEME ) - ) { - $allowed_templates = [ - 'is_singular', - ]; - if ( 'page' === get_option( 'show_on_front' ) ) { - $allowed_templates[] = 'is_home'; - $allowed_templates[] = 'is_front_page'; - } - - $disallowed_templates = array_diff( $include_conditionals, $allowed_templates ); - if ( ! empty( $disallowed_templates ) ) { - WP_CLI::error( - sprintf( - 'Templates not supported in legacy Reader mode with current configuration: %s', - implode( ',', $disallowed_templates ) - ) - ); - } - - if ( empty( $include_conditionals ) ) { - $include_conditionals = $allowed_templates; - } - } - - $this->scannable_url_provider = new ScannableURLProvider( - new URLScanningContext( - $limit_type_validate_count, - $include_conditionals - ) - ); - - return $this->scannable_url_provider; - } - - /** - * Provides the site scan instance. - * - * @return URLValidationProvider - */ - private function get_validation_provider() { - if ( ! is_null( $this->url_validation_provider ) ) { - return $this->url_validation_provider; - } - - $this->url_validation_provider = new URLValidationProvider(); - - return $this->url_validation_provider; - } - /** * Validates the URLs. * * @param array $urls URLs to validate, or null to get URLs from the scannable URL provider. */ - private function validate_urls( $urls = null ) { - $url_validation_provider = $this->get_validation_provider(); - + private function validate_urls( $urls = [] ) { if ( ! $urls ) { - $scannable_url_provider = $this->get_validation_url_provider(); - $urls = $scannable_url_provider->get_urls(); + $urls = $this->scannable_url_provider->get_urls(); } foreach ( $urls as $url ) { - $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'] ); + $validity = $this->url_validation_provider->get_url_validation( $url['url'], $url['type'] ); if ( $this->wp_cli_progress ) { $this->wp_cli_progress->tick(); diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 168b5d3da67..91aacb76dfd 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -8,8 +8,12 @@ namespace AmpProject\AmpWP\Validation; +use AmpProject\AmpWP\Infrastructure\Service; use AMP_Theme_Support; use WP_Query; +use AMP_Options_Manager; +use AmpProject\AmpWP\Option; +use AmpProject\AmpWP\Admin\ReaderThemes; /** * ScannableURLProvider class. @@ -17,33 +21,130 @@ * @since 2.1 * @internal */ -final class ScannableURLProvider { +final class ScannableURLProvider implements Service { /** - * Instance of URLScanningContext. + * AMP Settings (options). * - * @var URLScanningContext + * @see ScannableURLProvider::get_options() + * @var null|array */ - private $context; + private $options = null; /** - * Class constructor. + * Overrides to AMP Settings. * - * @param URLScanningContext $context Instance of URLScanningContext. + * @var array */ - public function __construct( URLScanningContext $context ) { - $this->context = $context; + private $option_overrides = []; + + /** + * Template conditionals to restrict results to. + * + * @var string[] + */ + private $include_conditionals = []; + + /** + * Limit for the number of URLs to obtain for each template type. + * + * @var int + */ + private $limit_per_type; + + /** + * Supportable templates. + * + * @see ScannableURLProvider::get_supportable_templates() + * @var null|array + */ + private $supportable_templates = null; + + /** + * @param array $option_overrides Overrides to AMP settings. + * @param string[] $include_conditionals Template conditionals to restrict results to. + * @param int $limit_per_type Limit of URLs to obtain per type. + */ + public function __construct( $option_overrides = [], $include_conditionals = [], $limit_per_type = 1 ) { + $this->option_overrides = $option_overrides; + $this->include_conditionals = $include_conditionals; + $this->limit_per_type = $limit_per_type; + } + + /** + * Get options with overrides merged on top. + * + * @return array + */ + private function get_options() { + if ( null === $this->options ) { + $this->options = array_merge( + AMP_Options_Manager::get_options(), + $this->option_overrides + ); + } + return $this->options; } /** - * Provides the array of URLs to check. + * Get supportable templates. + * + * If the current options are for legacy Reader mode, then the templates not supported by it are disabled. * - * Each URL is an array with two elements, with the URL at index 0 and the type at index 1. + * @see AMP_Theme_Support::get_supportable_templates() + * + * @return array + */ + private function get_supportable_templates() { + if ( null === $this->supportable_templates ) { + $options = $this->get_options(); + + $this->supportable_templates = AMP_Theme_Support::get_supportable_templates( $options ); + + if ( + AMP_Theme_Support::READER_MODE_SLUG === $options[ Option::THEME_SUPPORT ] + && + ReaderThemes::DEFAULT_READER_THEME === $options[ Option::READER_THEME ] + ) { + $allowed_templates = [ + 'is_singular', + ]; + if ( 'page' === get_option( 'show_on_front' ) ) { + $allowed_templates[] = 'is_home'; + $allowed_templates[] = 'is_front_page'; + } + foreach ( array_diff( array_keys( $this->supportable_templates ), $allowed_templates ) as $template ) { + $this->supportable_templates[ $template ]['supported'] = false; + } + } + } + return $this->supportable_templates; + } + + /** + * Set include conditionals. + * + * @param string[] $include_conditionals Include conditionals. + */ + public function set_include_conditionals( $include_conditionals ) { + $this->include_conditionals = $include_conditionals; + } + + /** + * Set limit per type. + * + * @param int $limit_per_type Limit per type. + */ + public function set_limit_per_type( $limit_per_type ) { + $this->limit_per_type = $limit_per_type; + } + + /** + * Get the array of URLs to check. * - * @param int|null $offset Optional. The number of URLs to offset by, where applicable. Defaults to 0. * @return array Array of URLs and types. */ - public function get_urls( $offset = 0 ) { + public function get_urls() { $urls = []; /* @@ -52,37 +153,33 @@ public function get_urls( $offset = 0 ) { if ( 'posts' === get_option( 'show_on_front' ) && $this->is_template_supported( 'is_home' ) ) { $urls[] = [ 'url' => home_url( '/' ), - 'type' => 'home', + 'type' => 'is_home', 'label' => __( 'Homepage', 'amp' ), ]; } $amp_enabled_taxonomies = array_filter( get_taxonomies( [ 'public' => true ] ), - [ $this, 'does_taxonomy_support_amp' ] + function ( $taxonomy ) { + return $this->does_taxonomy_support_amp( $taxonomy ); + } ); $public_post_types = get_post_types( [ 'public' => true ] ); - $limit_per_type = $this->context->get_limit_per_type(); // Include one URL of each template/content type, then another URL of each type on the next iteration. - for ( $i = $offset; $i < $limit_per_type + $offset; $i++ ) { - // Include all public, published posts. - foreach ( $public_post_types as $post_type ) { - // Note that we get 100 posts because it may be that some of them have AMP disabled. It is more - // efficient to do it this way than to try to do a meta query that looks for posts that have the - // amp_status meta equal to 'enabled' or else for posts that lack the meta key altogether. In the latter - // case, the absence of the meta may not mean AMP is enabled since the default-enabled state can be - // overridden with the `amp_post_status_default_enabled` filter. So in this case, we grab 100 post IDs - // and then just use the first one. - $post_ids = $this->get_posts_that_support_amp( $this->get_posts_by_type( $post_type, $i, 100 ) ); - $post_id = reset( $post_ids ); - if ( $post_id ) { - $post_type_object = get_post_type_object( $post_type ); - $urls[] = [ - 'url' => get_permalink( $post_id ), - 'type' => $post_type, - 'label' => $post_type_object->labels->singular_name ?: $post_type, - ]; + for ( $i = 0; $i < $this->limit_per_type; $i++ ) { + if ( $this->is_template_supported( 'is_singular' ) ) { + foreach ( $public_post_types as $post_type ) { + $post_ids = $this->get_posts_by_type( $post_type, $i ); + $post_id = reset( $post_ids ); + if ( $post_id ) { + $post_type_object = get_post_type_object( $post_type ); + $urls[] = [ + 'url' => get_permalink( $post_id ), + 'type' => sprintf( 'is_singular[%s]', $post_type ), + 'label' => $post_type_object->labels->singular_name ?: $post_type, + ]; + } } } @@ -93,7 +190,7 @@ public function get_urls( $offset = 0 ) { $taxonomy_object = get_taxonomy( $taxonomy ); $urls[] = [ 'url' => $link, - 'type' => $taxonomy, + 'type' => sprintf( 'is_tax[%s]', $taxonomy ), 'label' => $taxonomy_object->labels->singular_name ?: $taxonomy, ]; } @@ -104,7 +201,7 @@ public function get_urls( $offset = 0 ) { if ( $author_page_url ) { $urls[] = [ 'url' => $author_page_url, - 'type' => 'author', + 'type' => 'is_author', 'label' => __( 'Author Archive', 'amp' ), ]; } @@ -115,7 +212,7 @@ public function get_urls( $offset = 0 ) { if ( $url ) { $urls[] = [ 'url' => $url, - 'type' => 'date', + 'type' => 'is_date', 'label' => __( 'Date Archive', 'amp' ), ]; } @@ -123,7 +220,7 @@ public function get_urls( $offset = 0 ) { if ( $url ) { $urls[] = [ 'url' => $url, - 'type' => 'search', + 'type' => 'is_search', 'label' => __( 'Search Results', 'amp' ), ]; } @@ -138,14 +235,17 @@ public function get_urls( $offset = 0 ) { * @return bool Whether the template is supported. */ private function is_template_supported( $template ) { - $include_conditionals = $this->context->get_include_conditionals(); // If we received an allowlist of conditionals, this template conditional must be present in it. - if ( ! empty( $include_conditionals ) ) { - return in_array( $template, $include_conditionals, true ); + if ( + count( $this->include_conditionals ) > 0 + && + ! in_array( $template, $this->include_conditionals, true ) + ) { + return false; } - $supportable_templates = AMP_Theme_Support::get_supportable_templates(); + $supportable_templates = $this->get_supportable_templates(); // Check whether this taxonomy's template is supported, including in the 'AMP Settings' > 'Supported Templates' UI. return ! empty( $supportable_templates[ $template ]['supported'] ); @@ -162,32 +262,35 @@ private function is_template_supported( $template ) { * @return array The post IDs that support AMP, or an empty array. */ private function get_posts_that_support_amp( $ids ) { - if ( ! $this->is_template_supported( 'is_singular' ) ) { - return []; - } - return array_values( array_filter( $ids, - 'amp_is_post_supported' + static function ( $id ) { + return amp_is_post_supported( $id ); + } ) ); } /** - * Gets the IDs of public, published posts. + * Gets the IDs of published posts that support AMP. * * @see \amp_admin_get_preview_permalink() * * @param string $post_type The post type. * @param int|null $offset The offset of the query (optional). - * @param int|null $number The number of posts to query for (optional). * @return int[] $post_ids The post IDs in an array. */ - private function get_posts_by_type( $post_type, $offset = null, $number = null ) { + private function get_posts_by_type( $post_type, $offset = null ) { + // Note that we get 100 posts because it may be that some of them have AMP disabled. It is more + // efficient to do it this way than to try to do a meta query that looks for posts that have the + // amp_status meta equal to 'enabled' or else for posts that lack the meta key altogether. In the latter + // case, the absence of the meta may not mean AMP is enabled since the default-enabled state can be + // overridden with the `amp_post_status_default_enabled` filter. So in this case, we grab 100 post IDs + // and then just use the first one. $args = [ 'post_type' => $post_type, - 'posts_per_page' => is_int( $number ) ? $number : $this->context->get_limit_per_type(), + 'posts_per_page' => 100, 'post_status' => 'publish', 'orderby' => 'ID', 'order' => 'DESC', @@ -203,7 +306,7 @@ private function get_posts_by_type( $post_type, $offset = null, $number = null ) } $query = new WP_Query( $args ); - return $query->posts; + return $this->get_posts_that_support_amp( $query->posts ); } /** @@ -212,17 +315,16 @@ private function get_posts_by_type( $post_type, $offset = null, $number = null ) * Accepts an $offset parameter, for the query of authors. * 0 is the first author in the query, and 1 is the second. * - * @param int|string $offset The offset for the URL to query for, should be an int if passing an argument. - * @param int|string $number The total number to query for, should be an int if passing an argument. + * @param int $offset The offset for the URL to query for, should be an int if passing an argument. + * @param int $number The total number to query for, should be an int if passing an argument. * @return string[] The author page URLs, or an empty array. */ - private function get_author_page_urls( $offset = '', $number = '' ) { + private function get_author_page_urls( $offset, $number ) { $author_page_urls = []; if ( ! $this->is_template_supported( 'is_author' ) ) { return $author_page_urls; } - $number = ! empty( $number ) ? $number : $this->context->get_limit_per_type(); foreach ( get_users( compact( 'offset', 'number' ) ) as $author ) { $author_page_urls[] = get_author_posts_url( $author->ID, $author->user_nicename ); } @@ -253,6 +355,7 @@ private function get_date_page() { return null; } + // @todo This should use get_year_link() and it should use the year of the most recent-published post. return add_query_arg( 'year', gmdate( 'Y' ), home_url( '/' ) ); } @@ -275,18 +378,16 @@ private function does_taxonomy_support_amp( $taxonomy ) { * Gets the front-end links for taxonomy terms. * For example, https://example.org/?cat=2 * - * @param string $taxonomy The name of the taxonomy, like 'category' or 'post_tag'. - * @param int|string $offset The number at which to offset the query (optional). - * @param int $number The maximum amount of links to get (optional). + * @param string $taxonomy The name of the taxonomy, like 'category' or 'post_tag'. + * @param int $offset The number at which to offset the query (optional). + * @param int $number The maximum amount of links to get (optional). * @return string[] The term links, as an array of strings. */ - private function get_taxonomy_links( $taxonomy, $offset = '', $number = null ) { - if ( is_null( $number ) ) { - $number = $this->context->get_limit_per_type(); - } - + private function get_taxonomy_links( $taxonomy, $offset, $number ) { return array_map( - 'get_term_link', + static function ( $term ) { + return get_term_link( $term ); + }, get_terms( array_merge( compact( 'taxonomy', 'offset', 'number' ), diff --git a/src/Validation/URLScanningContext.php b/src/Validation/URLScanningContext.php deleted file mode 100644 index eed3e6c215d..00000000000 --- a/src/Validation/URLScanningContext.php +++ /dev/null @@ -1,76 +0,0 @@ -limit_per_type = $limit_per_type; - $this->include_conditionals = $include_conditionals; - } - - /** - * Provides the limit_per_type setting. - * - * @return int - */ - public function get_limit_per_type() { - return $this->limit_per_type; - } - - /** - * Provides the include_conditionals setting. - * - * @return string[] - */ - public function get_include_conditionals() { - return $this->include_conditionals; - } -} diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 40e09e7ebf7..949723a83eb 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -8,6 +8,7 @@ namespace AmpProject\AmpWP\Validation; +use AmpProject\AmpWP\Infrastructure\Service; use AMP_Validation_Error_Taxonomy; use AMP_Validation_Manager; use WP_Error; @@ -18,7 +19,7 @@ * @since 2.1 * @internal */ -final class URLValidationProvider { +final class URLValidationProvider implements Service { /** * The total number of validation errors, regardless of whether they were accepted. * diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index f83fe812edc..70ef190288a 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -10,7 +10,6 @@ use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; use AmpProject\AmpWP\Tests\TestCase; use AmpProject\AmpWP\Validation\ScannableURLProvider; -use AmpProject\AmpWP\Validation\URLScanningContext; use WP_Query; /** @coversDefaultClass \AmpProject\AmpWP\Validation\ScannableURLProvider */ @@ -31,7 +30,7 @@ final class ScannableURLProviderTest extends TestCase { */ public function setUp() { parent::setUp(); - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } @@ -46,11 +45,12 @@ public function test__construct() { * @covers ::get_urls() */ public function test_count_urls_to_validate() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $number_original_urls = 4; $this->assertCount( $number_original_urls, $this->scannable_url_provider->get_urls() ); - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 100 ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 100 ); $category = self::factory()->term->create( [ 'taxonomy' => 'category' ] ); $number_new_posts = 50; @@ -70,7 +70,7 @@ public function test_count_urls_to_validate() { $expected_url_count = $number_new_posts + $number_original_urls + 1; $this->assertCount( $expected_url_count, $this->scannable_url_provider->get_urls() ); - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 100 ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 100 ); $number_of_new_terms = 20; $expected_url_count += $number_of_new_terms; @@ -125,7 +125,7 @@ public function test_get_posts_that_support_amp() { AMP_Post_Meta_Box::ENABLED_STATUS ); - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); // In AMP-first, the IDs should include all of the newly-created posts. AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); @@ -139,20 +139,6 @@ public function test_get_posts_that_support_amp() { ] ); $this->assertEquals( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); - - /* - * If the WP-CLI command has an include argument, and is_singular isn't in it, no posts will have AMP enabled. - * For example, wp amp validate-site --include=is_tag,is_category - */ - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_tag', 'is_category' ] ) ); - $this->assertEquals( [], $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); - - /* - * If is_singular is in the WP-CLI argument, it should return these posts as being AMP-enabled. - * For example, wp amp validate-site include=is_singular,is_category - */ - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_singular', 'is_category' ] ) ); - $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ) ); } /** @@ -161,6 +147,8 @@ public function test_get_posts_that_support_amp() { * @covers ::get_author_page_urls() */ public function test_get_author_page_urls() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + self::factory()->user->create(); $users = get_users(); $first_author = $users[0]; @@ -179,14 +167,14 @@ public function test_get_author_page_urls() { $this->assertEquals( [ $second_author_url ], $actual_urls ); // If $include_conditionals is set and does not have is_author, this should not return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_category' ] ) ); - $this->assertEquals( [], $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls' ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_category' ], 20 ); + $this->assertEquals( [], $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls', [ 1, 0 ] ) ); // If $include_conditionals is set and has is_author, this should return URLs. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_author' ] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_author' ], 20 ); $this->assertEquals( [ $first_author_url, $second_author_url ], - $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls' ) + $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls', [ 2, 0 ] ) ); } @@ -196,6 +184,8 @@ public function test_get_author_page_urls() { * @covers ::does_taxonomy_support_amp() */ public function test_does_taxonomy_support_amp() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + $custom_taxonomy = 'foo_custom_taxonomy'; register_taxonomy( $custom_taxonomy, 'post' ); $taxonomies_to_test = [ $custom_taxonomy, 'category', 'post_tag' ]; @@ -207,13 +197,14 @@ public function test_does_taxonomy_support_amp() { } // When the user has not checked the boxes for 'Categories' and 'Tags,' this should be false. + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ 'is_author' ] ); AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); foreach ( $taxonomies_to_test as $taxonomy ) { $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); } - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); // When the user has checked the Option::ALL_TEMPLATES_SUPPORTED box, this should always be true. AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, true ); @@ -226,7 +217,7 @@ public function test_does_taxonomy_support_amp() { * If the user passed allowed conditionals to the WP-CLI command like wp amp validate-site --include=is_category,is_tag * these should be supported taxonomies. */ - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_category', 'is_tag' ] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [ Option::ALL_TEMPLATES_SUPPORTED => true ], [ 'is_category', 'is_tag' ], 20 ); $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'category' ] ) ); $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'tag' ] ) ); $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'author' ] ) ); @@ -244,10 +235,12 @@ public function test_is_template_supported() { AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ $author_conditional ] ); AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'is_template_supported', [ $author_conditional ] ) ); $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'is_template_supported', [ $search_conditional ] ) ); AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ $search_conditional ] ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'is_template_supported', [ $search_conditional ] ) ); $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'is_template_supported', [ $author_conditional ] ) ); } @@ -260,6 +253,8 @@ public function test_is_template_supported() { public function test_get_posts_by_type() { $number_posts_each_post_type = 20; $post_types = get_post_types( [ 'public' => true ], 'names' ); + AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, true ); + AMP_Options_Manager::update_option( Option::SUPPORTED_POST_TYPES, $post_types ); foreach ( $post_types as $post_type ) { // Start the expected posts with the existing post(s). @@ -354,11 +349,11 @@ public function test_get_search_page() { $this->assertTrue( is_string( $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ) ); // If $include_conditionals is set and does not have is_search, this should not return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_author' ] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_author' ], 20 ); $this->assertEquals( null, $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ); // If $include_conditionals has is_search, this should return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_search' ] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_search' ], 20 ); $this->assertTrue( is_string( $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ) ); } @@ -374,11 +369,11 @@ public function test_get_date_page() { $this->assertStringContainsString( $year, $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); // If $include_conditionals is set and does not have is_date, this should not return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_search' ] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_search' ], 20 ); $this->assertEquals( null, $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); // If $include_conditionals has is_date, this should return a URL. - $this->scannable_url_provider = new ScannableURLProvider( new URLScanningContext( 20, [ 'is_date' ] ) ); + $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_date' ], 20 ); $parsed_page_url = wp_parse_url( $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); $this->assertStringContainsString( $year, $parsed_page_url['query'] ); } diff --git a/tests/php/src/Validation/URLScanningContextTest.php b/tests/php/src/Validation/URLScanningContextTest.php deleted file mode 100644 index 7bf463f8e1f..00000000000 --- a/tests/php/src/Validation/URLScanningContextTest.php +++ /dev/null @@ -1,79 +0,0 @@ -test_instance = new URLScanningContext(); - } - - /** - * @covers ::__construct() - */ - public function test__construct() { - $this->assertInstanceof( URLScanningContext::class, $this->test_instance ); - } - - /** @covers ::get_limit_per_type() */ - public function test_get_limit_per_type() { - $this->assertEquals( 1, $this->test_instance->get_limit_per_type() ); - - add_filter( - 'amp_url_validation_limit_per_type', - static function() { - return -4; - } - ); - $this->assertEquals( -1, $this->test_instance->get_limit_per_type() ); - - add_filter( - 'amp_url_validation_limit_per_type', - static function() { - return -1; - } - ); - $this->assertEquals( -1, $this->test_instance->get_limit_per_type() ); - - add_filter( - 'amp_url_validation_limit_per_type', - static function() { - return 0; - } - ); - $this->assertEquals( 0, $this->test_instance->get_limit_per_type() ); - } - - /** - * @covers ::get_limit_per_type() - * @covers ::get_include_conditionals() - */ - public function test_getters() { - $this->test_instance = new URLScanningContext( 99, [ 'is_date', 'is_search' ] ); - - $this->assertEquals( 99, $this->test_instance->get_limit_per_type() ); - $this->assertEquals( - [ 'is_date', 'is_search' ], - $this->test_instance->get_include_conditionals() - ); - } -} From ab2bff10f5dbe12766a36b71db045103de507f87 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 12:30:30 -0700 Subject: [PATCH 28/44] Include more AMP settings in validated environment --- .../class-amp-validated-url-post-type.php | 22 +++++++++++++---- .../php/src/Support/SupportCliCommandTest.php | 7 +++--- tests/php/src/Support/SupportDataTest.php | 1 + ...test-class-amp-validated-url-post-type.php | 24 +++++++++++++++++-- 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index 5cf9cbd2b3e..3c86848ff9f 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -1084,9 +1084,16 @@ public static function get_validated_environment() { return [ 'theme' => $theme, 'plugins' => wp_list_pluck( $plugin_registry->get_plugins( true, false ), 'Version' ), // @todo What about multiple plugins being in the same directory? - 'options' => [ - Option::THEME_SUPPORT => AMP_Options_Manager::get_option( Option::THEME_SUPPORT ), - ], + 'options' => wp_array_slice_assoc( + AMP_Options_Manager::get_options(), + [ + Option::ALL_TEMPLATES_SUPPORTED, + Option::READER_THEME, + Option::SUPPORTED_POST_TYPES, + Option::SUPPORTED_TEMPLATES, + Option::THEME_SUPPORT, + ] + ), ]; } @@ -1149,7 +1156,14 @@ public static function get_post_staleness( $post ) { } else { $old_options = []; } - $option_differences = array_diff_assoc( $old_options, $new_validated_environment['options'] ); + $option_differences = []; + foreach ( $new_validated_environment['options'] as $option => $value ) { + if ( ! isset( $old_options[ $option ] ) ) { + $option_differences[ $option ] = null; + } elseif ( $old_options[ $option ] !== $value ) { + $option_differences[ $option ] = $old_options[ $option ]; + } + } if ( ! empty( $option_differences ) ) { $staleness['options'] = $option_differences; } diff --git a/tests/php/src/Support/SupportCliCommandTest.php b/tests/php/src/Support/SupportCliCommandTest.php index 5a1360bc23b..3d8299f005a 100644 --- a/tests/php/src/Support/SupportCliCommandTest.php +++ b/tests/php/src/Support/SupportCliCommandTest.php @@ -95,6 +95,7 @@ public function create_validated_url() { 'id' => $post->ID, 'type' => 'post', ], + \AMP_Validated_URL_Post_Type::VALIDATED_ENVIRONMENT_POST_META_KEY => \AMP_Validated_URL_Post_Type::get_validated_environment(), ], ] ); @@ -124,8 +125,8 @@ public function test_send_diagnostic() { ); $output = ob_get_clean(); - $this->assertContains( wp_json_encode( $amp_validated_url->post_title ), $output ); - $this->assertContains( wp_json_encode( '/wp-includes/js/jquery/jquery.js?ver=__normalized__' ), $output ); - $this->assertContains( wp_json_encode( 'DISALLOWED_TAG' ), $output ); + $this->assertStringContainsString( wp_json_encode( $amp_validated_url->post_title ), $output ); + $this->assertStringContainsString( wp_json_encode( '/wp-includes/js/jquery/jquery.js?ver=__normalized__' ), $output ); + $this->assertStringContainsString( wp_json_encode( 'DISALLOWED_TAG' ), $output ); } } diff --git a/tests/php/src/Support/SupportDataTest.php b/tests/php/src/Support/SupportDataTest.php index 5fd09d9df87..7c8cac7e7e2 100644 --- a/tests/php/src/Support/SupportDataTest.php +++ b/tests/php/src/Support/SupportDataTest.php @@ -517,6 +517,7 @@ public function create_validated_url() { 'id' => $post->ID, 'type' => 'post', ], + AMP_Validated_URL_Post_Type::VALIDATED_ENVIRONMENT_POST_META_KEY => AMP_Validated_URL_Post_Type::get_validated_environment(), ], ] ); diff --git a/tests/php/validation/test-class-amp-validated-url-post-type.php b/tests/php/validation/test-class-amp-validated-url-post-type.php index a4e44891119..00d21356ccb 100644 --- a/tests/php/validation/test-class-amp-validated-url-post-type.php +++ b/tests/php/validation/test-class-amp-validated-url-post-type.php @@ -5,6 +5,7 @@ * @package AMP */ +use AmpProject\AmpWP\Admin\ReaderThemes; use AmpProject\AmpWP\DevTools\UserAccess; use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Services; @@ -599,15 +600,34 @@ public function test_get_validated_environment() { $this->assertArrayHasKey( 'theme', $old_env ); $this->assertArrayHasKey( 'plugins', $old_env ); $this->assertEquals( [ 'twentysixteen' => wp_get_theme( 'twentysixteen' )->get( 'Version' ) ], $old_env['theme'] ); - $this->assertEquals( [ Option::THEME_SUPPORT => AMP_Theme_Support::TRANSITIONAL_MODE_SLUG ], $old_env['options'] ); + $this->assertEquals( + [ + Option::THEME_SUPPORT => AMP_Theme_Support::TRANSITIONAL_MODE_SLUG, + Option::ALL_TEMPLATES_SUPPORTED => true, + Option::READER_THEME => ReaderThemes::DEFAULT_READER_THEME, + Option::SUPPORTED_POST_TYPES => [ 'post', 'page' ], + Option::SUPPORTED_TEMPLATES => [ 'is_singular' ], + ], + $old_env['options'] + ); switch_theme( 'twentyseventeen' ); update_option( 'active_plugins', [ 'foo/foo.php', 'baz.php' ] ); AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); $new_env = AMP_Validated_URL_Post_Type::get_validated_environment(); $this->assertNotEquals( $old_env, $new_env ); $this->assertEquals( [ 'twentyseventeen' => wp_get_theme( 'twentyseventeen' )->get( 'Version' ) ], $new_env['theme'] ); - $this->assertEquals( [ Option::THEME_SUPPORT => AMP_Theme_Support::STANDARD_MODE_SLUG ], $new_env['options'] ); + $this->assertEquals( + [ + Option::THEME_SUPPORT => AMP_Theme_Support::STANDARD_MODE_SLUG, + Option::ALL_TEMPLATES_SUPPORTED => false, + Option::READER_THEME => ReaderThemes::DEFAULT_READER_THEME, + Option::SUPPORTED_POST_TYPES => [ 'post', 'page' ], + Option::SUPPORTED_TEMPLATES => [ 'is_singular' ], + ], + $new_env['options'] + ); } /** From f0c7b3b7cc536efaa0dc465c4d08bc12990d6760 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 13:06:11 -0700 Subject: [PATCH 29/44] Fix tests after 5bbadea --- .../src/Validation/ScannableURLProviderTest.php | 7 +++++++ .../php/src/Validation/URLValidationCronTest.php | 4 +++- .../test-class-amp-cli-validation-command.php | 16 +++++++++------- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 70ef190288a..86e1823b6ab 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -230,6 +230,8 @@ public function test_does_taxonomy_support_amp() { * @covers ::is_template_supported() */ public function test_is_template_supported() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + $author_conditional = 'is_author'; $search_conditional = 'is_search'; @@ -345,6 +347,8 @@ public function test_get_taxonomy_links() { * @covers ::get_search_page() */ public function test_get_search_page() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + // Normally, this should return a string, unless the user has opted out of the search template. $this->assertTrue( is_string( $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ) ); @@ -363,6 +367,9 @@ public function test_get_search_page() { * @covers ::get_date_page() */ public function test_get_date_page() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); + $year = gmdate( 'Y' ); // Normally, this should return the date page, unless the user has opted out of that template. diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 3ed3c468874..2764a6879b4 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -5,6 +5,7 @@ use AmpProject\AmpWP\BackgroundTask\CronBasedBackgroundTask; use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; +use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Tests\DependencyInjectedTestCase; use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; @@ -140,10 +141,11 @@ public function test_schedule_event_with_different_recurrence() { * @covers ::process() * @covers ::dequeue() */ - public function test_validate_urls() { + public function test_process_and_dequeue() { /** @var ScannableURLProvider $scannable_url_provider */ $scannable_url_provider = $this->get_private_property( $this->test_instance, 'scannable_url_provider' ); + self::factory()->post->create(); $initial_urls = wp_list_pluck( $scannable_url_provider->get_urls(), 'url' ); $initial_url_count = count( $initial_urls ); $this->assertGreaterThan( 0, $initial_url_count ); diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index bd3c5d0a10e..cd47aae8ff1 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -9,7 +9,7 @@ use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; -use AmpProject\AmpWP\Tests\TestCase; +use AmpProject\AmpWP\Tests\DependencyInjectedTestCase; /** * Tests for Test_AMP_CLI_Validation_Command class. @@ -18,7 +18,7 @@ * * @coversDefaultClass \AmpProject\AmpWP\Cli\ValidationCommand */ -class Test_AMP_CLI_Validation_Command extends TestCase { +class Test_AMP_CLI_Validation_Command extends DependencyInjectedTestCase { use PrivateAccess, ValidationRequestMocking; @@ -38,7 +38,7 @@ public function setUp() { parent::setUp(); AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); - $this->validation = new ValidationCommand(); + $this->validation = $this->injector->make( ValidationCommand::class ); add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } @@ -46,8 +46,6 @@ public function setUp() { * Test validate_urls. * * @covers ::validate_urls() - * @covers ::get_validation_provider() - * @covers ::get_validation_url_provider() */ public function test_validate_urls() { $number_of_posts = 20; @@ -56,6 +54,9 @@ public function test_validate_urls() { $post_permalinks = []; $terms = []; + $this->validation = $this->injector->make( ValidationCommand::class ); + $this->get_private_property( $this->validation, 'scannable_url_provider' )->set_limit_per_type( 100 ); + for ( $i = 0; $i < $number_of_posts; $i++ ) { $post_id = self::factory()->post->create(); $posts[] = $post_id; @@ -66,7 +67,8 @@ public function test_validate_urls() { // All of the posts created above should be present in $validated_urls. $this->assertEmpty( array_diff( $post_permalinks, $this->get_validated_urls() ) ); - $this->validation = new ValidationCommand(); + $this->validation = $this->injector->make( ValidationCommand::class ); + $this->get_private_property( $this->validation, 'scannable_url_provider' )->set_limit_per_type( 100 ); for ( $i = 0; $i < $number_of_terms; $i++ ) { $terms[] = self::factory()->category->create(); } @@ -79,6 +81,6 @@ public function test_validate_urls() { // All of the terms created above should be present in $validated_urls. $this->assertEmpty( array_diff( $expected_validated_urls, $actual_validated_urls ) ); - $this->assertStringContainsString( home_url( '/' ), $this->get_validated_urls() ); + $this->assertContains( home_url( '/' ), $this->get_validated_urls() ); } } From 55c0ea7ef0d9125e22b88099eb05fafe5b8be75c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 13:17:35 -0700 Subject: [PATCH 30/44] Clear out cron validation queue whenever validated environment changes --- src/Validation/URLValidationCron.php | 15 +++++++++++++-- .../php/src/Validation/URLValidationCronTest.php | 9 +++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index 016afe85b65..097c033fc4a 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 AMP_Validated_URL_Post_Type; use AMP_Validation_Manager; /** @@ -77,14 +78,24 @@ protected function dequeue() { if ( ! is_array( $data ) ) { $data = []; } + $data = array_merge( [ - 'timestamp' => 0, 'urls' => [], + 'timestamp' => 0, + 'env' => [], ], $data ); + $current_env = AMP_Validated_URL_Post_Type::get_validated_environment(); + + // When the validated environment changes, make sure the URLs and timestamp are reset so that new URLs are obtained. + if ( $data['timestamp'] && $data['env'] !== $current_env ) { + $data['urls'] = []; + $data['timestamp'] = 0; + } + // If there are no URLs queued, then obtain a new set. if ( empty( $data['urls'] ) ) { @@ -93,7 +104,6 @@ protected function dequeue() { 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(); } @@ -101,6 +111,7 @@ protected function dequeue() { // If there is not a queued URL, then enqueue a new set of URLs. $url = array_shift( $data['urls'] ); + $data['env'] = $current_env; update_option( self::OPTION_KEY, $data ); return $url ?: null; diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 2764a6879b4..f54b1c91490 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -2,6 +2,8 @@ namespace AmpProject\AmpWP\Tests\Validation; +use AMP_Options_Manager; +use AMP_Theme_Support; use AmpProject\AmpWP\BackgroundTask\CronBasedBackgroundTask; use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; @@ -142,6 +144,8 @@ public function test_schedule_event_with_different_recurrence() { * @covers ::dequeue() */ public function test_process_and_dequeue() { + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); + /** @var ScannableURLProvider $scannable_url_provider */ $scannable_url_provider = $this->get_private_property( $this->test_instance, 'scannable_url_provider' ); @@ -182,6 +186,11 @@ public function test_process_and_dequeue() { $this->assertEquals( $before_request_count + 1, $this->request_count ); $data = get_option( URLValidationCron::OPTION_KEY ); $this->assertCount( $initial_url_count - 1, $data['urls'] ); + + // Now test once the validated environment has changed, that the URLs are re-queued. + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG ); + $this->test_instance->process(); + $this->assertCount( $initial_url_count - 1, $data['urls'] ); } /** @covers ::get_event_name() */ From 042a74c4d790a568f008f75b630032395d5c9352 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 13:18:44 -0700 Subject: [PATCH 31/44] Remove obsolete test for amp_should_use_new_onboarding --- tests/php/test-includes-admin-functions.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/php/test-includes-admin-functions.php b/tests/php/test-includes-admin-functions.php index 97aa94876eb..6a6295c2447 100644 --- a/tests/php/test-includes-admin-functions.php +++ b/tests/php/test-includes-admin-functions.php @@ -248,9 +248,4 @@ public function test_amp_bootstrap_admin() { $this->assertTrue( has_action( 'admin_enqueue_scripts' ) ); $this->assertTrue( has_action( 'post_submitbox_misc_actions' ) ); } - - /** @covers ::amp_should_use_new_onboarding() */ - public function test_amp_should_use_new_onboarding() { - $this->markTestIncomplete( 'This function may be eliminated as of https://github.com/ampproject/amp-wp/pull/6225' ); - } } From 4e0dda5a89050323d1d5243fea5ed5f4ea75beb9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 14:35:41 -0700 Subject: [PATCH 32/44] Fix test assertions by using polyfilled TestCase --- tests/php/src/Support/SupportCliCommandTest.php | 4 ++-- tests/php/src/Support/SupportDataTest.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/php/src/Support/SupportCliCommandTest.php b/tests/php/src/Support/SupportCliCommandTest.php index 3d8299f005a..2cf75225ee5 100644 --- a/tests/php/src/Support/SupportCliCommandTest.php +++ b/tests/php/src/Support/SupportCliCommandTest.php @@ -9,7 +9,7 @@ use AmpProject\AmpWP\Support\SupportData; use AmpProject\AmpWP\Support\SupportCliCommand; -use WP_UnitTestCase; +use AmpProject\AmpWP\Tests\TestCase; /** * Tests for SupportCliCommandTest. @@ -17,7 +17,7 @@ * @group support-admin * @coversDefaultClass \AmpProject\AmpWP\Support\SupportCliCommand */ -class SupportCliCommandTest extends WP_UnitTestCase { +class SupportCliCommandTest extends TestCase { /** * Instance of OptionsMenu diff --git a/tests/php/src/Support/SupportDataTest.php b/tests/php/src/Support/SupportDataTest.php index 7c8cac7e7e2..6ddda00d0f4 100644 --- a/tests/php/src/Support/SupportDataTest.php +++ b/tests/php/src/Support/SupportDataTest.php @@ -10,7 +10,7 @@ use AmpProject\AmpWP\QueryVar; use AmpProject\AmpWP\Support\SupportData; use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; -use WP_UnitTestCase; +use AmpProject\AmpWP\Tests\TestCase; use AMP_Validated_URL_Post_Type; /** @@ -19,7 +19,7 @@ * @group support-admin * @coversDefaultClass \AmpProject\AmpWP\Support\SupportData */ -class SupportDataTest extends WP_UnitTestCase { +class SupportDataTest extends TestCase { use PrivateAccess; From 42a2c35c181cf74e793446d6cc8b3bb0d9f87407 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 14:43:53 -0700 Subject: [PATCH 33/44] Remove unused DEFAULT_INTERVAL_WEEKLY and fix test_process_and_dequeue assertions --- src/BackgroundTask/CronBasedBackgroundTask.php | 1 - tests/php/src/Validation/URLValidationCronTest.php | 6 ++++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/BackgroundTask/CronBasedBackgroundTask.php b/src/BackgroundTask/CronBasedBackgroundTask.php index 8a9ff58a372..925e81ed4a6 100644 --- a/src/BackgroundTask/CronBasedBackgroundTask.php +++ b/src/BackgroundTask/CronBasedBackgroundTask.php @@ -22,7 +22,6 @@ 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. diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index f54b1c91490..677e292d1f3 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -187,9 +187,15 @@ public function test_process_and_dequeue() { $data = get_option( URLValidationCron::OPTION_KEY ); $this->assertCount( $initial_url_count - 1, $data['urls'] ); + $this->test_instance->process(); + $this->assertEquals( $before_request_count + 2, $this->request_count ); + $data = get_option( URLValidationCron::OPTION_KEY ); + $this->assertCount( $initial_url_count - 2, $data['urls'] ); + // Now test once the validated environment has changed, that the URLs are re-queued. AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG ); $this->test_instance->process(); + $data = get_option( URLValidationCron::OPTION_KEY ); $this->assertCount( $initial_url_count - 1, $data['urls'] ); } From acf93dd745c746b6876ecbdba587812cbb6b5318 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 15:24:41 -0700 Subject: [PATCH 34/44] Use get_year_link() to obtain the year archive link and ensure it has posts on it --- src/Validation/ScannableURLProvider.php | 28 +++++++++++-- src/Validation/URLValidationCron.php | 1 - .../Validation/ScannableURLProviderTest.php | 40 +++++++++++++++++-- 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 91aacb76dfd..fbfcb76c957 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -346,17 +346,37 @@ private function get_search_page() { } /** - * Gets a single date page URL, like https://example.com/?year=2018. + * Gets a single date page URL, like https://example.com/2018/. * - * @return string|null An example search page, or null. + * @return string|null An example year archive URL, or null. */ private function get_date_page() { if ( ! $this->is_template_supported( 'is_date' ) ) { return null; } - // @todo This should use get_year_link() and it should use the year of the most recent-published post. - return add_query_arg( 'year', gmdate( 'Y' ), home_url( '/' ) ); + $query = new WP_Query( + [ + 'post_type' => 'post', + 'post_status' => 'publish', + 'posts_per_page' => 1, + 'orderby' => 'date', + 'order' => 'DESC', + ] + ); + $posts = $query->get_posts(); + + $latest_post = array_shift( $posts ); + if ( ! $latest_post ) { + return null; + } + + $year = (int) get_the_date( 'Y', $latest_post ); + if ( ! $year ) { + return null; + } + + return get_year_link( $year ); } /** diff --git a/src/Validation/URLValidationCron.php b/src/Validation/URLValidationCron.php index 097c033fc4a..29315358852 100644 --- a/src/Validation/URLValidationCron.php +++ b/src/Validation/URLValidationCron.php @@ -108,7 +108,6 @@ protected function dequeue() { $data['timestamp'] = time(); } - // If there is not a queued URL, then enqueue a new set of URLs. $url = array_shift( $data['urls'] ); $data['env'] = $current_env; diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 86e1823b6ab..79945f5c250 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -367,13 +367,19 @@ public function test_get_search_page() { * @covers ::get_date_page() */ public function test_get_date_page() { + $this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' ); + + $post = self::factory()->post->create(); + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $this->scannable_url_provider = new ScannableURLProvider( [], [], 20 ); - $year = gmdate( 'Y' ); + $year = get_the_date( 'Y', $post ); // Normally, this should return the date page, unless the user has opted out of that template. - $this->assertStringContainsString( $year, $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); + $url = $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ); + $this->assertIsString( $url ); + $this->assertStringContainsString( get_year_link( $year ), $url ); // If $include_conditionals is set and does not have is_date, this should not return a URL. $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_search' ], 20 ); @@ -381,7 +387,33 @@ public function test_get_date_page() { // If $include_conditionals has is_date, this should return a URL. $this->scannable_url_provider = new ScannableURLProvider( [], [ 'is_date' ], 20 ); - $parsed_page_url = wp_parse_url( $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); - $this->assertStringContainsString( $year, $parsed_page_url['query'] ); + $this->assertStringContainsString( get_year_link( $year ), $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); + + // If all posts are deleted, then nothing should be returned. + $query = new WP_Query( + [ + 'post_type' => 'post', + 'posts_per_page' => -1, + 'fields' => 'ids', + ] + ); + foreach ( $query->get_posts() as $deleted_post ) { + wp_delete_post( $deleted_post ); + } + $this->assertNull( $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); + + // Same goes if there is only one post and it lacks a year. + $timeless_post = self::factory()->post->create(); + global $wpdb; + $wpdb->update( + $wpdb->posts, + [ + 'post_date' => '0000-00-00 00:00:00', + 'post_date_gmt' => '0000-00-00 00:00:00', + ], + [ 'ID' => $timeless_post ] + ); + clean_post_cache( $timeless_post ); + $this->assertNull( $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); } } From 2e94ba4b2b1b5bd8979be35ac77e28a242b8d939 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 15:48:11 -0700 Subject: [PATCH 35/44] Ensure author links are not 404 --- src/Validation/ScannableURLProvider.php | 12 +++++++++++- .../php/src/Validation/ScannableURLProviderTest.php | 11 +++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index fbfcb76c957..1556fb3d4e1 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -326,7 +326,17 @@ private function get_author_page_urls( $offset, $number ) { } foreach ( get_users( compact( 'offset', 'number' ) ) as $author ) { - $author_page_urls[] = get_author_posts_url( $author->ID, $author->user_nicename ); + $authored_post_query = new WP_Query( + [ + 'post_type' => 'post', + 'post_status' => 'publish', + 'author' => $author->ID, + 'posts_per_page' => 1, + ] + ); + if ( count( $authored_post_query->get_posts() ) > 0 ) { + $author_page_urls[] = get_author_posts_url( $author->ID, $author->user_nicename ); + } } return $author_page_urls; diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 79945f5c250..1aaf4952c12 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -45,8 +45,11 @@ public function test__construct() { * @covers ::get_urls() */ public function test_count_urls_to_validate() { + $user = self::factory()->user->create(); + self::factory()->post->create( [ 'post_author' => $user ] ); + AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); - $number_original_urls = 4; + $number_original_urls = 6; $this->assertCount( $number_original_urls, $this->scannable_url_provider->get_urls() ); @@ -63,11 +66,7 @@ public function test_count_urls_to_validate() { ); } - /* - * Add the number of new posts, original URLs, and 1 for the $category that all of them have. - * And ensure that the tested method finds a URL for all of them. - */ - $expected_url_count = $number_new_posts + $number_original_urls + 1; + $expected_url_count = $number_new_posts + $number_original_urls; $this->assertCount( $expected_url_count, $this->scannable_url_provider->get_urls() ); $this->scannable_url_provider = new ScannableURLProvider( [], [], 100 ); From 6d25e3cd5ad265ea5adc49be7222443245054e1e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 15:55:03 -0700 Subject: [PATCH 36/44] Improve test coverage --- tests/php/src/Validation/ScannableURLProviderTest.php | 3 +++ tests/php/src/Validation/URLValidationCronTest.php | 2 +- tests/php/test-class-amp-cli-validation-command.php | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 1aaf4952c12..8b2632acc71 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -43,6 +43,9 @@ public function test__construct() { * Test retrieval of urls. * * @covers ::get_urls() + * @covers ::get_options() + * @covers ::get_supportable_templates() + * @covers ::is_template_supported() */ public function test_count_urls_to_validate() { $user = self::factory()->user->create(); diff --git a/tests/php/src/Validation/URLValidationCronTest.php b/tests/php/src/Validation/URLValidationCronTest.php index 677e292d1f3..784c50cc780 100644 --- a/tests/php/src/Validation/URLValidationCronTest.php +++ b/tests/php/src/Validation/URLValidationCronTest.php @@ -154,7 +154,7 @@ public function test_process_and_dequeue() { $initial_url_count = count( $initial_urls ); $this->assertGreaterThan( 0, $initial_url_count ); - delete_option( URLValidationCron::OPTION_KEY ); + update_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++ ) { diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index cd47aae8ff1..f9d613cd70b 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -46,6 +46,7 @@ public function setUp() { * Test validate_urls. * * @covers ::validate_urls() + * @covers \AmpProject\AmpWP\Validation\ScannableURLProvider::set_limit_per_type() */ public function test_validate_urls() { $number_of_posts = 20; From 1b1b7b1fccd83dde683f4000128bb6cff7cfa414 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 16:08:20 -0700 Subject: [PATCH 37/44] Fix test for get_author_page_urls --- tests/php/src/Validation/ScannableURLProviderTest.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 8b2632acc71..b8cbd6f40ae 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -158,6 +158,11 @@ public function test_get_author_page_urls() { $second_author = $users[1]; $second_author_url = get_author_posts_url( $second_author->ID, $second_author->user_nicename ); + $actual_urls = $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls', [ 0, 1 ] ); + $this->assertCount( 0, $actual_urls ); + + self::factory()->post->create( [ 'post_author' => $first_author->ID ] ); + self::factory()->post->create( [ 'post_author' => $second_author->ID ] ); $actual_urls = $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls', [ 0, 1 ] ); // Passing 0 as the offset argument should get the first author. From 27b18debdb1cfd25a02d87fde3b36405ca06524a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 15 Oct 2021 16:21:43 -0700 Subject: [PATCH 38/44] Fix get_date_page test in WP<=5.1 --- src/Validation/ScannableURLProvider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 1556fb3d4e1..ec4b326edfa 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -382,7 +382,7 @@ private function get_date_page() { } $year = (int) get_the_date( 'Y', $latest_post ); - if ( ! $year ) { + if ( $year <= 0 ) { return null; } From fce116ddbf0c7e5a78657fd4427f0933d5df9eb2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 20 Oct 2021 11:24:05 -0700 Subject: [PATCH 39/44] Create a post before going to the onboarding wizard so there is something to preview --- tests/e2e/utils/onboarding-wizard-utils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/e2e/utils/onboarding-wizard-utils.js b/tests/e2e/utils/onboarding-wizard-utils.js index e9f8286a262..124ac3f9e98 100644 --- a/tests/e2e/utils/onboarding-wizard-utils.js +++ b/tests/e2e/utils/onboarding-wizard-utils.js @@ -1,7 +1,7 @@ /** * WordPress dependencies */ -import { visitAdminPage } from '@wordpress/e2e-test-utils'; +import { createNewPost, visitAdminPage } from '@wordpress/e2e-test-utils'; export const NEXT_BUTTON_SELECTOR = '#next-button'; export const PREV_BUTTON_SELECTOR = '.amp-settings-nav__prev-next button:not(.is-primary)'; @@ -9,6 +9,7 @@ export const PREV_BUTTON_SELECTOR = '.amp-settings-nav__prev-next button:not(.is export async function goToOnboardingWizard() { await visitAdminPage( 'index.php' ); await expect( page ).not.toMatchElement( '#amp-onboarding-wizard' ); + await createNewPost(); // So that there is a post to appear on the done screen. await visitAdminPage( 'admin.php', 'page=amp-onboarding-wizard' ); await expect( page ).toMatchElement( '#amp-onboarding-wizard' ); } From 8f2985a26b97251b211f2f41095b4160823b0fcf Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Thu, 21 Oct 2021 13:02:31 +0200 Subject: [PATCH 40/44] Create test post and page using REST API --- tests/e2e/specs/amp-onboarding/done.js | 41 ++++++++++++++++++++++ tests/e2e/utils/onboarding-wizard-utils.js | 3 +- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/tests/e2e/specs/amp-onboarding/done.js b/tests/e2e/specs/amp-onboarding/done.js index a053be31d16..6c1454bb1a5 100644 --- a/tests/e2e/specs/amp-onboarding/done.js +++ b/tests/e2e/specs/amp-onboarding/done.js @@ -1,3 +1,7 @@ +/** + * WordPress dependencies + */ +import { trashAllPosts, visitAdminPage } from '@wordpress/e2e-test-utils'; /** * Internal dependencies @@ -29,6 +33,43 @@ async function testCommonDoneStepElements() { } describe( 'Done', () => { + let testPost; + let testPage; + + beforeAll( async () => { + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + testPost = await page.evaluate( () => wp.apiFetch( { + path: '/wp/v2/posts', + method: 'POST', + data: { title: 'Test Post', status: 'publish' }, + } ) ); + testPage = await page.evaluate( () => wp.apiFetch( { + path: '/wp/v2/pages', + method: 'POST', + data: { title: 'Test Page', status: 'publish' }, + } ) ); + } ); + + afterAll( async () => { + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + if ( testPost.id ) { + await page.evaluate( ( id ) => wp.apiFetch( { + path: `/wp/v2/posts/${ id }`, + method: 'DELETE', + data: { force: true }, + } ), testPost.id ); + } + if ( testPage.id ) { + await page.evaluate( ( id ) => wp.apiFetch( { + path: `/wp/v2/pages/${ id }`, + method: 'DELETE', + data: { force: true }, + } ), testPage.id ); + } + } ); + afterEach( async () => { await cleanUpSettings(); } ); diff --git a/tests/e2e/utils/onboarding-wizard-utils.js b/tests/e2e/utils/onboarding-wizard-utils.js index 124ac3f9e98..e9f8286a262 100644 --- a/tests/e2e/utils/onboarding-wizard-utils.js +++ b/tests/e2e/utils/onboarding-wizard-utils.js @@ -1,7 +1,7 @@ /** * WordPress dependencies */ -import { createNewPost, visitAdminPage } from '@wordpress/e2e-test-utils'; +import { visitAdminPage } from '@wordpress/e2e-test-utils'; export const NEXT_BUTTON_SELECTOR = '#next-button'; export const PREV_BUTTON_SELECTOR = '.amp-settings-nav__prev-next button:not(.is-primary)'; @@ -9,7 +9,6 @@ export const PREV_BUTTON_SELECTOR = '.amp-settings-nav__prev-next button:not(.is export async function goToOnboardingWizard() { await visitAdminPage( 'index.php' ); await expect( page ).not.toMatchElement( '#amp-onboarding-wizard' ); - await createNewPost(); // So that there is a post to appear on the done screen. await visitAdminPage( 'admin.php', 'page=amp-onboarding-wizard' ); await expect( page ).toMatchElement( '#amp-onboarding-wizard' ); } From 43899d394458d3a9315350b60dbb306b7b091172 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Thu, 21 Oct 2021 13:03:55 +0200 Subject: [PATCH 41/44] Do not show site preview if there are no preview URLs --- .../src/onboarding-wizard/pages/done/index.js | 77 +++++++++++++------ .../pages/done/use-preview.js | 9 ++- assets/src/settings-page/site-review.js | 2 +- tests/e2e/specs/amp-onboarding/done.js | 11 +++ 4 files changed, 70 insertions(+), 29 deletions(-) diff --git a/assets/src/onboarding-wizard/pages/done/index.js b/assets/src/onboarding-wizard/pages/done/index.js index 525f4a6f0ab..188226f9507 100644 --- a/assets/src/onboarding-wizard/pages/done/index.js +++ b/assets/src/onboarding-wizard/pages/done/index.js @@ -47,7 +47,14 @@ export function Done() { const { didSaveDeveloperToolsOption, saveDeveloperToolsOption, savingDeveloperToolsOption } = useContext( User ); const { canGoForward, setCanGoForward } = useContext( Navigation ); const { downloadedTheme, downloadingTheme, downloadingThemeError } = useContext( ReaderThemes ); - const { previewLinks, setActivePreviewLink, previewUrl, isPreviewingAMP, toggleIsPreviewingAMP } = usePreview(); + const { + hasPreview, + isPreviewingAMP, + previewLinks, + previewUrl, + setActivePreviewLink, + toggleIsPreviewingAMP, + } = usePreview(); /** * Allow the finish button to be enabled. @@ -104,39 +111,55 @@ export function Done() { { __( 'Your site is ready to bring great experiences to your users!', 'amp' ) }

{ STANDARD === themeSupport && ( -

- { __( 'In Standard mode there is a single AMP version of your site. Browse your site here to ensure it meets your expectations.', 'amp' ) } -

+ <> +

+ { __( 'In Standard mode there is a single AMP version of your site.', 'amp' ) } +

+ { hasPreview && ( +

+ { __( 'Browse your site here to ensure it meets your expectations.', 'amp' ) } +

+ ) } + ) } { TRANSITIONAL === themeSupport && ( <>

{ __( 'In Transitional mode AMP and non-AMP versions of your site are served using your currently active theme.', 'amp' ) }

-

- { __( 'Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) } -

+ { hasPreview && ( +

+ { __( 'Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) } +

+ ) } ) } { READER === themeSupport && ( <>

- { __( 'In Reader mode AMP is served using your selected Reader theme, and pages for your non-AMP site are served using your primary theme. Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) } + { __( 'In Reader mode AMP is served using your selected Reader theme, and pages for your non-AMP site are served using your primary theme.', 'amp' ) }

+ { hasPreview && ( +

+ { __( 'Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) } +

+ ) }

{ __( 'As a last step, use the Customizer to tailor the Reader theme as needed.', 'amp' ) }

) } -
- { - e.preventDefault(); - setActivePreviewLink( link ); - } } - /> -
+ { hasPreview && ( +
+ { + e.preventDefault(); + setActivePreviewLink( link ); + } } + /> +
+ ) }
{ READER === themeSupport && downloadingThemeError && ( @@ -144,15 +167,19 @@ export function Done() { { __( 'There was an error downloading your Reader theme. As a result, your site is currently using the legacy reader theme. Please install your chosen theme manually.', 'amp' ) } ) } - { STANDARD !== themeSupport && ( - + { hasPreview && ( + <> + { STANDARD !== themeSupport && ( + + ) } + + ) } -

diff --git a/assets/src/onboarding-wizard/pages/done/use-preview.js b/assets/src/onboarding-wizard/pages/done/use-preview.js index 1c2fd67a066..e9a2b708f75 100644 --- a/assets/src/onboarding-wizard/pages/done/use-preview.js +++ b/assets/src/onboarding-wizard/pages/done/use-preview.js @@ -15,9 +15,11 @@ import { Options } from '../../../components/options-context-provider'; import { STANDARD } from '../../../common/constants'; export function usePreview() { + const hasPreview = PREVIEW_URLS.length > 0; + const { editedOptions: { theme_support: themeSupport } } = useContext( Options ); const [ isPreviewingAMP, setIsPreviewingAMP ] = useState( themeSupport !== STANDARD ); - const [ previewedPageType, setPreviewedPageType ] = useState( PREVIEW_URLS[ 0 ].type ); + const [ previewedPageType, setPreviewedPageType ] = useState( hasPreview ? PREVIEW_URLS[ 0 ].type : null ); const toggleIsPreviewingAMP = () => setIsPreviewingAMP( ( mode ) => ! mode ); const setActivePreviewLink = ( link ) => setPreviewedPageType( link.type ); @@ -32,10 +34,11 @@ export function usePreview() { const previewUrl = useMemo( () => previewLinks.find( ( link ) => link.isActive )?.url, [ previewLinks ] ); return { + hasPreview, + isPreviewingAMP, previewLinks, - setActivePreviewLink, previewUrl, - isPreviewingAMP, + setActivePreviewLink, toggleIsPreviewingAMP, }; } diff --git a/assets/src/settings-page/site-review.js b/assets/src/settings-page/site-review.js index b4bf410a69b..00c29ea232b 100644 --- a/assets/src/settings-page/site-review.js +++ b/assets/src/settings-page/site-review.js @@ -40,7 +40,7 @@ export function SiteReview() { return null; } - const previewPermalink = STANDARD === themeSupport ? HOME_URL : pairedUrlExamples[ pairedUrlStructure ][ 0 ]; + const previewPermalink = STANDARD === themeSupport ? HOME_URL : pairedUrlExamples?.[ pairedUrlStructure ]?.[ 0 ] ?? HOME_URL; return ( { await expect( '.done__preview-container input[type="checkbox"]' ).countToBe( 1 ); } ); + + it( 'does not render site preview in reader mode if there are no posts and pages', async () => { + await trashAllPosts(); + await trashAllPosts( 'page' ); + + await moveToDoneScreen( { mode: 'reader' } ); + + await expect( page ).toMatchElement( 'h1', { text: 'Done' } ); + await expect( page ).not.toMatchElement( '.done__preview-iframe' ); + } ); } ); From 331955f4e17808de31b394be7005361c854d6d8d Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Thu, 21 Oct 2021 13:05:04 +0200 Subject: [PATCH 42/44] Clean up site preview E2E test --- tests/e2e/specs/amp-onboarding/done.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/e2e/specs/amp-onboarding/done.js b/tests/e2e/specs/amp-onboarding/done.js index c6254eee342..f4b858c4d91 100644 --- a/tests/e2e/specs/amp-onboarding/done.js +++ b/tests/e2e/specs/amp-onboarding/done.js @@ -83,7 +83,7 @@ describe( 'Done', () => { await testCommonDoneStepElements(); await expect( page ).toMatchElement( 'p', { text: /Standard mode/i } ); - await expect( '.done__preview-container input[type="checkbox"]' ).countToBe( 0 ); + await expect( page ).not.toMatchElement( '.done__preview-container input[type="checkbox"]' ); } ); it( 'renders transitional mode site review screen', async () => { @@ -94,7 +94,7 @@ describe( 'Done', () => { await testCommonDoneStepElements(); await expect( page ).toMatchElement( 'p', { text: /Transitional mode/i } ); - await expect( '.done__preview-container input[type="checkbox"]:checked' ).countToBe( 1 ); + await expect( page ).toMatchElement( '.done__preview-container input[type="checkbox"]:checked' ); await page.waitForSelector( '.done__preview-iframe' ); const originalIframeSrc = await page.$eval( '.done__preview-iframe', ( e ) => e.getAttribute( 'src' ) ); @@ -106,7 +106,7 @@ describe( 'Done', () => { expect( updatedIframeSrc ).not.toBe( originalIframeSrc ); - await expect( '.done__preview-container input[type="checkbox"]:not(:checked)' ).countToBe( 1 ); + await expect( page ).toMatchElement( '.done__preview-container input[type="checkbox"]:not(:checked)' ); } ); it( 'renders reader mode site review screen', async () => { @@ -117,9 +117,7 @@ describe( 'Done', () => { await testCommonDoneStepElements(); await expect( page ).toMatchElement( 'p', { text: /Reader mode/i } ); - await expect( page ).toMatchElement( '.done__preview-iframe' ); - - await expect( '.done__preview-container input[type="checkbox"]' ).countToBe( 1 ); + await expect( page ).toMatchElement( '.done__preview-container input[type="checkbox"]' ); } ); it( 'does not render site preview in reader mode if there are no posts and pages', async () => { From 8376583aed9670c6380677fd78c99c688153c7cd Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Thu, 21 Oct 2021 13:28:16 +0200 Subject: [PATCH 43/44] Fix AMP Settings Review panel E2E test There needs to be at least one post or page for the "Browse Site" button to redirect to an AMP document. We're creating a test post before all tests and clean up the environment afterwards. --- tests/e2e/specs/admin/amp-options.js | 52 ++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/tests/e2e/specs/admin/amp-options.js b/tests/e2e/specs/admin/amp-options.js index 4dadfb5f473..3f55054a9d9 100644 --- a/tests/e2e/specs/admin/amp-options.js +++ b/tests/e2e/specs/admin/amp-options.js @@ -137,6 +137,30 @@ describe( 'Saving', () => { } ); describe( 'AMP settings screen Review panel', () => { + let testPost; + + beforeAll( async () => { + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + testPost = await page.evaluate( () => wp.apiFetch( { + path: '/wp/v2/posts', + method: 'POST', + data: { title: 'Test Post', status: 'publish' }, + } ) ); + } ); + + afterAll( async () => { + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + if ( testPost.id ) { + await page.evaluate( ( id ) => wp.apiFetch( { + path: `/wp/v2/posts/${ id }`, + method: 'DELETE', + data: { force: true }, + } ), testPost.id ); + } + } ); + beforeEach( async () => { await visitAdminPage( 'admin.php', 'page=amp-options' ); } ); @@ -145,10 +169,13 @@ describe( 'AMP settings screen Review panel', () => { await cleanUpSettings(); } ); - async function clickBrowseSiteInTemplateMode( mode ) { + async function changeAndSaveTemplateMode( mode ) { await clickMode( mode ); - await expect( page ).toClick( 'a', { text: 'Browse Site' } ); - await page.waitForNavigation(); + + await Promise.all( [ + scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ), + page.waitForResponse( ( response ) => response.url().includes( '/wp-json/amp/v1/options' ), { timeout: 10000 } ), + ] ); } it( 'is present on the page', async () => { @@ -161,21 +188,28 @@ describe( 'AMP settings screen Review panel', () => { } ); it( 'button redirects to an AMP page in transitional mode', async () => { - await clickBrowseSiteInTemplateMode( 'transitional' ); + await changeAndSaveTemplateMode( 'transitional' ); + + await expect( page ).toClick( 'a', { text: 'Browse Site' } ); + await page.waitForNavigation(); await page.waitForSelector( 'html[amp]' ); await expect( page ).toMatchElement( 'html[amp]' ); } ); it( 'button redirects to an AMP page in reader mode', async () => { - await clickBrowseSiteInTemplateMode( 'reader' ); + await expect( page ).toClick( 'a', { text: 'Browse Site' } ); + await page.waitForNavigation(); await page.waitForSelector( 'html[amp]' ); await expect( page ).toMatchElement( 'html[amp]' ); } ); it( 'button redirects to an AMP page in standard mode', async () => { - await clickBrowseSiteInTemplateMode( 'standard' ); + await changeAndSaveTemplateMode( 'standard' ); + + await expect( page ).toClick( 'a', { text: 'Browse Site' } ); + await page.waitForNavigation(); await page.waitForSelector( 'html[amp]' ); await expect( page ).toMatchElement( 'html[amp]' ); @@ -195,11 +229,7 @@ describe( 'AMP settings screen Review panel', () => { await page.waitForSelector( '#amp-settings-root' ); await expect( page ).not.toMatchElement( '.settings-site-review' ); - await clickMode( 'standard' ); - - await expect( page ).toClick( 'button', { text: 'Save' } ); - await page.waitForSelector( '.amp-save-success-notice' ); - await expect( page ).toMatchElement( '.amp-save-success-notice', { text: 'Saved' } ); + await changeAndSaveTemplateMode( 'standard' ); await page.waitForSelector( '.settings-site-review' ); await expect( page ).toMatchElement( 'h2', { text: 'Review' } ); From 284e48955adea0e38c08aefdd0eee50ffbff8191 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 21 Oct 2021 07:20:08 -0700 Subject: [PATCH 44/44] Harden check for post being set Co-authored-by: Piotr Delawski --- tests/e2e/specs/admin/amp-options.js | 2 +- tests/e2e/specs/amp-onboarding/done.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/e2e/specs/admin/amp-options.js b/tests/e2e/specs/admin/amp-options.js index 3f55054a9d9..b2b93e8aa55 100644 --- a/tests/e2e/specs/admin/amp-options.js +++ b/tests/e2e/specs/admin/amp-options.js @@ -152,7 +152,7 @@ describe( 'AMP settings screen Review panel', () => { afterAll( async () => { await visitAdminPage( 'admin.php', 'page=amp-options' ); - if ( testPost.id ) { + if ( testPost?.id ) { await page.evaluate( ( id ) => wp.apiFetch( { path: `/wp/v2/posts/${ id }`, method: 'DELETE', diff --git a/tests/e2e/specs/amp-onboarding/done.js b/tests/e2e/specs/amp-onboarding/done.js index f4b858c4d91..c80fb1d2d0d 100644 --- a/tests/e2e/specs/amp-onboarding/done.js +++ b/tests/e2e/specs/amp-onboarding/done.js @@ -55,14 +55,14 @@ describe( 'Done', () => { afterAll( async () => { await visitAdminPage( 'admin.php', 'page=amp-options' ); - if ( testPost.id ) { + if ( testPost?.id ) { await page.evaluate( ( id ) => wp.apiFetch( { path: `/wp/v2/posts/${ id }`, method: 'DELETE', data: { force: true }, } ), testPost.id ); } - if ( testPage.id ) { + if ( testPage?.id ) { await page.evaluate( ( id ) => wp.apiFetch( { path: `/wp/v2/pages/${ id }`, method: 'DELETE',