From 717ce3616370ac509e72645031788b9cea29331a Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 31 Aug 2020 17:28:11 -0500 Subject: [PATCH 01/41] Refactor URL validation CLI script --- .../cli/class-amp-cli-validation-command.php | 604 +++++------------- src/Validation/URLValidationProvider.php | 218 +++++++ src/Validation/ValidationURLProvider.php | 307 +++++++++ .../src/Helpers/ValidationRequestMocking.php | 63 ++ .../Validation/URLValidationProviderTest.php | 96 +++ .../Validation/ValidationURLProviderTest.php | 386 +++++++++++ .../test-class-amp-cli-validation-command.php | 464 +------------- 7 files changed, 1234 insertions(+), 904 deletions(-) create mode 100644 src/Validation/URLValidationProvider.php create mode 100644 src/Validation/ValidationURLProvider.php create mode 100644 tests/php/src/Helpers/ValidationRequestMocking.php create mode 100644 tests/php/src/Validation/URLValidationProviderTest.php create mode 100644 tests/php/src/Validation/ValidationURLProviderTest.php diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 61b1b39719b..dddcb038afd 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -9,6 +9,8 @@ use AmpProject\AmpWP\Admin\ReaderThemes; use AmpProject\AmpWP\Option; +use AmpProject\AmpWP\Validation\URLValidationProvider; +use AmpProject\AmpWP\Validation\ValidationURLProvider; /** * Crawls the site for validation errors or resets the stored validation errors. @@ -60,76 +62,25 @@ final class AMP_CLI_Validation_Command { public $wp_cli_progress; /** - * The total number of validation errors, regardless of whether they were accepted. + * URLValidationProvider instance. * - * @var int + * @var URLValidationProvider */ - public $total_errors = 0; + private $validation_provider; /** - * The total number of unaccepted validation errors. + * ValidationURLProvider instance. * - * If an error has been accepted in the /wp-admin validation UI, - * it won't count toward this. - * - * @var int - */ - public $unaccepted_errors = 0; - - /** - * The number of URLs crawled, regardless of whether they have validation errors. - * - * @var int + * @var ValidationURLProvider */ - public $number_crawled = 0; + private $validation_url_provider; /** - * Whether to force crawling of URLs. - * - * By default, this script only crawls URLs that support AMP, - * where the user has not opted-out of AMP for the URL. - * For example, by un-checking 'Posts' in 'AMP Settings' > 'Supported Templates'. - * Or un-checking 'Enable AMP' in the post's editor. - * - * @var bool - */ - public $force_crawl_urls = false; - - /** - * An allowlist of conditionals to use for validation. - * - * Usually, this script will validate all of the templates that don't have AMP disabled. - * But this allows validating based on only these conditionals. - * This is set if the WP-CLI command has an --include argument. + * Associative args passed to the command. * * @var array */ - public $include_conditionals = []; - - /** - * The maximum number of URLs to validate for each type. - * - * Templates are each a separate type, like those for is_category() and is_tag(). - * Also, each post type is a separate type. - * This value is overridden if the WP-CLI command has an --limit argument, like --limit=10. - * - * @var int - */ - public $limit_type_validate_count; - - /** - * The validation counts by type, like template or post type. - * - * @var array[] { - * Validity by type. - * - * @type array $type { - * @type int $valid The number of valid URLs for this type. - * @type int $total The total number of URLs for this type, valid or invalid. - * } - * } - */ - public $validity_by_type = []; + private $assoc_args; /** * Crawl the entire site to get AMP validation results. @@ -157,47 +108,12 @@ final class AMP_CLI_Validation_Command { * @throws Exception If an error happens. */ public function run( $args, $assoc_args ) { - $this->include_conditionals = []; - $this->force_crawl_urls = false; - $this->limit_type_validate_count = (int) $assoc_args[ self::LIMIT_URLS_ARGUMENT ]; - - /* - * Handle the argument and flag passed to the command: --include and --force. - * If the self::INCLUDE_ARGUMENT is present, force crawling or URLs. - * The WP-CLI command should indicate which templates are crawled, not the /wp-admin options. - */ - if ( ! empty( $assoc_args[ self::INCLUDE_ARGUMENT ] ) ) { - $this->include_conditionals = explode( ',', $assoc_args[ self::INCLUDE_ARGUMENT ] ); - $this->force_crawl_urls = true; - } elseif ( isset( $assoc_args[ self::FLAG_NAME_FORCE_VALIDATION ] ) ) { - $this->force_crawl_urls = true; - } - - // 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( $this->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 ) ) ); - } + $this->assoc_args = $assoc_args; - if ( empty( $this->include_conditionals ) ) { - $this->include_conditionals = $allowed_templates; - } - } + $validation_url_provider = $this->get_validation_url_provider(); + $validation_provider = $this->get_validation_provider(); - $number_urls_to_crawl = $this->count_urls_to_validate(); + $number_urls_to_crawl = count( $validation_url_provider->get_urls() ); if ( ! $number_urls_to_crawl ) { if ( ! empty( $this->include_conditionals ) ) { WP_CLI::error( @@ -230,7 +146,7 @@ public function run( $args, $assoc_args ) { $key_validity_rate = 'Validity Rate'; $table_validation_by_type = []; - foreach ( $this->validity_by_type as $type_name => $validity ) { + foreach ( $validation_provider->validity_by_type as $type_name => $validity ) { $table_validation_by_type[] = [ $key_template_type => $type_name, $key_url_count => $validity['total'], @@ -246,9 +162,9 @@ public function run( $args, $assoc_args ) { 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.', - $this->number_crawled, - $this->total_errors, - $this->unaccepted_errors + $validation_provider->number_crawled, + $validation_provider->total_errors, + $validation_provider->unaccepted_errors ) ); @@ -267,6 +183,142 @@ public function run( $args, $assoc_args ) { WP_CLI::line( sprintf( 'For more details, please see: %s', $url_more_details ) ); } + /** + * Provides the associative args for the command. + * + * @return array + */ + private function get_assoc_args() { + if ( ! is_array( $this->assoc_args ) ) { + $this->assoc_args = []; + } + + return array_merge( + [ + self::LIMIT_URLS_ARGUMENT => 100, + self::INCLUDE_ARGUMENT => [], + self::FLAG_NAME_FORCE_VALIDATION => false, + ], + $this->assoc_args + ); + } + + /** + * Provides the ValidationURLProvider instance. + * + * @return ValidationURLProvider + */ + private function get_validation_url_provider() { + if ( ! is_null( $this->validation_url_provider ) ) { + return $this->validation_url_provider; + } + + $assoc_args = $this->get_assoc_args(); + + $include_conditionals = []; + $force_crawl_urls = false; + $limit_type_validate_count = (int) $assoc_args[ self::LIMIT_URLS_ARGUMENT ]; + + /* + * Handle the argument and flag passed to the command: --include and --force. + * If the self::INCLUDE_ARGUMENT is present, force crawling or URLs. + * The WP-CLI command should indicate which templates are crawled, not the /wp-admin options. + */ + if ( ! empty( $assoc_args[ self::INCLUDE_ARGUMENT ] ) ) { + $include_conditionals = explode( ',', $assoc_args[ self::INCLUDE_ARGUMENT ] ); + $force_crawl_urls = true; + } elseif ( isset( $assoc_args[ self::FLAG_NAME_FORCE_VALIDATION ] ) ) { + $force_crawl_urls = true; + } + + // 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->validation_url_provider = new ValidationURLProvider( + $limit_type_validate_count, + $include_conditionals, + $force_crawl_urls + ); + + return $this->validation_url_provider; + } + + /** + * Provides the site scan instance. + * + * @return URLValidationProvider + */ + private function get_validation_provider() { + if ( ! is_null( $this->validation_provider ) ) { + return $this->validation_provider; + } + + $this->validation_provider = new URLValidationProvider(); + + return $this->validation_provider; + } + + /** + * Validates the URLs of the entire site. + * + * Includes the URLs of public, published posts, public taxonomies, and other templates. + * This validates one of each type at a time, + * and iterates until it reaches the maximum number of URLs for each type. + */ + private function crawl_site() { + $validation_url_provider = $this->get_validation_url_provider(); + $validation_provider = $this->get_validation_provider(); + + $result = $validation_provider->with_lock( + function() use ( $validation_url_provider, $validation_provider ) { + foreach ( $validation_url_provider->get_urls() as $url ) { + $validity = $validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); + + if ( $this->wp_cli_progress ) { + $this->wp_cli_progress->tick(); + } + + if ( is_wp_error( $validity['error'] ) ) { + WP_CLI::warning( + sprintf( + 'Validate URL error (%1$s): %2$s URL: %3$s', + $validity['error']->get_error_code(), + $validity['error']->get_error_message(), + $url + ) + ); + } + } + } + ); + + if ( is_wp_error( $result ) ) { + WP_CLI::error( 'The site cannot be crawled at this time because validation is running in another process.' ); + return; + } + } + /** * Reset all validation data on a site. * @@ -396,352 +448,4 @@ public function check_url( $args ) { WP_CLI::line( wp_json_encode( $result, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES ) ); } - - /** - * Gets the total number of URLs to validate. - * - * By default, this only counts AMP-enabled posts and terms. - * But if $force_crawl_urls is true, it counts all of them, regardless of their AMP status. - * It also uses $this->maximum_urls_to_validate_for_each_type, - * which can be overridden with a command line argument. - * - * @return int The number of URLs to validate. - */ - private function count_urls_to_validate() { - /* - * If the homepage is set to 'Your latest posts,' start the $total_count at 1. - * Otherwise, it will probably be counted in the query for pages below. - */ - $total_count = 'posts' === get_option( 'show_on_front' ) && $this->is_template_supported( 'is_home' ) ? 1 : 0; - - $amp_enabled_taxonomies = array_filter( - get_taxonomies( [ 'public' => true ] ), - [ $this, 'does_taxonomy_support_amp' ] - ); - - // Count all public taxonomy terms. - foreach ( $amp_enabled_taxonomies as $taxonomy ) { - $term_query = new WP_Term_Query( - [ - 'taxonomy' => $taxonomy, - 'fields' => 'ids', - 'number' => $this->limit_type_validate_count, - ] - ); - - // If $term_query->terms is an empty array, passing it to count() will throw an error. - $total_count += ! empty( $term_query->terms ) ? count( $term_query->terms ) : 0; - } - - // Count posts by type, like post, page, attachment, etc. - $public_post_types = get_post_types( [ 'public' => true ], 'names' ); - foreach ( $public_post_types as $post_type ) { - $posts = $this->get_posts_that_support_amp( $this->get_posts_by_type( $post_type ) ); - $total_count += ! empty( $posts ) ? count( $posts ) : 0; - } - - // Count author pages, like https://example.com/author/admin/. - $total_count += count( $this->get_author_page_urls() ); - - // Count a single example date page, like https://example.com/?year=2019. - if ( $this->get_date_page() ) { - $total_count++; - } - - // Count a single example search page, like https://example.com/?s=example. - if ( $this->get_search_page() ) { - $total_count++; - } - - return $total_count; - } - - /** - * Gets the posts IDs that support AMP. - * - * By default, this only gets the post IDs if they support AMP. - * This means that 'Posts' isn't deselected in 'AMP Settings' > 'Supported Templates'. - * And 'Enable AMP' isn't unchecked in the post's editor. - * But if $force_crawl_urls is true, this simply returns all of the IDs. - * - * @param array $ids THe post IDs to check for AMP support. - * @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 []; - } - - if ( $this->force_crawl_urls ) { - return $ids; - } - - return array_filter( - $ids, - 'amp_is_post_supported' - ); - } - - /** - * Gets whether the taxonomy supports AMP. - * - * This only gets the term IDs if they support AMP. - * If their taxonomy is unchecked in 'AMP Settings' > 'Supported Templates,' this does not return them. - * For example, if 'Categories' is unchecked. - * This can be overridden by passing the self::FLAG_NAME_FORCE_VALIDATION argument to the WP-CLI command. - * - * @param string $taxonomy The taxonomy. - * @return boolean Whether the taxonomy supports AMP. - */ - private function does_taxonomy_support_amp( $taxonomy ) { - if ( 'post_tag' === $taxonomy ) { - $taxonomy = 'tag'; - } - $taxonomy_key = 'is_' . $taxonomy; - $custom_taxonomy_key = sprintf( 'is_tax[%s]', $taxonomy ); - return $this->is_template_supported( $taxonomy_key ) || $this->is_template_supported( $custom_taxonomy_key ); - } - - /** - * Gets whether the template is supported. - * - * If the user has passed an include argument to the WP-CLI command, use that to find if this template supports AMP. - * For example, wp amp validation run --include=is_tag,is_category - * would return true only if is_tag() or is_category(). - * But passing the self::FLAG_NAME_FORCE_VALIDATION argument to the WP-CLI command overrides this. - * - * @param string $template The template to check. - * @return bool Whether the template is supported. - */ - private function is_template_supported( $template ) { - // If the --include argument is present in the WP-CLI command, this template conditional must be present in it. - if ( ! empty( $this->include_conditionals ) ) { - return in_array( $template, $this->include_conditionals, true ); - } - if ( $this->force_crawl_urls ) { - return true; - } - - $supportable_templates = AMP_Theme_Support::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'] ); - } - - /** - * Gets the IDs of public, published posts. - * - * @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 ) { - $args = [ - 'post_type' => $post_type, - 'posts_per_page' => is_int( $number ) ? $number : $this->limit_type_validate_count, - 'post_status' => 'publish', - 'orderby' => 'ID', - 'order' => 'DESC', - 'fields' => 'ids', - ]; - if ( is_int( $offset ) ) { - $args['offset'] = $offset; - } - - // Attachment posts usually have the post_status of 'inherit,' so they can use the status of the post they're attached to. - if ( 'attachment' === $post_type ) { - $args['post_status'] = 'inherit'; - } - $query = new WP_Query( $args ); - - return $query->posts; - } - - /** - * 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). - * @return string[] The term links, as an array of strings. - */ - private function get_taxonomy_links( $taxonomy, $offset = '', $number = 1 ) { - return array_map( - 'get_term_link', - get_terms( - array_merge( - compact( 'taxonomy', 'offset', 'number' ), - [ - 'orderby' => 'id', - ] - ) - ) - ); - } - - /** - * Gets the author page URLs, like https://example.com/author/admin/. - * - * 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. - * @return array The author page URLs, or an empty array. - */ - 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->limit_type_validate_count; - foreach ( get_users( compact( 'offset', 'number' ) ) as $author ) { - $author_page_urls[] = get_author_posts_url( $author->ID, $author->user_nicename ); - } - - return $author_page_urls; - } - - /** - * Gets a single search page URL, like https://example.com/?s=example. - * - * @return string|null An example search page, or null. - */ - private function get_search_page() { - if ( ! $this->is_template_supported( 'is_search' ) ) { - return null; - } - - return add_query_arg( 's', 'example', home_url( '/' ) ); - } - - /** - * Gets a single date page URL, like https://example.com/?year=2018. - * - * @return string|null An example search page, or null. - */ - private function get_date_page() { - if ( ! $this->is_template_supported( 'is_date' ) ) { - return null; - } - - return add_query_arg( 'year', gmdate( 'Y' ), home_url( '/' ) ); - } - - /** - * Validates the URLs of the entire site. - * - * Includes the URLs of public, published posts, public taxonomies, and other templates. - * This validates one of each type at a time, - * and iterates until it reaches the maximum number of URLs for each type. - */ - private function crawl_site() { - /* - * If 'Your homepage displays' is set to 'Your latest posts', validate the homepage. - * It will not be part of the page validation below. - */ - if ( 'posts' === get_option( 'show_on_front' ) && $this->is_template_supported( 'is_home' ) ) { - $this->validate_and_store_url( home_url( '/' ), 'home' ); - } - - $amp_enabled_taxonomies = array_filter( - get_taxonomies( [ 'public' => true ] ), - [ $this, 'does_taxonomy_support_amp' ] - ); - $public_post_types = get_post_types( [ 'public' => true ], 'names' ); - - // Validate one URL of each template/content type, then another URL of each type on the next iteration. - for ( $i = 0; $i < $this->limit_type_validate_count; $i++ ) { - // Validate 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] ) ) { - $this->validate_and_store_url( get_permalink( $post_ids[0] ), $post_type ); - } - } - - foreach ( $amp_enabled_taxonomies as $taxonomy ) { - $taxonomy_links = $this->get_taxonomy_links( $taxonomy, $i, 1 ); - $link = reset( $taxonomy_links ); - if ( ! empty( $link ) ) { - $this->validate_and_store_url( $link, $taxonomy ); - } - } - - $author_page_urls = $this->get_author_page_urls( $i, 1 ); - if ( ! empty( $author_page_urls[0] ) ) { - $this->validate_and_store_url( $author_page_urls[0], 'author' ); - } - } - - // Only validate 1 date and 1 search page. - $url = $this->get_date_page(); - if ( $url ) { - $this->validate_and_store_url( $url, 'date' ); - } - $url = $this->get_search_page(); - if ( $url ) { - $this->validate_and_store_url( $url, 'search' ); - } - } - - /** - * Validates the URL, stores the results, and increments the counts. - * - * @param string $url The URL to validate. - * @param string $type The type of template, post, or taxonomy. - */ - private function validate_and_store_url( $url, $type ) { - $validity = AMP_Validation_Manager::validate_url_and_store( $url ); - - /* - * If the request to validate this returns a WP_Error, return. - * One cause of an error is if the validation request results in a 404 response code. - */ - if ( is_wp_error( $validity ) ) { - WP_CLI::warning( sprintf( 'Validate URL error (%1$s): %2$s URL: %3$s', $validity->get_error_code(), $validity->get_error_message(), $url ) ); - return; - } - if ( $this->wp_cli_progress ) { - $this->wp_cli_progress->tick(); - } - - $validation_errors = wp_list_pluck( $validity['results'], 'error' ); - $unaccepted_error_count = count( - array_filter( - $validation_errors, - static function( $error ) { - $validation_status = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $error ); - return ( - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS !== $validation_status['term_status'] - && - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS !== $validation_status['term_status'] - ); - } - ) - ); - - if ( count( $validation_errors ) > 0 ) { - $this->total_errors++; - } - if ( $unaccepted_error_count > 0 ) { - $this->unaccepted_errors++; - } - - $this->number_crawled++; - - if ( ! isset( $this->validity_by_type[ $type ] ) ) { - $this->validity_by_type[ $type ] = [ - 'valid' => 0, - 'total' => 0, - ]; - } - $this->validity_by_type[ $type ]['total']++; - if ( 0 === $unaccepted_error_count ) { - $this->validity_by_type[ $type ]['valid']++; - } - } } diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php new file mode 100644 index 00000000000..867eb745ffe --- /dev/null +++ b/src/Validation/URLValidationProvider.php @@ -0,0 +1,218 @@ +get_lock_timeout() ); + } + + /** + * Unlocks validation. + */ + private function unlock() { + delete_transient( self::LOCK_TRANSIENT ); + } + + /** + * Returns whether validation is currently locked. + * + * @return boolean + */ + public function is_locked() { + return get_transient( self::LOCK_TRANSIENT ); + } + + /** + * Provides the length of time, in seconds, to lock validation when this runs. + * + * @return int + */ + private function get_lock_timeout() { + /** + * Filters the length of time to lock URL validation when a process starts. + * + * @param int $timeout Time in seconds. Default 300 seconds. + */ + return apply_filters( 'amp_validation_lock_timeout', 5 * MINUTE_IN_SECONDS ); + } + + /** + * Runs a callback with a lock set for the duration of the callback. + * + * @param callable $callback Callback to run with the lock set. + * @return mixed WP_Error if a lock is in place. Otherwise, the result of the callback or void if it doesn't return anything. + */ + public function with_lock( $callback ) { + if ( $this->is_locked() ) { + return new WP_Error( + 'amp_url_validation_locked', + __( 'URL validation cannot start right now because another process is already validating URLs. Try again in a few minutes.', 'amp' ) + ); + } + + $this->lock(); + $result = call_user_func( $callback ); + $this->unlock(); + + return $result; + } + + /** + * Validates a URL, stores the results, and increments the counts. + * + * @param string $url The URL to validate. + * @param string $type The type of template, post, or taxonomy. + * @param string $flag Flag determining whether the URL should be revalidated. + * @return array Associative array containing validity results along with error info and whether the URL was revalidated. + */ + public function get_url_validation( $url, $type, $flag = null ) { + $validity = null; + $revalidated = true; + $error = null; + + if ( self::FLAG_FORCE_REVALIDATE !== $flag ) { + $url_post = AMP_Validated_URL_Post_Type::get_invalid_url_post( $url ); + + if ( $url_post && empty( AMP_Validated_URL_Post_Type::get_post_staleness( $url_post ) ) ) { + $validity = AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( $url_post ); + $revalidated = false; + } + } + + if ( self::FLAG_NO_REVALIDATE !== $flag && ( is_null( $validity ) || self::FLAG_FORCE_REVALIDATE === $flag ) ) { + $validity = AMP_Validation_Manager::validate_url_and_store( $url ); + } + + if ( is_wp_error( $validity ) ) { + $validity = null; + $error = $validity; + } elseif ( $validity && isset( $validity['results'] ) ) { + $this->update_state_from_validity( $validity, $type ); + } + + return compact( 'validity', 'revalidated', 'error' ); + } + + /** + * Increments crawl counts from a validation result. + * + * @param array $validity Validity results. + * @param string $type The URL type. + */ + private function update_state_from_validity( $validity, $type ) { + $validation_errors = wp_list_pluck( $validity['results'], 'error' ); + $unaccepted_error_count = count( + array_filter( + $validation_errors, + static function( $error ) { + $validation_status = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $error ); + return ( + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS !== $validation_status['term_status'] + && + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS !== $validation_status['term_status'] + ); + } + ) + ); + + if ( count( $validation_errors ) > 0 ) { + $this->total_errors++; + } + if ( $unaccepted_error_count > 0 ) { + $this->unaccepted_errors++; + } + + $this->number_crawled++; + + if ( ! isset( $this->validity_by_type[ $type ] ) ) { + $this->validity_by_type[ $type ] = [ + 'valid' => 0, + 'total' => 0, + ]; + } + $this->validity_by_type[ $type ]['total']++; + if ( 0 === $unaccepted_error_count ) { + $this->validity_by_type[ $type ]['valid']++; + } + } +} diff --git a/src/Validation/ValidationURLProvider.php b/src/Validation/ValidationURLProvider.php new file mode 100644 index 00000000000..96fd7a0aa13 --- /dev/null +++ b/src/Validation/ValidationURLProvider.php @@ -0,0 +1,307 @@ +limit_per_type = $limit_per_type; + $this->include_conditionals = $include_conditionals; + $this->include_unsupported = $include_unsupported; + } + + /** + * Provides the array of URLs to check. Each URL is an array with two elements, with the URL at index 0 and the type at index 1. + * + * @param int|null $offset The number of URLs to offset by, where applicable. + * @return array Array of URLs and types. + */ + public function get_urls( $offset = 0 ) { + $urls = []; + + /* + * If 'Your homepage displays' is set to 'Your latest posts', include the homepage. + */ + if ( 'posts' === get_option( 'show_on_front' ) && $this->is_template_supported( 'is_home' ) ) { + $urls[] = [ + 'url' => home_url( '/' ), + 'type' => 'home', + ]; + } + + $amp_enabled_taxonomies = array_filter( + get_taxonomies( [ 'public' => true ] ), + [ $this, 'does_taxonomy_support_amp' ] + ); + $public_post_types = get_post_types( [ 'public' => true ], 'names' ); + + // Include one URL of each template/content type, then another URL of each type on the next iteration. + for ( $i = $offset; $i < $this->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] ) ) { + $urls[] = [ + 'url' => get_permalink( $post_ids[0] ), + 'type' => $post_type, + ]; + } + } + + foreach ( $amp_enabled_taxonomies as $taxonomy ) { + $taxonomy_links = $this->get_taxonomy_links( $taxonomy, $i, 1 ); + $link = reset( $taxonomy_links ); + if ( ! empty( $link ) ) { + $urls[] = [ + 'url' => $link, + 'type' => $taxonomy, + ]; + } + } + + $author_page_urls = $this->get_author_page_urls( $i, 1 ); + if ( ! empty( $author_page_urls[0] ) ) { + $urls[] = [ + 'url' => $author_page_urls[0], + 'type' => 'author', + ]; + } + } + + // Only validate 1 date and 1 search page. + $url = $this->get_date_page(); + if ( $url ) { + $urls[] = [ + 'url' => $url, + 'type' => 'date', + ]; + } + $url = $this->get_search_page(); + if ( $url ) { + $urls[] = [ + 'url' => $url, + 'type' => 'search', + ]; + } + + return $urls; + } + + /** + * Gets whether the template is supported. + * + * @param string $template The template to check. + * @return bool Whether the template is supported. + */ + public function is_template_supported( $template ) { + // If the --include argument is present in the WP-CLI command, this template conditional must be present in it. + if ( ! empty( $this->include_conditionals ) ) { + return in_array( $template, $this->include_conditionals, true ); + } + if ( $this->include_unsupported ) { + return true; + } + + $supportable_templates = AMP_Theme_Support::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'] ); + } + + /** + * Gets the posts IDs that support AMP. + * + * By default, this only gets the post IDs if they support AMP. + * This means that 'Posts' isn't deselected in 'AMP Settings' > 'Supported Templates'. + * And 'Enable AMP' isn't unchecked in the post's editor. + * + * @param array $ids The post IDs to check for AMP support. + * @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 []; + } + + if ( $this->include_unsupported ) { + return $ids; + } + + return array_filter( + $ids, + 'post_supports_amp' + ); + } + + /** + * Gets the IDs of public, published posts. + * + * @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 ) { + $args = [ + 'post_type' => $post_type, + 'posts_per_page' => is_int( $number ) ? $number : $this->limit_per_type, + 'post_status' => 'publish', + 'orderby' => 'ID', + 'order' => 'DESC', + 'fields' => 'ids', + ]; + if ( is_int( $offset ) ) { + $args['offset'] = $offset; + } + + // Attachment posts usually have the post_status of 'inherit,' so they can use the status of the post they're attached to. + if ( 'attachment' === $post_type ) { + $args['post_status'] = 'inherit'; + } + $query = new WP_Query( $args ); + + return $query->posts; + } + + /** + * Gets the author page URLs, like https://example.com/author/admin/. + * + * 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. + * @return array The author page URLs, or an empty array. + */ + 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->limit_per_type; + foreach ( get_users( compact( 'offset', 'number' ) ) as $author ) { + $author_page_urls[] = get_author_posts_url( $author->ID, $author->user_nicename ); + } + + return $author_page_urls; + } + + /** + * Gets a single search page URL, like https://example.com/?s=example. + * + * @return string|null An example search page, or null. + */ + private function get_search_page() { + if ( ! $this->is_template_supported( 'is_search' ) ) { + return null; + } + + return add_query_arg( 's', 'example', home_url( '/' ) ); + } + + /** + * Gets a single date page URL, like https://example.com/?year=2018. + * + * @return string|null An example search page, or null. + */ + private function get_date_page() { + if ( ! $this->is_template_supported( 'is_date' ) ) { + return null; + } + + return add_query_arg( 'year', gmdate( 'Y' ), home_url( '/' ) ); + } + + /** + * Gets whether the taxonomy supports AMP. + * + * This only gets the term IDs if they support AMP. + * If their taxonomy is unchecked in 'AMP Settings' > 'Supported Templates,' this does not return them. + * For example, if 'Categories' is unchecked. + * + * @param string $taxonomy The taxonomy. + * @return boolean Whether the taxonomy supports AMP. + */ + private function does_taxonomy_support_amp( $taxonomy ) { + if ( 'post_tag' === $taxonomy ) { + $taxonomy = 'tag'; + } + $taxonomy_key = 'is_' . $taxonomy; + $custom_taxonomy_key = sprintf( 'is_tax[%s]', $taxonomy ); + return $this->is_template_supported( $taxonomy_key ) || $this->is_template_supported( $custom_taxonomy_key ); + } + + /** + * 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). + * @return string[] The term links, as an array of strings. + */ + private function get_taxonomy_links( $taxonomy, $offset = '', $number = 1 ) { + return array_map( + 'get_term_link', + get_terms( + array_merge( + compact( 'taxonomy', 'offset', 'number' ), + [ + 'orderby' => 'id', + ] + ) + ) + ); + } +} diff --git a/tests/php/src/Helpers/ValidationRequestMocking.php b/tests/php/src/Helpers/ValidationRequestMocking.php new file mode 100644 index 00000000000..b3c65bf0cf2 --- /dev/null +++ b/tests/php/src/Helpers/ValidationRequestMocking.php @@ -0,0 +1,63 @@ + AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, + 'posts_per_page' => 100, + 'fields' => 'ids', + ] + ); + + return array_map( + static function( $post ) { + return remove_query_arg( 'amp', AMP_Validated_URL_Post_Type::get_url_from_post( $post ) ); + }, + $query->posts + ); + } + + /** + * Construct a WP HTTP response for a validation request. + * + * @return array The response. + */ + public static function get_validate_response() { + $mock_validation = [ + 'results' => [ + [ + 'error' => [ + 'code' => 'foo', + ], + 'sanitized' => false, + ], + ], + 'url' => home_url( '/' ), + ]; + + return [ + 'body' => wp_json_encode( $mock_validation ), + 'response' => [ + 'code' => 200, + 'message' => 'ok', + ], + ]; + } +} diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php new file mode 100644 index 00000000000..ad46cf4d7b6 --- /dev/null +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -0,0 +1,96 @@ +validation_provider = new URLValidationProvider( 100 ); + add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); + } + + /** + * Test get_url_validation. + * + * @covers ::get_url_validation() + */ + public function test_get_url_validation() { + $single_post_permalink = get_permalink( self::factory()->post->create() ); + $this->call_private_method( $this->validation_provider, 'get_url_validation', [ $single_post_permalink, 'post' ] ); + $this->assertTrue( in_array( $single_post_permalink, ValidationRequestMocking::get_validated_urls(), true ) ); + + $number_of_posts = 30; + $post_permalinks = []; + + for ( $i = 0; $i < $number_of_posts; $i++ ) { + $permalink = get_permalink( self::factory()->post->create() ); + $post_permalinks[] = $permalink; + $this->call_private_method( $this->validation_provider, 'get_url_validation', [ $permalink, 'post' ] ); + } + + // All of the posts created should be present in the validated URLs. + $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); + + $this->assertEquals( 31, $this->validation_provider->total_errors ); + $this->assertEmpty( $this->validation_provider->unaccepted_errors ); + $this->assertEquals( 31, $this->validation_provider->number_crawled ); + + $this->assertEquals( + [ 'post' ], + array_keys( $this->validation_provider->validity_by_type ) + ); + } + + /** + * Tests locking and unlocking. + * + * @covers ::lock + * @covers ::unlock + * @covers ::is_locked + * @covers ::get_lock_timeout + * @covers ::with_lock + */ + public function test_locking() { + $this->assertFalse( get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + + $expected_result = 'EXPECTED RESULT'; + $result = $this->validation_provider->with_lock( + function() use ( $expected_result ) { + $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + + // Expect an error when lock is already in place. + $this->assertWPError( $this->validation_provider->with_lock( '__return_empty_string' ) ); + + return $expected_result; + } + ); + + $this->assertEquals( $expected_result, $result ); + $this->assertFalse( get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + + // Test with_lock with no return value. + $this->assertNull( + $this->validation_provider->with_lock( + function() { + $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + return; + } + ) + ); + $this->assertFalse( get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + } +} diff --git a/tests/php/src/Validation/ValidationURLProviderTest.php b/tests/php/src/Validation/ValidationURLProviderTest.php new file mode 100644 index 00000000000..6e9c082c2f4 --- /dev/null +++ b/tests/php/src/Validation/ValidationURLProviderTest.php @@ -0,0 +1,386 @@ +validation_url_provider = new ValidationURLProvider(); + add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); + } + + /** + * Test retreival of urls. + * + * @covers ::get_urls() + */ + public function test_count_urls_to_validate() { + $this->validation_url_provider = new ValidationURLProvider(); + + $number_original_urls = 4; + + $this->assertEquals( $number_original_urls, count( $this->validation_url_provider->get_urls() ) ); + + $this->validation_url_provider = new ValidationURLProvider( 100 ); + + $category = self::factory()->term->create( [ 'taxonomy' => 'category' ] ); + $number_new_posts = 50; + $post_ids = []; + for ( $i = 0; $i < $number_new_posts; $i++ ) { + $post_ids[] = self::factory()->post->create( + [ + 'tax_input' => [ 'category' => $category ], + ] + ); + } + + /* + * 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; + $this->assertEquals( $expected_url_count, count( $this->validation_url_provider->get_urls() ) ); + + $this->validation_url_provider = new ValidationURLProvider( 100 ); + + $number_of_new_terms = 20; + $expected_url_count += $number_of_new_terms; + $taxonomy = 'category'; + $terms_for_current_taxonomy = []; + for ( $i = 0; $i < $number_of_new_terms; $i++ ) { + $terms_for_current_taxonomy[] = self::factory()->term->create( + [ + 'taxonomy' => $taxonomy, + ] + ); + } + + // Terms need to be associated with a post in order to be returned in get_terms(). + wp_set_post_terms( + $post_ids[0], + $terms_for_current_taxonomy, + $taxonomy + ); + + $this->assertEquals( $expected_url_count, count( $this->validation_url_provider->get_urls() ) ); + } + + /** + * Test get_posts_that_support_amp. + * + * @covers ::get_posts_that_support_amp() + */ + public function test_get_posts_that_support_amp() { + $number_of_posts = 20; + $ids = []; + for ( $i = 0; $i < $number_of_posts; $i++ ) { + $ids[] = self::factory()->post->create(); + } + + // This should count all of the newly-created posts as supporting AMP. + $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + + // Simulate 'Enable AMP' being unchecked in the post editor, in which case get_url_count() should not count it. + $first_id = $ids[0]; + update_post_meta( + $first_id, + AMP_Post_Meta_Box::STATUS_POST_META_KEY, + AMP_Post_Meta_Box::DISABLED_STATUS + ); + $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ [ $first_id ] ] ) ); + + update_post_meta( + $first_id, + AMP_Post_Meta_Box::STATUS_POST_META_KEY, + 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->validation_url_provider->include_unsupported = true; + $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + $this->validation_url_provider->include_unsupported = false; + + // 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 ); + $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + + // In Transitional Mode, the IDs should also include all of the newly-created posts. + add_theme_support( + AMP_Theme_Support::SLUG, + [ + AMP_Theme_Support::PAIRED_FLAG => true, + ] + ); + $this->assertEquals( $ids, $this->call_private_method( $this->validation_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->validation_url_provider->include_conditionals = [ 'is_tag', 'is_category' ]; + $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + + /* + * If is_singular is in the WP-CLI argument, it should allow return these posts as being AMP-enabled. + * For example, wp amp validate-site include=is_singular,is_category + */ + $this->validation_url_provider->include_conditionals = [ 'is_singular', 'is_category' ]; + $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ) ); + $this->validation_url_provider->include_conditionals = []; + } + + /** + * Test get_author_page_urls. + * + * @covers ::get_author_page_urls() + */ + public function test_get_author_page_urls() { + self::factory()->user->create(); + $users = get_users(); + $first_author = $users[0]; + $first_author_url = get_author_posts_url( $first_author->ID, $first_author->user_nicename ); + $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->validation_url_provider, 'get_author_page_urls', [ 0, 1 ] ); + + // Passing 0 as the offset argument should get the first author. + $this->assertEquals( [ $first_author_url ], $actual_urls ); + + $actual_urls = $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls', [ 1, 1 ] ); + + // Passing 1 as the offset argument should get the second author. + $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->validation_url_provider->include_conditionals = [ 'is_category' ]; + $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls' ) ); + + // If $include_conditionals is set and has is_author, this should return URLs. + $this->validation_url_provider->include_conditionals = [ 'is_author' ]; + $this->assertEquals( + [ $first_author_url, $second_author_url ], + $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls' ) + ); + $this->validation_url_provider->include_conditionals = []; + } + + /** + * Test does_taxonomy_support_amp. + * + * @covers ::does_taxonomy_support_amp() + */ + public function test_does_taxonomy_support_amp() { + $custom_taxonomy = 'foo_custom_taxonomy'; + register_taxonomy( $custom_taxonomy, 'post' ); + $taxonomies_to_test = [ $custom_taxonomy, 'category', 'post_tag' ]; + AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ 'is_category', 'is_tag', sprintf( 'is_tax[%s]', $custom_taxonomy ) ] ); + + // When these templates are not unchecked in the 'AMP Settings' UI, these should be supported. + foreach ( $taxonomies_to_test as $taxonomy ) { + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + } + + // When the user has not checked the boxes for 'Categories' and 'Tags,' this should be false. + 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->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + } + + // When $include_unsupported is true, all taxonomies should be supported. + $this->validation_url_provider->include_unsupported = true; + foreach ( $taxonomies_to_test as $taxonomy ) { + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + } + $this->validation_url_provider->include_unsupported = false; + + // 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 ); + foreach ( $taxonomies_to_test as $taxonomy ) { + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + } + AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); + + /* + * 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->validation_url_provider->include_conditionals = [ 'is_category', 'is_tag' ]; + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'category' ] ) ); + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'tag' ] ) ); + $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'author' ] ) ); + $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'search' ] ) ); + $this->validation_url_provider->include_conditionals = []; + } + + /** + * Test is_template_supported. + * + * @covers ::is_template_supported() + */ + public function test_is_template_supported() { + $author_conditional = 'is_author'; + $search_conditional = 'is_search'; + + AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ $author_conditional ] ); + AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $author_conditional ] ) ); + $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $search_conditional ] ) ); + + AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ $search_conditional ] ); + $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $search_conditional ] ) ); + $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $author_conditional ] ) ); + } + + /** + * Test get_posts_by_type. + * + * @covers ::get_posts_by_type() + */ + public function test_get_posts_by_type() { + $number_posts_each_post_type = 20; + $post_types = get_post_types( [ 'public' => true ], 'names' ); + + foreach ( $post_types as $post_type ) { + // Start the expected posts with the existing post(s). + $query = new WP_Query( + [ + 'fields' => 'ids', + 'post_type' => $post_type, + ] + ); + $expected_posts = $query->posts; + + for ( $i = 0; $i < $number_posts_each_post_type; $i++ ) { + array_unshift( + $expected_posts, + self::factory()->post->create( + [ + 'post_type' => $post_type, + ] + ) + ); + } + + $actual_posts = $this->call_private_method( $this->validation_url_provider, 'get_posts_by_type', [ $post_type ] ); + $this->assertEquals( $expected_posts, array_values( $actual_posts ) ); + + // Test with the $offset and $number arguments. + $offset = 0; + $actual_posts = $this->call_private_method( $this->validation_url_provider, 'get_posts_by_type', [ $post_type, $offset, $number_posts_each_post_type ] ); + $this->assertEquals( array_slice( $expected_posts, $offset, $number_posts_each_post_type ), $actual_posts ); + } + } + + /** + * Test get_taxonomy_links. + * + * @covers ::get_taxonomy_links() + */ + public function test_get_taxonomy_links() { + $number_links_each_taxonomy = 20; + $taxonomies = get_taxonomies( + [ + 'public' => true, + ] + ); + + foreach ( $taxonomies as $taxonomy ) { + // Begin the expected links with the term links that already exist. + $expected_links = array_map( 'get_term_link', get_terms( [ 'taxonomy' => $taxonomy ] ) ); + $terms_for_current_taxonomy = []; + for ( $i = 0; $i < $number_links_each_taxonomy; $i++ ) { + $terms_for_current_taxonomy[] = self::factory()->term->create( + [ + 'taxonomy' => $taxonomy, + ] + ); + } + + // Terms need to be associated with a post in order to be returned in get_terms(). + wp_set_post_terms( + self::factory()->post->create(), + $terms_for_current_taxonomy, + $taxonomy + ); + + $expected_links = array_merge( + $expected_links, + array_map( 'get_term_link', $terms_for_current_taxonomy ) + ); + $number_of_links = 100; + $actual_links = $this->call_private_method( $this->validation_url_provider, 'get_taxonomy_links', [ $taxonomy, 0, $number_of_links ] ); + + // The get_terms() call in get_taxonomy_links() returns an array with a first index of 1, so correct for that with array_values(). + $this->assertEquals( $expected_links, array_values( $actual_links ) ); + $this->assertLessThan( $number_of_links, count( $actual_links ) ); + + $number_of_links = 5; + $offset = 10; + $actual_links_using_offset = $this->call_private_method( $this->validation_url_provider, 'get_taxonomy_links', [ $taxonomy, $offset, $number_of_links ] ); + $this->assertEquals( array_slice( $expected_links, $offset, $number_of_links ), array_values( $actual_links_using_offset ) ); + $this->assertEquals( $number_of_links, count( $actual_links_using_offset ) ); + } + } + + /** + * Test get_search_page. + * + * @covers ::get_search_page() + */ + public function test_get_search_page() { + // 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->validation_url_provider, 'get_search_page' ) ) ); + + // If $include_conditionals is set and does not have is_search, this should not return a URL. + $this->validation_url_provider->include_conditionals = [ 'is_author' ]; + $this->assertEquals( null, $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); + + // If $include_conditionals has is_search, this should return a URL. + $this->validation_url_provider->include_conditionals = [ 'is_search' ]; + $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); + $this->validation_url_provider->include_conditionals = []; + } + + /** + * Test get_date_page. + * + * @covers ::get_date_page() + */ + public function test_get_date_page() { + $year = gmdate( 'Y' ); + + // Normally, this should return the date page, unless the user has opted out of that template. + $this->assertStringContains( $year, $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + + // If $include_conditionals is set and does not have is_date, this should not return a URL. + $this->validation_url_provider->include_conditionals = [ 'is_search' ]; + $this->assertEquals( null, $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + + // If $include_conditionals has is_date, this should return a URL. + $this->validation_url_provider->include_conditionals = [ 'is_date' ]; + $parsed_page_url = wp_parse_url( $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + $this->assertStringContains( $year, $parsed_page_url['query'] ); + $this->validation_url_provider->include_conditionals = []; + } +} diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index 3280a98ffb6..000193aa9a6 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -8,11 +8,14 @@ use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; +use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; /** * Tests for Test_AMP_CLI_Validation_Command class. * * @since 1.0 + * + * @coversDefaultClass AMP_CLI_Validation_Command */ class Test_AMP_CLI_Validation_Command extends \WP_UnitTestCase { @@ -33,367 +36,16 @@ class Test_AMP_CLI_Validation_Command extends \WP_UnitTestCase { */ public function setUp() { parent::setUp(); - $this->validation = new AMP_CLI_Validation_Command(); - add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); - $this->validation->include_conditionals = []; - $this->validation->limit_type_validate_count = 100; - } - - /** - * Test count_urls_to_validate. - * - * @covers AMP_CLI_Validation_Command::count_urls_to_validate() - */ - public function test_count_urls_to_validate() { - // The number of original URLs present before adding these test URLs. - $number_original_urls = $this->get_inital_url_count(); - $this->assertEquals( $number_original_urls, $this->call_private_method( $this->validation, 'count_urls_to_validate' ) ); - $this->validation->limit_type_validate_count = 100; - - $category = self::factory()->term->create( [ 'taxonomy' => 'category' ] ); - $number_new_posts = $this->validation->limit_type_validate_count / 2; - $post_ids = []; - for ( $i = 0; $i < $number_new_posts; $i++ ) { - $post_ids[] = self::factory()->post->create( - [ - 'tax_input' => [ 'category' => $category ], - ] - ); - } - /* - * 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; - $this->assertEquals( $expected_url_count, $this->call_private_method( $this->validation, 'count_urls_to_validate' ) ); - - $number_of_new_terms = 20; - $expected_url_count += $number_of_new_terms; - $taxonomy = 'category'; - $terms_for_current_taxonomy = []; - for ( $i = 0; $i < $number_of_new_terms; $i++ ) { - $terms_for_current_taxonomy[] = self::factory()->term->create( - [ - 'taxonomy' => $taxonomy, - ] - ); - } - - // Terms need to be associated with a post in order to be returned in get_terms(). - wp_set_post_terms( - $post_ids[0], - $terms_for_current_taxonomy, - $taxonomy - ); - - $this->assertEquals( $expected_url_count, $this->call_private_method( $this->validation, 'count_urls_to_validate' ) ); - } - - /** - * Test get_posts_that_support_amp. - * - * @covers AMP_CLI_Validation_Command::get_posts_that_support_amp() - */ - public function test_get_posts_that_support_amp() { - $number_of_posts = 20; - $ids = []; - for ( $i = 0; $i < $number_of_posts; $i++ ) { - $ids[] = self::factory()->post->create(); - } - - // This should count all of the newly-created posts as supporting AMP. - $this->assertEquals( $ids, $this->call_private_method( $this->validation, 'get_posts_that_support_amp', [ $ids ] ) ); - - // Simulate 'Enable AMP' being unchecked in the post editor, in which case get_url_count() should not count it. - $first_id = $ids[0]; - update_post_meta( - $first_id, - AMP_Post_Meta_Box::STATUS_POST_META_KEY, - AMP_Post_Meta_Box::DISABLED_STATUS - ); - $this->assertEquals( [], $this->call_private_method( $this->validation, 'get_posts_that_support_amp', [ [ $first_id ] ] ) ); - - update_post_meta( - $first_id, - AMP_Post_Meta_Box::STATUS_POST_META_KEY, - 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->validation->force_crawl_urls = true; - $this->assertEquals( $ids, $this->call_private_method( $this->validation, 'get_posts_that_support_amp', [ $ids ] ) ); - $this->validation->force_crawl_urls = false; - - // 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 ); - $this->assertEquals( $ids, $this->call_private_method( $this->validation, 'get_posts_that_support_amp', [ $ids ] ) ); - - // In Transitional Mode, the IDs should also include all of the newly-created posts. - add_theme_support( - AMP_Theme_Support::SLUG, - [ - AMP_Theme_Support::PAIRED_FLAG => true, - ] - ); - $this->assertEquals( $ids, $this->call_private_method( $this->validation, '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->validation->include_conditionals = [ 'is_tag', 'is_category' ]; - $this->assertEquals( [], $this->call_private_method( $this->validation, 'get_posts_that_support_amp', [ $ids ] ) ); - - /* - * If is_singular is in the WP-CLI argument, it should allow return these posts as being AMP-enabled. - * For example, wp amp validate-site include=is_singular,is_category - */ - $this->validation->include_conditionals = [ 'is_singular', 'is_category' ]; - $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->validation, 'get_posts_that_support_amp', [ $ids ] ) ) ); - $this->validation->include_conditionals = []; - } - - /** - * Test does_taxonomy_support_amp. - * - * @covers AMP_CLI_Validation_Command::does_taxonomy_support_amp() - */ - public function test_does_taxonomy_support_amp() { - $custom_taxonomy = 'foo_custom_taxonomy'; - register_taxonomy( $custom_taxonomy, 'post' ); - $taxonomies_to_test = [ $custom_taxonomy, 'category', 'post_tag' ]; - AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ 'is_category', 'is_tag', sprintf( 'is_tax[%s]', $custom_taxonomy ) ] ); - - // When these templates are not unchecked in the 'AMP Settings' UI, these should be supported. - foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); - } - - // When the user has not checked the boxes for 'Categories' and 'Tags,' this should be false. - 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->validation, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); - } - - // When $force_crawl_urls is true, all taxonomies should be supported. - $this->validation->force_crawl_urls = true; - foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); - } - $this->validation->force_crawl_urls = false; - - // 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 ); - foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); - } - AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); - - /* - * 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->validation->include_conditionals = [ 'is_category', 'is_tag' ]; - $this->assertTrue( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ 'category' ] ) ); - $this->assertTrue( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ 'tag' ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ 'author' ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation, 'does_taxonomy_support_amp', [ 'search' ] ) ); - $this->validation->include_conditionals = []; - } - - /** - * Test is_template_supported. - * - * @covers AMP_CLI_Validation_Command::is_template_supported() - */ - public function test_is_template_supported() { - $author_conditional = 'is_author'; - $search_conditional = 'is_search'; - - AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ $author_conditional ] ); - AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); - $this->assertTrue( $this->call_private_method( $this->validation, 'is_template_supported', [ $author_conditional ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation, 'is_template_supported', [ $search_conditional ] ) ); - - AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ $search_conditional ] ); - $this->assertTrue( $this->call_private_method( $this->validation, 'is_template_supported', [ $search_conditional ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation, 'is_template_supported', [ $author_conditional ] ) ); - } - - /** - * Test get_posts_by_type. - * - * @covers AMP_CLI_Validation_Command::get_posts_by_type() - */ - public function test_get_posts_by_type() { - $number_posts_each_post_type = 20; - $post_types = get_post_types( [ 'public' => true ], 'names' ); - - foreach ( $post_types as $post_type ) { - // Start the expected posts with the existing post(s). - $query = new WP_Query( - [ - 'fields' => 'ids', - 'post_type' => $post_type, - ] - ); - $expected_posts = $query->posts; - - for ( $i = 0; $i < $number_posts_each_post_type; $i++ ) { - array_unshift( - $expected_posts, - self::factory()->post->create( - [ - 'post_type' => $post_type, - ] - ) - ); - } - - $actual_posts = $this->call_private_method( $this->validation, 'get_posts_by_type', [ $post_type ] ); - $this->assertEquals( $expected_posts, array_values( $actual_posts ) ); - - // Test with the $offset and $number arguments. - $offset = 0; - $actual_posts = $this->call_private_method( $this->validation, 'get_posts_by_type', [ $post_type, $offset, $number_posts_each_post_type ] ); - $this->assertEquals( array_slice( $expected_posts, $offset, $number_posts_each_post_type ), $actual_posts ); - } - } - - /** - * Test get_taxonomy_links. - * - * @covers AMP_CLI_Validation_Command::get_taxonomy_links() - */ - public function test_get_taxonomy_links() { - $number_links_each_taxonomy = 20; - $taxonomies = get_taxonomies( - [ - 'public' => true, - ] - ); - - foreach ( $taxonomies as $taxonomy ) { - // Begin the expected links with the term links that already exist. - $expected_links = array_map( 'get_term_link', get_terms( [ 'taxonomy' => $taxonomy ] ) ); - $terms_for_current_taxonomy = []; - for ( $i = 0; $i < $number_links_each_taxonomy; $i++ ) { - $terms_for_current_taxonomy[] = self::factory()->term->create( - [ - 'taxonomy' => $taxonomy, - ] - ); - } - - // Terms need to be associated with a post in order to be returned in get_terms(). - wp_set_post_terms( - self::factory()->post->create(), - $terms_for_current_taxonomy, - $taxonomy - ); - - $expected_links = array_merge( - $expected_links, - array_map( 'get_term_link', $terms_for_current_taxonomy ) - ); - $number_of_links = 100; - $actual_links = $this->call_private_method( $this->validation, 'get_taxonomy_links', [ $taxonomy, 0, $number_of_links ] ); - - // The get_terms() call in get_taxonomy_links() returns an array with a first index of 1, so correct for that with array_values(). - $this->assertEquals( $expected_links, array_values( $actual_links ) ); - $this->assertLessThan( $number_of_links, count( $actual_links ) ); - - $number_of_links = 5; - $offset = 10; - $actual_links_using_offset = $this->call_private_method( $this->validation, 'get_taxonomy_links', [ $taxonomy, $offset, $number_of_links ] ); - $this->assertEquals( array_slice( $expected_links, $offset, $number_of_links ), array_values( $actual_links_using_offset ) ); - $this->assertEquals( $number_of_links, count( $actual_links_using_offset ) ); - } - } - - /** - * Test get_author_page_urls. - * - * @covers AMP_CLI_Validation_Command::get_author_page_urls() - */ - public function test_get_author_page_urls() { - self::factory()->user->create(); - $users = get_users(); - $first_author = $users[0]; - $first_author_url = get_author_posts_url( $first_author->ID, $first_author->user_nicename ); - $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->validation, 'get_author_page_urls', [ 0, 1 ] ); - - // Passing 0 as the offset argument should get the first author. - $this->assertEquals( [ $first_author_url ], $actual_urls ); - - $actual_urls = $this->call_private_method( $this->validation, 'get_author_page_urls', [ 1, 1 ] ); - - // Passing 1 as the offset argument should get the second author. - $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->validation->include_conditionals = [ 'is_category' ]; - $this->assertEquals( [], $this->call_private_method( $this->validation, 'get_author_page_urls' ) ); - - // If $include_conditionals is set and has is_author, this should return URLs. - $this->validation->include_conditionals = [ 'is_author' ]; - $this->assertEquals( - [ $first_author_url, $second_author_url ], - $this->call_private_method( $this->validation, 'get_author_page_urls' ) - ); - $this->validation->include_conditionals = []; - } - - /** - * Test get_search_page. - * - * @covers AMP_CLI_Validation_Command::get_search_page() - */ - public function test_get_search_page() { - // 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->validation, 'get_search_page' ) ) ); - - // If $include_conditionals is set and does not have is_search, this should not return a URL. - $this->validation->include_conditionals = [ 'is_author' ]; - $this->assertEquals( null, $this->call_private_method( $this->validation, 'get_search_page' ) ); - - // If $include_conditionals has is_search, this should return a URL. - $this->validation->include_conditionals = [ 'is_search' ]; - $this->assertTrue( is_string( $this->call_private_method( $this->validation, 'get_search_page' ) ) ); - $this->validation->include_conditionals = []; - } - - /** - * Test get_date_page. - * - * @covers AMP_CLI_Validation_Command::get_date_page() - */ - public function test_get_date_page() { - $year = gmdate( 'Y' ); - - // Normally, this should return the date page, unless the user has opted out of that template. - $this->assertStringContains( $year, $this->call_private_method( $this->validation, 'get_date_page' ) ); - - // If $include_conditionals is set and does not have is_date, this should not return a URL. - $this->validation->include_conditionals = [ 'is_search' ]; - $this->assertEquals( null, $this->call_private_method( $this->validation, 'get_date_page' ) ); - - // If $include_conditionals has is_date, this should return a URL. - $this->validation->include_conditionals = [ 'is_date' ]; - $parsed_page_url = wp_parse_url( $this->call_private_method( $this->validation, 'get_date_page' ) ); - $this->assertStringContains( $year, $parsed_page_url['query'] ); - $this->validation->include_conditionals = []; + $this->validation = new AMP_CLI_Validation_Command(); + add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } /** * Test crawl_site. * - * @covers AMP_CLI_Validation_Command::crawl_site() + * @covers ::crawl_site() */ public function test_crawl_site() { $number_of_posts = 20; @@ -410,8 +62,9 @@ public function test_crawl_site() { $this->call_private_method( $this->validation, 'crawl_site' ); // All of the posts created above should be present in $validated_urls. - $this->assertEmpty( array_diff( $post_permalinks, self::get_validated_urls() ) ); + $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); + $this->validation = new AMP_CLI_Validation_Command(); for ( $i = 0; $i < $number_of_terms; $i++ ) { $terms[] = self::factory()->category->create(); } @@ -420,107 +73,10 @@ public function test_crawl_site() { wp_set_post_terms( $posts[0], $terms, 'category' ); $this->call_private_method( $this->validation, 'crawl_site' ); $expected_validated_urls = array_map( 'get_term_link', $terms ); - $actual_validated_urls = self::get_validated_urls(); + $actual_validated_urls = ValidationRequestMocking::get_validated_urls(); // All of the terms created above should be present in $validated_urls. $this->assertEmpty( array_diff( $expected_validated_urls, $actual_validated_urls ) ); - $this->assertTrue( in_array( home_url( '/' ), self::get_validated_urls(), true ) ); - } - - /** - * Test validate_and_store_url. - * - * @covers AMP_CLI_Validation_Command::validate_and_store_url() - */ - public function test_validate_and_store_url() { - $single_post_permalink = get_permalink( self::factory()->post->create() ); - $this->call_private_method( $this->validation, 'validate_and_store_url', [ $single_post_permalink, 'post' ] ); - $this->assertTrue( in_array( $single_post_permalink, self::get_validated_urls(), true ) ); - - $number_of_posts = 30; - $post_permalinks = []; - - for ( $i = 0; $i < $number_of_posts; $i++ ) { - $permalink = get_permalink( self::factory()->post->create() ); - $post_permalinks[] = $permalink; - $this->call_private_method( $this->validation, 'validate_and_store_url', [ $permalink, 'post' ] ); - } - - // All of the posts created should be present in the validated URLs. - $this->assertEmpty( array_diff( $post_permalinks, self::get_validated_urls() ) ); - } - - /** - * Gets the initial count of URLs on the site. - * - * @return int The initial count of URLs. - */ - public function get_inital_url_count() { - $total_count = 'posts' === get_option( 'show_on_front' ) ? 1 : 0; - $post_query = new WP_Query( [ 'post_type' => get_post_types( [ 'public' => true ], 'names' ) ] ); - $total_count += $post_query->found_posts; - - $term_query = new WP_Term_Query( - [ - 'taxonomy' => get_taxonomies( [ 'public' => true ] ), - 'fields' => 'ids', - ] - ); - - $total_count += count( $term_query->terms ); - $total_count += count( $this->call_private_method( $this->validation, 'get_author_page_urls' ) ); - $total_count += is_string( $this->call_private_method( $this->validation, 'get_search_page' ) ) ? 1 : 0; - $total_count += is_string( $this->call_private_method( $this->validation, 'get_date_page' ) ) ? 1 : 0; - - return $total_count; - } - - /** - * Gets all of the validated URLs. - * - * @return string[] $urls The validated URLs. - */ - public function get_validated_urls() { - $query = new WP_Query( - [ - 'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, - 'posts_per_page' => 100, - 'fields' => 'ids', - ] - ); - - return array_map( - static function( $post ) { - return remove_query_arg( 'amp', AMP_Validated_URL_Post_Type::get_url_from_post( $post ) ); - }, - $query->posts - ); - } - - /** - * Construct a WP HTTP response for a validation request. - * - * @return array The response. - */ - public function get_validate_response() { - $mock_validation = [ - 'results' => [ - [ - 'error' => [ - 'code' => 'foo', - ], - 'sanitized' => false, - ], - ], - 'url' => home_url( '/' ), - ]; - - return [ - 'body' => wp_json_encode( $mock_validation ), - 'response' => [ - 'code' => 200, - 'message' => 'ok', - ], - ]; + $this->assertTrue( in_array( home_url( '/' ), ValidationRequestMocking::get_validated_urls(), true ) ); } } From 6c3a318661f573e194bd6a9a5ecd00483f21ee14 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 31 Aug 2020 20:39:33 -0500 Subject: [PATCH 02/41] URL validation: return WP_Error if validity is an error --- src/Validation/URLValidationProvider.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 867eb745ffe..85afacbf0b6 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -143,12 +143,11 @@ public function with_lock( $callback ) { * @param string $url The URL to validate. * @param string $type The type of template, post, or taxonomy. * @param string $flag Flag determining whether the URL should be revalidated. - * @return array Associative array containing validity results along with error info and whether the URL was revalidated. + * @return array|WP_Error Associative array containing validity result and whether the URL was revalidated, or a WP_Error on failure. */ public function get_url_validation( $url, $type, $flag = null ) { $validity = null; $revalidated = true; - $error = null; if ( self::FLAG_FORCE_REVALIDATE !== $flag ) { $url_post = AMP_Validated_URL_Post_Type::get_invalid_url_post( $url ); @@ -164,13 +163,12 @@ public function get_url_validation( $url, $type, $flag = null ) { } if ( is_wp_error( $validity ) ) { - $validity = null; - $error = $validity; + return $validity; } elseif ( $validity && isset( $validity['results'] ) ) { $this->update_state_from_validity( $validity, $type ); } - return compact( 'validity', 'revalidated', 'error' ); + return compact( 'validity', 'revalidated' ); } /** From 4a069f4d516b6d838308e3f0905fc7a7963324de Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 31 Aug 2020 20:42:45 -0500 Subject: [PATCH 03/41] Use with_lock where method is called instead of inside method --- .../cli/class-amp-cli-validation-command.php | 60 +++++++++---------- .../test-class-amp-cli-validation-command.php | 10 ++-- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index dddcb038afd..a666f2720ae 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -138,7 +138,18 @@ public function run( $args, $assoc_args ) { sprintf( 'Validating %d URLs...', $number_urls_to_crawl ), $number_urls_to_crawl ); - $this->crawl_site(); + + $result = $validation_provider->with_lock( + function() { + $this->validate_urls(); + } + ); + + if ( is_wp_error( $result ) ) { + WP_CLI::error( 'The site cannot be crawled at this time because validation is running in another process.' ); + return; + } + $this->wp_cli_progress->finish(); $key_template_type = 'Template or content type'; @@ -280,42 +291,29 @@ private function get_validation_provider() { } /** - * Validates the URLs of the entire site. - * - * Includes the URLs of public, published posts, public taxonomies, and other templates. - * This validates one of each type at a time, - * and iterates until it reaches the maximum number of URLs for each type. + * Validates the URLs. */ - private function crawl_site() { + private function validate_urls() { $validation_url_provider = $this->get_validation_url_provider(); $validation_provider = $this->get_validation_provider(); - $result = $validation_provider->with_lock( - function() use ( $validation_url_provider, $validation_provider ) { - foreach ( $validation_url_provider->get_urls() as $url ) { - $validity = $validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); - - if ( $this->wp_cli_progress ) { - $this->wp_cli_progress->tick(); - } - - if ( is_wp_error( $validity['error'] ) ) { - WP_CLI::warning( - sprintf( - 'Validate URL error (%1$s): %2$s URL: %3$s', - $validity['error']->get_error_code(), - $validity['error']->get_error_message(), - $url - ) - ); - } - } + foreach ( $validation_url_provider->get_urls() as $url ) { + $validity = $validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); + + if ( $this->wp_cli_progress ) { + $this->wp_cli_progress->tick(); } - ); - if ( is_wp_error( $result ) ) { - WP_CLI::error( 'The site cannot be crawled at this time because validation is running in another process.' ); - return; + if ( is_wp_error( $validity ) ) { + WP_CLI::warning( + sprintf( + 'Validate URL error (%1$s): %2$s URL: %3$s', + $validity['error']->get_error_code(), + $validity['error']->get_error_message(), + $url + ) + ); + } } } diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index 000193aa9a6..87bfa8ddd19 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -43,11 +43,11 @@ public function setUp() { } /** - * Test crawl_site. + * Test validate_urls. * - * @covers ::crawl_site() + * @covers ::validate_urls() */ - public function test_crawl_site() { + public function test_validate_urls() { $number_of_posts = 20; $number_of_terms = 30; $posts = []; @@ -59,7 +59,7 @@ public function test_crawl_site() { $posts[] = $post_id; $post_permalinks[] = get_permalink( $post_id ); } - $this->call_private_method( $this->validation, 'crawl_site' ); + $this->call_private_method( $this->validation, 'validate_urls' ); // All of the posts created above should be present in $validated_urls. $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); @@ -71,7 +71,7 @@ public function test_crawl_site() { // Terms need to be associated with a post in order to be returned in get_terms(). wp_set_post_terms( $posts[0], $terms, 'category' ); - $this->call_private_method( $this->validation, 'crawl_site' ); + $this->call_private_method( $this->validation, 'validate_urls' ); $expected_validated_urls = array_map( 'get_term_link', $terms ); $actual_validated_urls = ValidationRequestMocking::get_validated_urls(); From 47f22c895cef4cd0345addb02340b563a21d039e Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 6 Oct 2020 10:05:11 -0500 Subject: [PATCH 04/41] Minor updates primarily in formatting and comments Co-authored-by: Alain Schlesser --- .../cli/class-amp-cli-validation-command.php | 4 +-- src/Validation/URLValidationProvider.php | 6 ++-- src/Validation/ValidationURLProvider.php | 26 ++++++++------- .../Validation/URLValidationProviderTest.php | 24 ++++++++------ .../Validation/ValidationURLProviderTest.php | 32 +++++++++++-------- 5 files changed, 53 insertions(+), 39 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index a666f2720ae..dc30dfd791f 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -140,7 +140,7 @@ public function run( $args, $assoc_args ) { ); $result = $validation_provider->with_lock( - function() { + function () { $this->validate_urls(); } ); @@ -232,7 +232,7 @@ private function get_validation_url_provider() { /* * Handle the argument and flag passed to the command: --include and --force. - * If the self::INCLUDE_ARGUMENT is present, force crawling or URLs. + * 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( $assoc_args[ self::INCLUDE_ARGUMENT ] ) ) { diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 85afacbf0b6..7e7ecfeb60d 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -131,7 +131,7 @@ public function with_lock( $callback ) { } $this->lock(); - $result = call_user_func( $callback ); + $result = $callback(); $this->unlock(); return $result; @@ -164,7 +164,9 @@ public function get_url_validation( $url, $type, $flag = null ) { if ( is_wp_error( $validity ) ) { return $validity; - } elseif ( $validity && isset( $validity['results'] ) ) { + } + + if ( $validity && isset( $validity['results'] ) ) { $this->update_state_from_validity( $validity, $type ); } diff --git a/src/Validation/ValidationURLProvider.php b/src/Validation/ValidationURLProvider.php index 96fd7a0aa13..9bbf58fbd3b 100644 --- a/src/Validation/ValidationURLProvider.php +++ b/src/Validation/ValidationURLProvider.php @@ -30,7 +30,7 @@ final class ValidationURLProvider { * * Usually, this class will query all of the templates that don't have AMP disabled. This allows inclusion based on only these conditionals. * - * @var array + * @var string[] */ public $include_conditionals; @@ -46,9 +46,9 @@ final class ValidationURLProvider { /** * Class constructor. * - * @param integer $limit_per_type The maximum number of URLs to validate for each type. + * @param integer $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 boolean $include_unsupported Whether to include URLs that don't support AMP. + * @param boolean $include_unsupported Whether to include URLs that don't support AMP. */ public function __construct( $limit_per_type = 20, @@ -61,9 +61,11 @@ public function __construct( } /** - * Provides the array of URLs to check. Each URL is an array with two elements, with the URL at index 0 and the type at index 1. + * Provides the array of URLs to check. * - * @param int|null $offset The number of URLs to offset by, where applicable. + * Each URL is an array with two elements, with the URL at index 0 and the type at index 1. + * + * @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 ) { @@ -143,8 +145,8 @@ public function get_urls( $offset = 0 ) { * @param string $template The template to check. * @return bool Whether the template is supported. */ - public function is_template_supported( $template ) { - // If the --include argument is present in the WP-CLI command, this template conditional must be present in it. + private function is_template_supported( $template ) { + // If we received an allowlist of conditionals, this template conditional must be present in it. if ( ! empty( $this->include_conditionals ) ) { return in_array( $template, $this->include_conditionals, true ); } @@ -159,13 +161,13 @@ public function is_template_supported( $template ) { } /** - * Gets the posts IDs that support AMP. + * Gets the post IDs that support AMP. * * By default, this only gets the post IDs if they support AMP. - * This means that 'Posts' isn't deselected in 'AMP Settings' > 'Supported Templates'. - * And 'Enable AMP' isn't unchecked in the post's editor. + * This means that 'Posts' isn't deselected in 'AMP Settings' > 'Supported Templates' + * and 'Enable AMP' isn't unchecked in the post's editor. * - * @param array $ids The post IDs to check for AMP support. + * @param int[] $ids The post IDs to check for AMP support. * @return array The post IDs that support AMP, or an empty array. */ private function get_posts_that_support_amp( $ids ) { @@ -221,7 +223,7 @@ private function get_posts_by_type( $post_type, $offset = null, $number = null ) * * @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. - * @return array The author page URLs, or an empty array. + * @return string[] The author page URLs, or an empty array. */ private function get_author_page_urls( $offset = '', $number = '' ) { $author_page_urls = []; diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index ad46cf4d7b6..0134412b1a1 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -2,15 +2,20 @@ namespace AmpProject\AmpWP\Tests\Validation; -use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; -use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; use AmpProject\AmpWP\Validation\URLValidationProvider; use WP_UnitTestCase; /** @coversDefaultClass URLValidationProvider */ final class URLValidationProviderTest extends WP_UnitTestCase { - use PrivateAccess, AssertContainsCompatibility; + use AssertContainsCompatibility; + + /** + * Validation provider instance to use. + * + * @var URLValidationProvider + */ + private $validation_provider; /** * Setup. @@ -19,7 +24,7 @@ final class URLValidationProviderTest extends WP_UnitTestCase { */ public function setUp() { parent::setUp(); - $this->validation_provider = new URLValidationProvider( 100 ); + $this->validation_provider = new URLValidationProvider(); add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } @@ -30,8 +35,8 @@ public function setUp() { */ public function test_get_url_validation() { $single_post_permalink = get_permalink( self::factory()->post->create() ); - $this->call_private_method( $this->validation_provider, 'get_url_validation', [ $single_post_permalink, 'post' ] ); - $this->assertTrue( in_array( $single_post_permalink, ValidationRequestMocking::get_validated_urls(), true ) ); + $this->validation_provider->get_url_validation( $single_post_permalink, 'post' ); + $this->assertContains( $single_post_permalink, ValidationRequestMocking::get_validated_urls() ); $number_of_posts = 30; $post_permalinks = []; @@ -39,7 +44,7 @@ public function test_get_url_validation() { for ( $i = 0; $i < $number_of_posts; $i++ ) { $permalink = get_permalink( self::factory()->post->create() ); $post_permalinks[] = $permalink; - $this->call_private_method( $this->validation_provider, 'get_url_validation', [ $permalink, 'post' ] ); + $this->validation_provider->get_url_validation( $permalink, 'post' ); } // All of the posts created should be present in the validated URLs. @@ -69,7 +74,7 @@ public function test_locking() { $expected_result = 'EXPECTED RESULT'; $result = $this->validation_provider->with_lock( - function() use ( $expected_result ) { + function () use ( $expected_result ) { $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); // Expect an error when lock is already in place. @@ -85,9 +90,8 @@ function() use ( $expected_result ) { // Test with_lock with no return value. $this->assertNull( $this->validation_provider->with_lock( - function() { + function () { $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); - return; } ) ); diff --git a/tests/php/src/Validation/ValidationURLProviderTest.php b/tests/php/src/Validation/ValidationURLProviderTest.php index 6e9c082c2f4..693280e85bc 100644 --- a/tests/php/src/Validation/ValidationURLProviderTest.php +++ b/tests/php/src/Validation/ValidationURLProviderTest.php @@ -17,6 +17,12 @@ final class ValidationURLProviderTest extends WP_UnitTestCase { use PrivateAccess, AssertContainsCompatibility; + /** + * Validation URL provider instance to use. + * + * @var ValidationURLProvider + */ + /** * Setup. * @@ -29,16 +35,14 @@ public function setUp() { } /** - * Test retreival of urls. + * Test retrieval of urls. * * @covers ::get_urls() */ public function test_count_urls_to_validate() { - $this->validation_url_provider = new ValidationURLProvider(); - $number_original_urls = 4; - $this->assertEquals( $number_original_urls, count( $this->validation_url_provider->get_urls() ) ); + $this->assertCount( $number_original_urls, $this->validation_url_provider->get_urls() ); $this->validation_url_provider = new ValidationURLProvider( 100 ); @@ -58,7 +62,7 @@ public function test_count_urls_to_validate() { * And ensure that the tested method finds a URL for all of them. */ $expected_url_count = $number_new_posts + $number_original_urls + 1; - $this->assertEquals( $expected_url_count, count( $this->validation_url_provider->get_urls() ) ); + $this->assertCount( $expected_url_count, $this->validation_url_provider->get_urls() ); $this->validation_url_provider = new ValidationURLProvider( 100 ); @@ -75,13 +79,14 @@ public function test_count_urls_to_validate() { } // Terms need to be associated with a post in order to be returned in get_terms(). - wp_set_post_terms( + $result = wp_set_post_terms( $post_ids[0], $terms_for_current_taxonomy, $taxonomy ); + $this->assertFalse( is_wp_error( $result ) ); - $this->assertEquals( $expected_url_count, count( $this->validation_url_provider->get_urls() ) ); + $this->assertCount( $expected_url_count, $this->validation_url_provider->get_urls() ); } /** @@ -140,7 +145,7 @@ public function test_get_posts_that_support_amp() { $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); /* - * If is_singular is in the WP-CLI argument, it should allow return these posts as being AMP-enabled. + * 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->validation_url_provider->include_conditionals = [ 'is_singular', 'is_category' ]; @@ -318,11 +323,12 @@ public function test_get_taxonomy_links() { } // Terms need to be associated with a post in order to be returned in get_terms(). - wp_set_post_terms( - self::factory()->post->create(), - $terms_for_current_taxonomy, - $taxonomy - ); + $result = wp_set_post_terms( + self::factory()->post->create(), + $terms_for_current_taxonomy, + $taxonomy + ); + $this->assertFalse( is_wp_error( $result ) ); $expected_links = array_merge( $expected_links, From 640d6b641058c76f0e1e18fe211042610141e8c6 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 07:34:21 -0500 Subject: [PATCH 05/41] Use get_flag_value in CLI script to handle assoc args Co-authored-by: Alain Schlesser --- .../cli/class-amp-cli-validation-command.php | 31 +++---------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index dc30dfd791f..d28dae36e12 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -194,25 +194,6 @@ function () { WP_CLI::line( sprintf( 'For more details, please see: %s', $url_more_details ) ); } - /** - * Provides the associative args for the command. - * - * @return array - */ - private function get_assoc_args() { - if ( ! is_array( $this->assoc_args ) ) { - $this->assoc_args = []; - } - - return array_merge( - [ - self::LIMIT_URLS_ARGUMENT => 100, - self::INCLUDE_ARGUMENT => [], - self::FLAG_NAME_FORCE_VALIDATION => false, - ], - $this->assoc_args - ); - } /** * Provides the ValidationURLProvider instance. @@ -224,22 +205,18 @@ private function get_validation_url_provider() { return $this->validation_url_provider; } - $assoc_args = $this->get_assoc_args(); - - $include_conditionals = []; - $force_crawl_urls = false; - $limit_type_validate_count = (int) $assoc_args[ self::LIMIT_URLS_ARGUMENT ]; + $include_conditionals = Utils::get_flag_value( $assoc_args, self::INCLUDE_ARGUMENT, [] ); + $force_crawl_urls = Utils::get_flag_value( $assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); + $limit_type_validate_count = Utils::get_flag_value( $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( $assoc_args[ self::INCLUDE_ARGUMENT ] ) ) { + if ( ! empty( $include_conditionals ) ) { $include_conditionals = explode( ',', $assoc_args[ self::INCLUDE_ARGUMENT ] ); $force_crawl_urls = true; - } elseif ( isset( $assoc_args[ self::FLAG_NAME_FORCE_VALIDATION ] ) ) { - $force_crawl_urls = true; } // Handle special case for Legacy Reader mode. From 9661becd75191c49f03d61ff8e0f7d2c68d22f48 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 07:34:39 -0500 Subject: [PATCH 06/41] PHPCS fixes --- tests/php/src/Validation/ValidationURLProviderTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/php/src/Validation/ValidationURLProviderTest.php b/tests/php/src/Validation/ValidationURLProviderTest.php index 693280e85bc..aa281a0bdde 100644 --- a/tests/php/src/Validation/ValidationURLProviderTest.php +++ b/tests/php/src/Validation/ValidationURLProviderTest.php @@ -324,11 +324,11 @@ public function test_get_taxonomy_links() { // Terms need to be associated with a post in order to be returned in get_terms(). $result = wp_set_post_terms( - self::factory()->post->create(), - $terms_for_current_taxonomy, - $taxonomy - ); - $this->assertFalse( is_wp_error( $result ) ); + self::factory()->post->create(), + $terms_for_current_taxonomy, + $taxonomy + ); + $this->assertFalse( is_wp_error( $result ) ); $expected_links = array_merge( $expected_links, From 1cf083d269a9261354ee2fd483705ad2078edbf8 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 08:19:00 -0500 Subject: [PATCH 07/41] Fix CLI script imports --- .../cli/class-amp-cli-validation-command.php | 24 +++++++------ src/Validation/URLValidationProvider.php | 35 ++++++++++++++++--- .../Validation/URLValidationProviderTest.php | 9 +++-- 3 files changed, 50 insertions(+), 18 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index d28dae36e12..e358234cbcc 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -11,6 +11,7 @@ use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Validation\URLValidationProvider; use AmpProject\AmpWP\Validation\ValidationURLProvider; +use Utils; /** * Crawls the site for validation errors or resets the stored validation errors. @@ -134,7 +135,7 @@ public function run( $args, $assoc_args ) { WP_CLI::log( 'Crawling the site for AMP validity.' ); - $this->wp_cli_progress = WP_CLI\Utils\make_progress_bar( + $this->wp_cli_progress = Utils::make_progress_bar( sprintf( 'Validating %d URLs...', $number_urls_to_crawl ), $number_urls_to_crawl ); @@ -173,14 +174,14 @@ function () { 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.', - $validation_provider->number_crawled, - $validation_provider->total_errors, - $validation_provider->unaccepted_errors + $validation_provider->get_number_validated(), + $validation_provider->get_total_errors(), + $validation_provider->get_unaccepted_errors() ) ); // Output a table of validity by template/content type. - WP_CLI\Utils\format_items( + Utils::format_items( 'table', $table_validation_by_type, [ $key_template_type, $key_url_count, $key_validity_rate ] @@ -205,9 +206,10 @@ private function get_validation_url_provider() { return $this->validation_url_provider; } - $include_conditionals = Utils::get_flag_value( $assoc_args, self::INCLUDE_ARGUMENT, [] ); - $force_crawl_urls = Utils::get_flag_value( $assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); - $limit_type_validate_count = Utils::get_flag_value( $assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); + + $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. @@ -215,7 +217,7 @@ private function get_validation_url_provider() { * The WP-CLI command should indicate which templates are crawled, not the /wp-admin options. */ if ( ! empty( $include_conditionals ) ) { - $include_conditionals = explode( ',', $assoc_args[ self::INCLUDE_ARGUMENT ] ); + $include_conditionals = explode( ',', $include_conditionals ); $force_crawl_urls = true; } @@ -321,7 +323,7 @@ public function reset( $args, $assoc_args ) { $query = $wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE post_type = %s", AMP_Validated_URL_Post_Type::POST_TYPE_SLUG ); $posts = new WP_CLI\Iterators\Query( $query, 10000 ); - $progress = WP_CLI\Utils\make_progress_bar( + $progress = Utils::make_progress_bar( sprintf( 'Deleting %d amp_validated_url posts...', $count ), $count ); @@ -338,7 +340,7 @@ public function reset( $args, $assoc_args ) { $query = $wpdb->prepare( "SELECT term_id FROM $wpdb->term_taxonomy WHERE taxonomy = %s", AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ); $terms = new WP_CLI\Iterators\Query( $query, 10000 ); - $progress = WP_CLI\Utils\make_progress_bar( + $progress = Utils::make_progress_bar( sprintf( 'Deleting %d amp_taxonomy_error terms...', $count ), $count ); diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 7e7ecfeb60d..4050e47a6fd 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -46,7 +46,7 @@ final class URLValidationProvider { * * @var int */ - public $total_errors = 0; + private $total_errors = 0; /** * The total number of unaccepted validation errors. @@ -56,14 +56,14 @@ final class URLValidationProvider { * * @var int */ - public $unaccepted_errors = 0; + private $unaccepted_errors = 0; /** * The number of URLs crawled, regardless of whether they have validation errors. * * @var int */ - public $number_crawled = 0; + private $number_validated = 0; /** * The validation counts by type, like template or post type. @@ -137,6 +137,33 @@ public function with_lock( $callback ) { return $result; } + /** + * Provides the total number of validation errors found. + * + * @return int + */ + public function get_total_errors() { + return $this->total_errors; + } + + /** + * Provides the total number of unaccepted errors. + * + * @return int + */ + public function get_unaccepted_errors() { + return $this->unaccepted_errors; + } + + /** + * Provides the number of URLs that have been checked. + * + * @return int + */ + public function get_number_validated() { + return $this->number_validated; + } + /** * Validates a URL, stores the results, and increments the counts. * @@ -202,7 +229,7 @@ static function( $error ) { $this->unaccepted_errors++; } - $this->number_crawled++; + $this->number_validated++; if ( ! isset( $this->validity_by_type[ $type ] ) ) { $this->validity_by_type[ $type ] = [ diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 0134412b1a1..17594d61f72 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -2,6 +2,7 @@ namespace AmpProject\AmpWP\Tests\Validation; +use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; use AmpProject\AmpWP\Validation\URLValidationProvider; use WP_UnitTestCase; @@ -32,6 +33,8 @@ public function setUp() { * Test get_url_validation. * * @covers ::get_url_validation() + * @covers ::get_total_errors() + * @covers ::get_unaccepted_errors() */ public function test_get_url_validation() { $single_post_permalink = get_permalink( self::factory()->post->create() ); @@ -50,9 +53,9 @@ public function test_get_url_validation() { // All of the posts created should be present in the validated URLs. $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); - $this->assertEquals( 31, $this->validation_provider->total_errors ); - $this->assertEmpty( $this->validation_provider->unaccepted_errors ); - $this->assertEquals( 31, $this->validation_provider->number_crawled ); + $this->assertEquals( 31, $this->validation_provider->get_total_errors() ); + $this->assertEmpty( $this->validation_provider->get_unaccepted_errors() ); + $this->assertEquals( 31, $this->validation_provider->get_number_validated() ); $this->assertEquals( [ 'post' ], From 8b6099c9e303eb02d71697ba2dad78e7ed88e76d Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 08:31:27 -0500 Subject: [PATCH 08/41] Use option instead of transient for locking --- includes/cli/class-amp-cli-validation-command.php | 2 -- src/Validation/URLValidationProvider.php | 11 +++++++---- .../php/src/Validation/URLValidationProviderTest.php | 10 +++++----- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index e358234cbcc..d75d3ca49fb 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -195,7 +195,6 @@ function () { WP_CLI::line( sprintf( 'For more details, please see: %s', $url_more_details ) ); } - /** * Provides the ValidationURLProvider instance. * @@ -206,7 +205,6 @@ private function get_validation_url_provider() { return $this->validation_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 ); diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 4050e47a6fd..75bcb71d84c 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -25,7 +25,7 @@ final class URLValidationProvider { * * @var string */ - const LOCK_TRANSIENT = 'amp_validation_locked'; + const LOCK_KEY = 'amp_validation_locked'; /** * Flag to pass to get_url_validation to force revalidation. @@ -83,14 +83,14 @@ final class URLValidationProvider { * Locks validation. */ private function lock() { - set_transient( self::LOCK_TRANSIENT, 'locked', $this->get_lock_timeout() ); + update_option( self::LOCK_KEY, time(), false ); } /** * Unlocks validation. */ private function unlock() { - delete_transient( self::LOCK_TRANSIENT ); + delete_option( self::LOCK_KEY ); } /** @@ -99,7 +99,10 @@ private function unlock() { * @return boolean */ public function is_locked() { - return get_transient( self::LOCK_TRANSIENT ); + $lock_time = intval( get_option( self::LOCK_KEY, 0 ) ); + + // It's locked if the difference between the lock time and the current time is less than the lockout time. + return time() - $lock_time < $this->get_lock_timeout(); } /** diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 17594d61f72..1728ec09035 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -73,12 +73,12 @@ public function test_get_url_validation() { * @covers ::with_lock */ public function test_locking() { - $this->assertFalse( get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + $this->assertFalse( get_transient( URLValidationProvider::LOCK_KEY ) ); $expected_result = 'EXPECTED RESULT'; $result = $this->validation_provider->with_lock( function () use ( $expected_result ) { - $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_KEY ) ); // Expect an error when lock is already in place. $this->assertWPError( $this->validation_provider->with_lock( '__return_empty_string' ) ); @@ -88,16 +88,16 @@ function () use ( $expected_result ) { ); $this->assertEquals( $expected_result, $result ); - $this->assertFalse( get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + $this->assertFalse( get_transient( URLValidationProvider::LOCK_KEY ) ); // Test with_lock with no return value. $this->assertNull( $this->validation_provider->with_lock( function () { - $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_KEY ) ); } ) ); - $this->assertFalse( get_transient( URLValidationProvider::LOCK_TRANSIENT ) ); + $this->assertFalse( get_transient( URLValidationProvider::LOCK_KEY ) ); } } From fdcc5f0de84aa162a53413f017bdd6a916706134 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 08:48:47 -0500 Subject: [PATCH 09/41] Rename ValidationURLProvider --- .../cli/class-amp-cli-validation-command.php | 27 +++++++++---------- ...LProvider.php => ScannableURLProvider.php} | 4 +-- .../Validation/ValidationURLProviderTest.php | 14 +++++----- 3 files changed, 22 insertions(+), 23 deletions(-) rename src/Validation/{ValidationURLProvider.php => ScannableURLProvider.php} (99%) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index d75d3ca49fb..72aa7c9317b 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -10,8 +10,7 @@ use AmpProject\AmpWP\Admin\ReaderThemes; use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Validation\URLValidationProvider; -use AmpProject\AmpWP\Validation\ValidationURLProvider; -use Utils; +use AmpProject\AmpWP\Validation\ScannableURLProvider; /** * Crawls the site for validation errors or resets the stored validation errors. @@ -70,9 +69,9 @@ final class AMP_CLI_Validation_Command { private $validation_provider; /** - * ValidationURLProvider instance. + * ScannableURLProvider instance. * - * @var ValidationURLProvider + * @var ScannableURLProvider */ private $validation_url_provider; @@ -135,7 +134,7 @@ public function run( $args, $assoc_args ) { WP_CLI::log( 'Crawling the site for AMP validity.' ); - $this->wp_cli_progress = Utils::make_progress_bar( + $this->wp_cli_progress = WP_CLI\Utils\make_progress_bar( sprintf( 'Validating %d URLs...', $number_urls_to_crawl ), $number_urls_to_crawl ); @@ -181,7 +180,7 @@ function () { ); // Output a table of validity by template/content type. - Utils::format_items( + WP_CLI\Utils\format_items( 'table', $table_validation_by_type, [ $key_template_type, $key_url_count, $key_validity_rate ] @@ -196,18 +195,18 @@ function () { } /** - * Provides the ValidationURLProvider instance. + * Provides the ScannableURLProvider instance. * - * @return ValidationURLProvider + * @return ScannableURLProvider */ private function get_validation_url_provider() { if ( ! is_null( $this->validation_url_provider ) ) { return $this->validation_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 ); + $include_conditionals = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); + $force_crawl_urls = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); + $limit_type_validate_count = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); /* * Handle the argument and flag passed to the command: --include and --force. @@ -243,7 +242,7 @@ private function get_validation_url_provider() { } } - $this->validation_url_provider = new ValidationURLProvider( + $this->validation_url_provider = new ScannableURLProvider( $limit_type_validate_count, $include_conditionals, $force_crawl_urls @@ -321,7 +320,7 @@ public function reset( $args, $assoc_args ) { $query = $wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE post_type = %s", AMP_Validated_URL_Post_Type::POST_TYPE_SLUG ); $posts = new WP_CLI\Iterators\Query( $query, 10000 ); - $progress = Utils::make_progress_bar( + $progress = WP_CLI\Utils\make_progress_bar( sprintf( 'Deleting %d amp_validated_url posts...', $count ), $count ); @@ -338,7 +337,7 @@ public function reset( $args, $assoc_args ) { $query = $wpdb->prepare( "SELECT term_id FROM $wpdb->term_taxonomy WHERE taxonomy = %s", AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ); $terms = new WP_CLI\Iterators\Query( $query, 10000 ); - $progress = Utils::make_progress_bar( + $progress = WP_CLI\Utils\make_progress_bar( sprintf( 'Deleting %d amp_taxonomy_error terms...', $count ), $count ); diff --git a/src/Validation/ValidationURLProvider.php b/src/Validation/ScannableURLProvider.php similarity index 99% rename from src/Validation/ValidationURLProvider.php rename to src/Validation/ScannableURLProvider.php index 9bbf58fbd3b..3f50d0b4698 100644 --- a/src/Validation/ValidationURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -12,11 +12,11 @@ use WP_Query; /** - * ValidationURLProvider class. + * ScannableURLProvider class. * * @since 2.1 */ -final class ValidationURLProvider { +final class ScannableURLProvider { /** * Whether to include URLs that don't support AMP. diff --git a/tests/php/src/Validation/ValidationURLProviderTest.php b/tests/php/src/Validation/ValidationURLProviderTest.php index aa281a0bdde..e3d620ca36b 100644 --- a/tests/php/src/Validation/ValidationURLProviderTest.php +++ b/tests/php/src/Validation/ValidationURLProviderTest.php @@ -9,18 +9,18 @@ use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; -use AmpProject\AmpWP\Validation\ValidationURLProvider; +use AmpProject\AmpWP\Validation\ScannableURLProvider; use WP_Query; use WP_UnitTestCase; -/** @coversDefaultClass ValidationURLProvider */ -final class ValidationURLProviderTest extends WP_UnitTestCase { +/** @coversDefaultClass ScannableURLProvider */ +final class ScannableURLProviderTest extends WP_UnitTestCase { use PrivateAccess, AssertContainsCompatibility; /** * Validation URL provider instance to use. * - * @var ValidationURLProvider + * @var ScannableURLProvider */ /** @@ -30,7 +30,7 @@ final class ValidationURLProviderTest extends WP_UnitTestCase { */ public function setUp() { parent::setUp(); - $this->validation_url_provider = new ValidationURLProvider(); + $this->validation_url_provider = new ScannableURLProvider(); add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } @@ -44,7 +44,7 @@ public function test_count_urls_to_validate() { $this->assertCount( $number_original_urls, $this->validation_url_provider->get_urls() ); - $this->validation_url_provider = new ValidationURLProvider( 100 ); + $this->validation_url_provider = new ScannableURLProvider( 100 ); $category = self::factory()->term->create( [ 'taxonomy' => 'category' ] ); $number_new_posts = 50; @@ -64,7 +64,7 @@ public function test_count_urls_to_validate() { $expected_url_count = $number_new_posts + $number_original_urls + 1; $this->assertCount( $expected_url_count, $this->validation_url_provider->get_urls() ); - $this->validation_url_provider = new ValidationURLProvider( 100 ); + $this->validation_url_provider = new ScannableURLProvider( 100 ); $number_of_new_terms = 20; $expected_url_count += $number_of_new_terms; From 4ff12fd3e32a597475f99f23fbae60342402acef Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 09:19:17 -0500 Subject: [PATCH 10/41] Make class properties private --- .../cli/class-amp-cli-validation-command.php | 11 +++--- src/Validation/ScannableURLProvider.php | 8 ++--- ...rTest.php => ScannableURLProviderTest.php} | 36 +++++++++---------- 3 files changed, 29 insertions(+), 26 deletions(-) rename tests/php/src/Validation/{ValidationURLProviderTest.php => ScannableURLProviderTest.php} (89%) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 72aa7c9317b..1f73eefced9 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -204,9 +204,9 @@ private function get_validation_url_provider() { return $this->validation_url_provider; } - $include_conditionals = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); - $force_crawl_urls = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); - $limit_type_validate_count = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); + $include_conditionals = ''; //WP_CLI\Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); + $force_crawl_urls = false; //WP_CLI\Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); + $limit_type_validate_count = 100; //WP_CLI\Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); /* * Handle the argument and flag passed to the command: --include and --force. @@ -214,7 +214,10 @@ private function get_validation_url_provider() { * The WP-CLI command should indicate which templates are crawled, not the /wp-admin options. */ if ( ! empty( $include_conditionals ) ) { - $include_conditionals = explode( ',', $include_conditionals ); + if ( is_string( $include_conditionals ) ) { + $include_conditionals = explode( ',', $include_conditionals ); + } + $force_crawl_urls = true; } diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 3f50d0b4698..1d4ac700688 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -23,7 +23,7 @@ final class ScannableURLProvider { * * @var bool */ - public $include_unsupported; + private $include_unsupported; /** * An allowlist of conditionals to use for querying URLs. @@ -32,7 +32,7 @@ final class ScannableURLProvider { * * @var string[] */ - public $include_conditionals; + private $include_conditionals; /** * The maximum number of URLs to provide for each content type. @@ -41,10 +41,10 @@ final class ScannableURLProvider { * * @var int */ - public $limit_per_type; + private $limit_per_type; /** - * Class constructor. + * Class construÎctor. * * @param integer $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. diff --git a/tests/php/src/Validation/ValidationURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php similarity index 89% rename from tests/php/src/Validation/ValidationURLProviderTest.php rename to tests/php/src/Validation/ScannableURLProviderTest.php index e3d620ca36b..a4a748d0cd2 100644 --- a/tests/php/src/Validation/ValidationURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -120,9 +120,9 @@ public function test_get_posts_that_support_amp() { ); // When the second $force_count_all_urls argument is true, all of the newly-created posts should be part of the URL count. - $this->validation_url_provider->include_unsupported = true; + $this->set_private_property( $this->validation_url_provider, 'include_unsupported', true ); $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); - $this->validation_url_provider->include_unsupported = false; + $this->set_private_property( $this->validation_url_provider, 'include_unsupported', false ); // 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 ); @@ -141,16 +141,16 @@ 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->validation_url_provider->include_conditionals = [ 'is_tag', 'is_category' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_tag', 'is_category' ] ); $this->assertEquals( [], $this->call_private_method( $this->validation_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->validation_url_provider->include_conditionals = [ 'is_singular', 'is_category' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_singular', 'is_category' ] ); $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ) ); - $this->validation_url_provider->include_conditionals = []; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } /** @@ -177,16 +177,16 @@ 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->validation_url_provider->include_conditionals = [ 'is_category' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_category' ] ); $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls' ) ); // If $include_conditionals is set and has is_author, this should return URLs. - $this->validation_url_provider->include_conditionals = [ 'is_author' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_author' ] ); $this->assertEquals( [ $first_author_url, $second_author_url ], $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls' ) ); - $this->validation_url_provider->include_conditionals = []; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } /** @@ -213,11 +213,11 @@ public function test_does_taxonomy_support_amp() { } // When $include_unsupported is true, all taxonomies should be supported. - $this->validation_url_provider->include_unsupported = true; + $this->set_private_property( $this->validation_url_provider, 'include_unsupported', true ); foreach ( $taxonomies_to_test as $taxonomy ) { $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); } - $this->validation_url_provider->include_unsupported = false; + $this->set_private_property( $this->validation_url_provider, 'include_unsupported', false ); // 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 ); @@ -230,12 +230,12 @@ 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->validation_url_provider->include_conditionals = [ 'is_category', 'is_tag' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_category', 'is_tag' ] ); $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'category' ] ) ); $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'tag' ] ) ); $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'author' ] ) ); $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'search' ] ) ); - $this->validation_url_provider->include_conditionals = []; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } /** @@ -359,13 +359,13 @@ public function test_get_search_page() { $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); // If $include_conditionals is set and does not have is_search, this should not return a URL. - $this->validation_url_provider->include_conditionals = [ 'is_author' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_author' ] ); $this->assertEquals( null, $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); // If $include_conditionals has is_search, this should return a URL. - $this->validation_url_provider->include_conditionals = [ 'is_search' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_search' ] ); $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); - $this->validation_url_provider->include_conditionals = []; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } /** @@ -380,13 +380,13 @@ public function test_get_date_page() { $this->assertStringContains( $year, $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); // If $include_conditionals is set and does not have is_date, this should not return a URL. - $this->validation_url_provider->include_conditionals = [ 'is_search' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_search' ] ); $this->assertEquals( null, $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); // If $include_conditionals has is_date, this should return a URL. - $this->validation_url_provider->include_conditionals = [ 'is_date' ]; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_date' ] ); $parsed_page_url = wp_parse_url( $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); $this->assertStringContains( $year, $parsed_page_url['query'] ); - $this->validation_url_provider->include_conditionals = []; + $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } } From 39c1665fbff3b744780a7dacbc221b4c1cda06b3 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 09:21:52 -0500 Subject: [PATCH 11/41] Trim outdated comment content --- src/Validation/ScannableURLProvider.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 1d4ac700688..28abbe73431 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -268,10 +268,6 @@ private function get_date_page() { /** * Gets whether the taxonomy supports AMP. * - * This only gets the term IDs if they support AMP. - * If their taxonomy is unchecked in 'AMP Settings' > 'Supported Templates,' this does not return them. - * For example, if 'Categories' is unchecked. - * * @param string $taxonomy The taxonomy. * @return boolean Whether the taxonomy supports AMP. */ From cdbad7a6c6972cb1d1ec8b7f8b130e90867859ff Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 09:32:34 -0500 Subject: [PATCH 12/41] Remove unneeded use --- tests/php/test-class-amp-cli-validation-command.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index 87bfa8ddd19..e28f50d1436 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -6,7 +6,6 @@ */ use AmpProject\AmpWP\Option; -use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; use AmpProject\AmpWP\Tests\Helpers\PrivateAccess; use AmpProject\AmpWP\Tests\Helpers\ValidationRequestMocking; @@ -19,7 +18,6 @@ */ class Test_AMP_CLI_Validation_Command extends \WP_UnitTestCase { - use AssertContainsCompatibility; use PrivateAccess; /** From 14c4d628d5599fa8c992e8b725b287b96bb75389 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 7 Oct 2020 09:37:05 -0500 Subject: [PATCH 13/41] Remove test code --- includes/cli/class-amp-cli-validation-command.php | 8 ++++---- tests/php/src/Validation/ScannableURLProviderTest.php | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 1f73eefced9..95824cd6628 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -204,9 +204,9 @@ private function get_validation_url_provider() { return $this->validation_url_provider; } - $include_conditionals = ''; //WP_CLI\Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); - $force_crawl_urls = false; //WP_CLI\Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); - $limit_type_validate_count = 100; //WP_CLI\Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); + $include_conditionals = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); + $force_crawl_urls = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); + $limit_type_validate_count = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); /* * Handle the argument and flag passed to the command: --include and --force. @@ -218,7 +218,7 @@ private function get_validation_url_provider() { $include_conditionals = explode( ',', $include_conditionals ); } - $force_crawl_urls = true; + $force_crawl_urls = true; } // Handle special case for Legacy Reader mode. diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index a4a748d0cd2..5a68285544f 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -385,7 +385,7 @@ public function test_get_date_page() { // If $include_conditionals has is_date, this should return a URL. $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_date' ] ); - $parsed_page_url = wp_parse_url( $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + $parsed_page_url = wp_parse_url( $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); $this->assertStringContains( $year, $parsed_page_url['query'] ); $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } From 60db6fa58cb679641c9ef41640b2bce249378742 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 7 Oct 2020 18:57:06 +0200 Subject: [PATCH 14/41] Try importing WP_CLI\Utils --- includes/cli/class-amp-cli-validation-command.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 95824cd6628..d4363d16813 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -11,6 +11,7 @@ use AmpProject\AmpWP\Option; use AmpProject\AmpWP\Validation\URLValidationProvider; use AmpProject\AmpWP\Validation\ScannableURLProvider; +use WP_CLI\Utils; /** * Crawls the site for validation errors or resets the stored validation errors. @@ -204,9 +205,9 @@ private function get_validation_url_provider() { return $this->validation_url_provider; } - $include_conditionals = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ); - $force_crawl_urls = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::FLAG_NAME_FORCE_VALIDATION, false ); - $limit_type_validate_count = WP_CLI\Utils\get_flag_value( $this->assoc_args, self::LIMIT_URLS_ARGUMENT, 100 ); + $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. From 2ccffb3de27ec8e6c866c795af6e1a4d9d81e963 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 19 Oct 2020 11:19:37 -0500 Subject: [PATCH 15/41] Include cli utils in autoload-dev --- composer.json | 1 + 1 file changed, 1 insertion(+) diff --git a/composer.json b/composer.json index 569daf6e14a..b5048326d14 100644 --- a/composer.json +++ b/composer.json @@ -84,6 +84,7 @@ "tests/php/validation/" ], "files": [ + "vendor/wp-cli/wp-cli/php/utils.php", "tests/php/register-wp-cli-commands.php", "docs/includes/register-wp-cli-commands.php" ] From b5693418e551be6db222f40c0a22c64b6ed9645e Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 21 Oct 2020 08:18:22 -0500 Subject: [PATCH 16/41] Remove cli utils from autoload-dev --- composer.json | 1 - 1 file changed, 1 deletion(-) diff --git a/composer.json b/composer.json index 4c840811eff..985aebfe92f 100644 --- a/composer.json +++ b/composer.json @@ -85,7 +85,6 @@ "tests/php/validation/" ], "files": [ - "vendor/wp-cli/wp-cli/php/utils.php", "tests/php/register-wp-cli-commands.php", "docs/includes/register-wp-cli-commands.php" ] From e7c12f2162a7c15836a3f1d63950e327c4d45c49 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 21 Oct 2020 10:16:40 -0500 Subject: [PATCH 17/41] Fix with_lock unit tests --- .../Validation/URLValidationProviderTest.php | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 1728ec09035..1c7f7b438f8 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -73,12 +73,12 @@ public function test_get_url_validation() { * @covers ::with_lock */ public function test_locking() { - $this->assertFalse( get_transient( URLValidationProvider::LOCK_KEY ) ); + $this->assertFalse( get_option( URLValidationProvider::LOCK_KEY ) ); $expected_result = 'EXPECTED RESULT'; $result = $this->validation_provider->with_lock( function () use ( $expected_result ) { - $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_KEY ) ); + $this->assertTrue( (bool) get_option( URLValidationProvider::LOCK_KEY ) ); // Expect an error when lock is already in place. $this->assertWPError( $this->validation_provider->with_lock( '__return_empty_string' ) ); @@ -88,16 +88,7 @@ function () use ( $expected_result ) { ); $this->assertEquals( $expected_result, $result ); - $this->assertFalse( get_transient( URLValidationProvider::LOCK_KEY ) ); - - // Test with_lock with no return value. - $this->assertNull( - $this->validation_provider->with_lock( - function () { - $this->assertEquals( 'locked', get_transient( URLValidationProvider::LOCK_KEY ) ); - } - ) - ); - $this->assertFalse( get_transient( URLValidationProvider::LOCK_KEY ) ); + + $this->assertFalse( get_option( URLValidationProvider::LOCK_KEY ) ); } } From 5ee7d42229048c3b384d56cc793392d61c82c73a Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 28 Oct 2020 10:14:15 -0500 Subject: [PATCH 18/41] Apply suggestions from code review Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com> --- src/Validation/ScannableURLProvider.php | 4 ++-- src/Validation/URLValidationProvider.php | 2 +- tests/php/src/Validation/ScannableURLProviderTest.php | 6 +++--- tests/php/src/Validation/URLValidationProviderTest.php | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 28abbe73431..0a71c926a61 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -44,7 +44,7 @@ final class ScannableURLProvider { private $limit_per_type; /** - * Class construÎctor. + * Class constructor. * * @param integer $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. @@ -85,7 +85,7 @@ public function get_urls( $offset = 0 ) { get_taxonomies( [ 'public' => true ] ), [ $this, 'does_taxonomy_support_amp' ] ); - $public_post_types = get_post_types( [ 'public' => true ], 'names' ); + $public_post_types = get_post_types( [ 'public' => true ] ); // Include one URL of each template/content type, then another URL of each type on the next iteration. for ( $i = $offset; $i < $this->limit_per_type + $offset; $i++ ) { diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 75bcb71d84c..fb16f6268ba 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -99,7 +99,7 @@ private function unlock() { * @return boolean */ public function is_locked() { - $lock_time = intval( get_option( self::LOCK_KEY, 0 ) ); + $lock_time = (int) get_option( self::LOCK_KEY, 0 ); // It's locked if the difference between the lock time and the current time is less than the lockout time. return time() - $lock_time < $this->get_lock_timeout(); diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 5a68285544f..a9b97e4dfaf 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -13,7 +13,7 @@ use WP_Query; use WP_UnitTestCase; -/** @coversDefaultClass ScannableURLProvider */ +/** @coversDefaultClass \AmpProject\AmpWP\Validation\ScannableURLProvider */ final class ScannableURLProviderTest extends WP_UnitTestCase { use PrivateAccess, AssertContainsCompatibility; @@ -356,7 +356,7 @@ public function test_get_taxonomy_links() { */ public function test_get_search_page() { // 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->validation_url_provider, 'get_search_page' ) ) ); + $this->assertIsString( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); // If $include_conditionals is set and does not have is_search, this should not return a URL. $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_author' ] ); @@ -364,7 +364,7 @@ public function test_get_search_page() { // If $include_conditionals has is_search, this should return a URL. $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_search' ] ); - $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); + $this->assertIsString( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 1c7f7b438f8..76f7f8db64e 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -7,7 +7,7 @@ use AmpProject\AmpWP\Validation\URLValidationProvider; use WP_UnitTestCase; -/** @coversDefaultClass URLValidationProvider */ +/** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationProvider */ final class URLValidationProviderTest extends WP_UnitTestCase { use AssertContainsCompatibility; From 58bacd0893fd99630ab924fbb290303285f77c50 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 28 Oct 2020 10:54:54 -0500 Subject: [PATCH 19/41] Replace obsolete class property with get_flag_value call --- includes/cli/class-amp-cli-validation-command.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index d4363d16813..b4a6481736c 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -116,7 +116,7 @@ public function run( $args, $assoc_args ) { $number_urls_to_crawl = count( $validation_url_provider->get_urls() ); if ( ! $number_urls_to_crawl ) { - if ( ! empty( $this->include_conditionals ) ) { + if ( ! empty( Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ) ) ) { WP_CLI::error( sprintf( 'The templates passed via the --%s argument did not match any URLs. You might try passing different templates to it.', From 050b1e74ccbb8e062edcaeaa0cd7c5a039715cb8 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 28 Oct 2020 10:57:44 -0500 Subject: [PATCH 20/41] Make validity_by_type class property private and add getter method --- includes/cli/class-amp-cli-validation-command.php | 2 +- src/Validation/URLValidationProvider.php | 11 ++++++++++- .../php/src/Validation/URLValidationProviderTest.php | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index b4a6481736c..70055d272a7 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -158,7 +158,7 @@ function () { $key_validity_rate = 'Validity Rate'; $table_validation_by_type = []; - foreach ( $validation_provider->validity_by_type as $type_name => $validity ) { + foreach ( $validation_provider->get_validity_by_type() as $type_name => $validity ) { $table_validation_by_type[] = [ $key_template_type => $type_name, $key_url_count => $validity['total'], diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index fb16f6268ba..e976a186657 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -77,7 +77,7 @@ final class URLValidationProvider { * } * } */ - public $validity_by_type = []; + private $validity_by_type = []; /** * Locks validation. @@ -167,6 +167,15 @@ public function get_number_validated() { return $this->number_validated; } + /** + * Provides the validity counts by type. + * + * @return array[] + */ + public function get_validity_by_type() { + return $this->validity_by_type; + } + /** * Validates a URL, stores the results, and increments the counts. * diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 76f7f8db64e..1a9be6911b4 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -59,7 +59,7 @@ public function test_get_url_validation() { $this->assertEquals( [ 'post' ], - array_keys( $this->validation_provider->validity_by_type ) + array_keys( $this->validation_provider->get_validity_by_type() ) ); } From 457d145f3b7193b3fd7284bc70a5126e01f65952 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 28 Oct 2020 11:07:07 -0500 Subject: [PATCH 21/41] ScannableURLProvider: return limit_per_type # of taxonomy links by default --- src/Validation/ScannableURLProvider.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Validation/ScannableURLProvider.php b/src/Validation/ScannableURLProvider.php index 0a71c926a61..c66c039b9b4 100644 --- a/src/Validation/ScannableURLProvider.php +++ b/src/Validation/ScannableURLProvider.php @@ -289,7 +289,11 @@ private function does_taxonomy_support_amp( $taxonomy ) { * @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 = 1 ) { + private function get_taxonomy_links( $taxonomy, $offset = '', $number = null ) { + if ( is_null( $number ) ) { + $number = $this->limit_per_type; + } + return array_map( 'get_term_link', get_terms( From 5893cfb6d4fd8de56fa27cb400a6ebf95bd81005 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 28 Oct 2020 11:08:07 -0500 Subject: [PATCH 22/41] Add omitted property --- tests/php/src/Validation/ScannableURLProviderTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index a9b97e4dfaf..5336fc530ce 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -22,6 +22,7 @@ final class ScannableURLProviderTest extends WP_UnitTestCase { * * @var ScannableURLProvider */ + private $validation_url_provider; /** * Setup. From 9559058fc45ad5d58475b35a8ed7d520d8076e99 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Wed, 28 Oct 2020 12:39:43 -0500 Subject: [PATCH 23/41] Undo use of assertIsString --- tests/php/src/Validation/ScannableURLProviderTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 5336fc530ce..27f72c4e84a 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -357,7 +357,7 @@ public function test_get_taxonomy_links() { */ public function test_get_search_page() { // Normally, this should return a string, unless the user has opted out of the search template. - $this->assertIsString( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); + $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); // If $include_conditionals is set and does not have is_search, this should not return a URL. $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_author' ] ); @@ -365,7 +365,7 @@ public function test_get_search_page() { // If $include_conditionals has is_search, this should return a URL. $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_search' ] ); - $this->assertIsString( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); + $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); } From d1e3bdedf8346296e2abb755d1f1b07261aac3ce Mon Sep 17 00:00:00 2001 From: John Watkins Date: Thu, 29 Oct 2020 10:11:11 -0500 Subject: [PATCH 24/41] PHPStan ignore a line --- includes/cli/class-amp-cli-validation-command.php | 1 + 1 file changed, 1 insertion(+) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 70055d272a7..4a791d5976e 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -116,6 +116,7 @@ public function run( $args, $assoc_args ) { $number_urls_to_crawl = count( $validation_url_provider->get_urls() ); if ( ! $number_urls_to_crawl ) { + /** @phpstan-ignore-next-line */ if ( ! empty( Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ) ) ) { WP_CLI::error( sprintf( From fbc39a63abdeaa0034a3424da15c1a9263a1f52d Mon Sep 17 00:00:00 2001 From: John Watkins Date: Thu, 29 Oct 2020 10:16:55 -0500 Subject: [PATCH 25/41] Update lock file --- composer.lock | 121 ++++++++++++++++++++++++-------------------------- 1 file changed, 59 insertions(+), 62 deletions(-) diff --git a/composer.lock b/composer.lock index f3449797701..d1b4a010d0c 100644 --- a/composer.lock +++ b/composer.lock @@ -477,16 +477,16 @@ }, { "name": "composer/semver", - "version": "3.2.1", + "version": "3.2.2", "source": { "type": "git", "url": "https://github.com/composer/semver.git", - "reference": "ebb714493b3a54f1dbbec6b15ab7bc9b3440e17b" + "reference": "4089fddb67bcf6bf860d91b979e95be303835002" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/composer/semver/zipball/ebb714493b3a54f1dbbec6b15ab7bc9b3440e17b", - "reference": "ebb714493b3a54f1dbbec6b15ab7bc9b3440e17b", + "url": "https://api.github.com/repos/composer/semver/zipball/4089fddb67bcf6bf860d91b979e95be303835002", + "reference": "4089fddb67bcf6bf860d91b979e95be303835002", "shasum": "" }, "require": { @@ -549,7 +549,7 @@ "type": "tidelift" } ], - "time": "2020-09-27T13:14:03+00:00" + "time": "2020-10-14T08:51:15+00:00" }, { "name": "dealerdirect/phpcodesniffer-composer-installer", @@ -669,16 +669,16 @@ }, { "name": "google/auth", - "version": "v1.13.0", + "version": "v1.14.3", "source": { "type": "git", "url": "https://github.com/googleapis/google-auth-library-php.git", - "reference": "173191f5defd1d9ae8bdfc28da31b63eb73dd34e" + "reference": "c1503299c779af0cbc99b43788f75930988852cf" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/googleapis/google-auth-library-php/zipball/173191f5defd1d9ae8bdfc28da31b63eb73dd34e", - "reference": "173191f5defd1d9ae8bdfc28da31b63eb73dd34e", + "url": "https://api.github.com/repos/googleapis/google-auth-library-php/zipball/c1503299c779af0cbc99b43788f75930988852cf", + "reference": "c1503299c779af0cbc99b43788f75930988852cf", "shasum": "" }, "require": { @@ -717,7 +717,7 @@ "google", "oauth2" ], - "time": "2020-09-18T20:03:05+00:00" + "time": "2020-10-16T21:33:48+00:00" }, { "name": "google/cloud-core", @@ -1329,16 +1329,16 @@ }, { "name": "paragonie/random_compat", - "version": "v2.0.18", + "version": "v2.0.19", "source": { "type": "git", "url": "https://github.com/paragonie/random_compat.git", - "reference": "0a58ef6e3146256cc3dc7cc393927bcc7d1b72db" + "reference": "446fc9faa5c2a9ddf65eb7121c0af7e857295241" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/paragonie/random_compat/zipball/0a58ef6e3146256cc3dc7cc393927bcc7d1b72db", - "reference": "0a58ef6e3146256cc3dc7cc393927bcc7d1b72db", + "url": "https://api.github.com/repos/paragonie/random_compat/zipball/446fc9faa5c2a9ddf65eb7121c0af7e857295241", + "reference": "446fc9faa5c2a9ddf65eb7121c0af7e857295241", "shasum": "" }, "require": { @@ -1374,7 +1374,7 @@ "pseudorandom", "random" ], - "time": "2019-01-03T20:59:08+00:00" + "time": "2020-10-15T10:06:57+00:00" }, { "name": "php-stubs/wordpress-stubs", @@ -1961,12 +1961,12 @@ "source": { "type": "git", "url": "https://github.com/Roave/SecurityAdvisories.git", - "reference": "bf103ea36b619559ced328ebf6c2f099c2585e59" + "reference": "327370943772f9917bc2dc2aa4263db2d572a112" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/Roave/SecurityAdvisories/zipball/bf103ea36b619559ced328ebf6c2f099c2585e59", - "reference": "bf103ea36b619559ced328ebf6c2f099c2585e59", + "url": "https://api.github.com/repos/Roave/SecurityAdvisories/zipball/327370943772f9917bc2dc2aa4263db2d572a112", + "reference": "327370943772f9917bc2dc2aa4263db2d572a112", "shasum": "" }, "conflict": { @@ -2066,6 +2066,7 @@ "magento/magento1ee": ">=1,<1.14.4.3", "magento/product-community-edition": ">=2,<2.2.10|>=2.3,<2.3.2-p.2", "marcwillmann/turn": "<0.3.3", + "mediawiki/core": ">=1.31,<1.31.4|>=1.32,<1.32.4|>=1.33,<1.33.1", "mittwald/typo3_forum": "<1.2.1", "monolog/monolog": ">=1.8,<1.12", "namshi/jose": "<2.2", @@ -2106,6 +2107,7 @@ "privatebin/privatebin": "<1.2.2|>=1.3,<1.3.2", "propel/propel": ">=2-alpha.1,<=2-alpha.7", "propel/propel1": ">=1,<=1.7.1", + "pterodactyl/panel": "<0.7.19|>=1-rc.0,<=1-rc.6", "pusher/pusher-php-server": "<2.2.1", "rainlab/debugbar-plugin": "<3.1", "robrichards/xmlseclibs": "<3.0.4", @@ -2261,7 +2263,7 @@ "type": "tidelift" } ], - "time": "2020-10-07T12:02:57+00:00" + "time": "2020-10-19T07:02:45+00:00" }, { "name": "sirbrillig/phpcs-variable-analysis", @@ -2313,16 +2315,16 @@ }, { "name": "squizlabs/php_codesniffer", - "version": "3.5.6", + "version": "3.5.8", "source": { "type": "git", "url": "https://github.com/squizlabs/PHP_CodeSniffer.git", - "reference": "e97627871a7eab2f70e59166072a6b767d5834e0" + "reference": "9d583721a7157ee997f235f327de038e7ea6dac4" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/squizlabs/PHP_CodeSniffer/zipball/e97627871a7eab2f70e59166072a6b767d5834e0", - "reference": "e97627871a7eab2f70e59166072a6b767d5834e0", + "url": "https://api.github.com/repos/squizlabs/PHP_CodeSniffer/zipball/9d583721a7157ee997f235f327de038e7ea6dac4", + "reference": "9d583721a7157ee997f235f327de038e7ea6dac4", "shasum": "" }, "require": { @@ -2360,31 +2362,26 @@ "phpcs", "standards" ], - "time": "2020-08-10T04:50:15+00:00" + "time": "2020-10-23T02:01:07+00:00" }, { "name": "symfony/finder", - "version": "v3.4.45", + "version": "v3.4.46", "source": { "type": "git", "url": "https://github.com/symfony/finder.git", - "reference": "52140652ed31cee3dabd0c481b5577201fa769b4" + "reference": "4e1da3c110c52d868f8a9153b7de3ebc381fba78" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/finder/zipball/52140652ed31cee3dabd0c481b5577201fa769b4", - "reference": "52140652ed31cee3dabd0c481b5577201fa769b4", + "url": "https://api.github.com/repos/symfony/finder/zipball/4e1da3c110c52d868f8a9153b7de3ebc381fba78", + "reference": "4e1da3c110c52d868f8a9153b7de3ebc381fba78", "shasum": "" }, "require": { "php": "^5.5.9|>=7.0.8" }, "type": "library", - "extra": { - "branch-alias": { - "dev-master": "3.4-dev" - } - }, "autoload": { "psr-4": { "Symfony\\Component\\Finder\\": "" @@ -2423,20 +2420,20 @@ "type": "tidelift" } ], - "time": "2020-09-02T16:06:40+00:00" + "time": "2020-10-24T10:57:07+00:00" }, { "name": "symfony/polyfill-intl-idn", - "version": "v1.18.1", + "version": "v1.19.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-intl-idn.git", - "reference": "5dcab1bc7146cf8c1beaa4502a3d9be344334251" + "reference": "4ad5115c0f5d5172a9fe8147675ec6de266d8826" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-intl-idn/zipball/5dcab1bc7146cf8c1beaa4502a3d9be344334251", - "reference": "5dcab1bc7146cf8c1beaa4502a3d9be344334251", + "url": "https://api.github.com/repos/symfony/polyfill-intl-idn/zipball/4ad5115c0f5d5172a9fe8147675ec6de266d8826", + "reference": "4ad5115c0f5d5172a9fe8147675ec6de266d8826", "shasum": "" }, "require": { @@ -2451,7 +2448,7 @@ "type": "library", "extra": { "branch-alias": { - "dev-master": "1.18-dev" + "dev-main": "1.19-dev" }, "thanks": { "name": "symfony/polyfill", @@ -2508,20 +2505,20 @@ "type": "tidelift" } ], - "time": "2020-08-04T06:02:08+00:00" + "time": "2020-10-21T09:57:48+00:00" }, { "name": "symfony/polyfill-intl-normalizer", - "version": "v1.18.1", + "version": "v1.19.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-intl-normalizer.git", - "reference": "37078a8dd4a2a1e9ab0231af7c6cb671b2ed5a7e" + "reference": "8db0ae7936b42feb370840cf24de1a144fb0ef27" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-intl-normalizer/zipball/37078a8dd4a2a1e9ab0231af7c6cb671b2ed5a7e", - "reference": "37078a8dd4a2a1e9ab0231af7c6cb671b2ed5a7e", + "url": "https://api.github.com/repos/symfony/polyfill-intl-normalizer/zipball/8db0ae7936b42feb370840cf24de1a144fb0ef27", + "reference": "8db0ae7936b42feb370840cf24de1a144fb0ef27", "shasum": "" }, "require": { @@ -2533,7 +2530,7 @@ "type": "library", "extra": { "branch-alias": { - "dev-master": "1.18-dev" + "dev-main": "1.19-dev" }, "thanks": { "name": "symfony/polyfill", @@ -2589,20 +2586,20 @@ "type": "tidelift" } ], - "time": "2020-07-14T12:35:20+00:00" + "time": "2020-10-23T09:01:57+00:00" }, { "name": "symfony/polyfill-php70", - "version": "v1.18.1", + "version": "v1.19.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-php70.git", - "reference": "0dd93f2c578bdc9c72697eaa5f1dd25644e618d3" + "reference": "3fe414077251a81a1b15b1c709faf5c2fbae3d4e" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-php70/zipball/0dd93f2c578bdc9c72697eaa5f1dd25644e618d3", - "reference": "0dd93f2c578bdc9c72697eaa5f1dd25644e618d3", + "url": "https://api.github.com/repos/symfony/polyfill-php70/zipball/3fe414077251a81a1b15b1c709faf5c2fbae3d4e", + "reference": "3fe414077251a81a1b15b1c709faf5c2fbae3d4e", "shasum": "" }, "require": { @@ -2612,7 +2609,7 @@ "type": "library", "extra": { "branch-alias": { - "dev-master": "1.18-dev" + "dev-main": "1.19-dev" }, "thanks": { "name": "symfony/polyfill", @@ -2666,20 +2663,20 @@ "type": "tidelift" } ], - "time": "2020-07-14T12:35:20+00:00" + "time": "2020-10-23T09:01:57+00:00" }, { "name": "symfony/polyfill-php72", - "version": "v1.18.1", + "version": "v1.19.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-php72.git", - "reference": "639447d008615574653fb3bc60d1986d7172eaae" + "reference": "beecef6b463b06954638f02378f52496cb84bacc" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-php72/zipball/639447d008615574653fb3bc60d1986d7172eaae", - "reference": "639447d008615574653fb3bc60d1986d7172eaae", + "url": "https://api.github.com/repos/symfony/polyfill-php72/zipball/beecef6b463b06954638f02378f52496cb84bacc", + "reference": "beecef6b463b06954638f02378f52496cb84bacc", "shasum": "" }, "require": { @@ -2688,7 +2685,7 @@ "type": "library", "extra": { "branch-alias": { - "dev-master": "1.18-dev" + "dev-main": "1.19-dev" }, "thanks": { "name": "symfony/polyfill", @@ -2739,7 +2736,7 @@ "type": "tidelift" } ], - "time": "2020-07-14T12:35:20+00:00" + "time": "2020-10-23T09:01:57+00:00" }, { "name": "togos/gitignore", @@ -3028,12 +3025,12 @@ "source": { "type": "git", "url": "https://github.com/wp-cli/wp-cli.git", - "reference": "d394b395995e70932df5a58baab18f2942afbc28" + "reference": "7d1e93d39dd5d322aa539e406a3ddfd1d9aa96ce" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/wp-cli/wp-cli/zipball/d394b395995e70932df5a58baab18f2942afbc28", - "reference": "d394b395995e70932df5a58baab18f2942afbc28", + "url": "https://api.github.com/repos/wp-cli/wp-cli/zipball/7d1e93d39dd5d322aa539e406a3ddfd1d9aa96ce", + "reference": "7d1e93d39dd5d322aa539e406a3ddfd1d9aa96ce", "shasum": "" }, "require": { @@ -3086,7 +3083,7 @@ "cli", "wordpress" ], - "time": "2020-10-09T07:57:08+00:00" + "time": "2020-10-27T08:33:01+00:00" }, { "name": "wp-coding-standards/wpcs", From 3c7d7d79807625cdfcee131ff12889590cf4c785 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 16:17:03 -0600 Subject: [PATCH 26/41] Simplify test assertion Co-authored-by: Alain Schlesser --- tests/php/test-class-amp-cli-validation-command.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index e28f50d1436..0f7c8fe9f1c 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -75,6 +75,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->assertTrue( in_array( home_url( '/' ), ValidationRequestMocking::get_validated_urls(), true ) ); + $this->assertContains( home_url('/'), ValidationRequestMocking::get_validated_urls() ); } } From e70f81696cba496eddb34acb3f532ea752878cc0 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 16:34:18 -0600 Subject: [PATCH 27/41] Improve coverage with more @covers comments --- tests/php/src/Validation/ScannableURLProviderTest.php | 5 +++++ tests/php/src/Validation/URLValidationProviderTest.php | 1 + tests/php/test-class-amp-cli-validation-command.php | 4 +++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 27f72c4e84a..9078b8ed3b0 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -35,6 +35,11 @@ public function setUp() { add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } + /** @covers ::__construct */ + public function test__construct() { + $this->assertInstanceOf( ScannableURLProvider::class, $this->validation_url_provider ); + } + /** * Test retrieval of urls. * diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 1a9be6911b4..810695e48c2 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -35,6 +35,7 @@ public function setUp() { * @covers ::get_url_validation() * @covers ::get_total_errors() * @covers ::get_unaccepted_errors() + * @covers ::update_state_from_validity */ public function test_get_url_validation() { $single_post_permalink = get_permalink( self::factory()->post->create() ); diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index 0f7c8fe9f1c..a6c6ffd9e01 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -43,7 +43,9 @@ public function setUp() { /** * Test validate_urls. * - * @covers ::validate_urls() + * @covers ::validate_urls + * @covers ::get_validation_provider + * @covers ::get_validation_url_provider */ public function test_validate_urls() { $number_of_posts = 20; From c4899a7d7e916ee95f6daf1ed33aceb32f57cfb0 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 16:44:41 -0600 Subject: [PATCH 28/41] Update covers comment style --- .../php/src/Validation/URLValidationProviderTest.php | 12 ++++++------ tests/php/test-class-amp-cli-validation-command.php | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 810695e48c2..3d48540d583 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -35,7 +35,7 @@ public function setUp() { * @covers ::get_url_validation() * @covers ::get_total_errors() * @covers ::get_unaccepted_errors() - * @covers ::update_state_from_validity + * @covers ::update_state_from_validity() */ public function test_get_url_validation() { $single_post_permalink = get_permalink( self::factory()->post->create() ); @@ -67,11 +67,11 @@ public function test_get_url_validation() { /** * Tests locking and unlocking. * - * @covers ::lock - * @covers ::unlock - * @covers ::is_locked - * @covers ::get_lock_timeout - * @covers ::with_lock + * @covers ::lock() + * @covers ::unlock() + * @covers ::is_locked() + * @covers ::get_lock_timeout() + * @covers ::with_lock() */ public function test_locking() { $this->assertFalse( get_option( URLValidationProvider::LOCK_KEY ) ); diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index a6c6ffd9e01..c1ff32d54c4 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -43,9 +43,9 @@ public function setUp() { /** * Test validate_urls. * - * @covers ::validate_urls - * @covers ::get_validation_provider - * @covers ::get_validation_url_provider + * @covers ::validate_urls() + * @covers ::get_validation_provider() + * @covers ::get_validation_url_provider() */ public function test_validate_urls() { $number_of_posts = 20; From 6c449c04cf81c7c1b7228e07ed881922a68802a7 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 16:53:59 -0600 Subject: [PATCH 29/41] PHPCS fix --- tests/php/test-class-amp-cli-validation-command.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index c1ff32d54c4..4a13af9d0c6 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -77,6 +77,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->assertContains( home_url('/'), ValidationRequestMocking::get_validated_urls() ); + $this->assertContains( home_url( '/' ), ValidationRequestMocking::get_validated_urls() ); } } From 4c0238d76519abb66c09300c089cf05014838722 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 20:35:11 -0600 Subject: [PATCH 30/41] Rename class properties to match the classes they're instances of --- .../cli/class-amp-cli-validation-command.php | 44 +++---- .../Validation/ScannableURLProviderTest.php | 118 +++++++++--------- .../Validation/URLValidationProviderTest.php | 20 +-- 3 files changed, 91 insertions(+), 91 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 4a791d5976e..3a1d46040a9 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -67,14 +67,14 @@ final class AMP_CLI_Validation_Command { * * @var URLValidationProvider */ - private $validation_provider; + private $url_validation_provider; /** * ScannableURLProvider instance. * * @var ScannableURLProvider */ - private $validation_url_provider; + private $scannable_url_provider; /** * Associative args passed to the command. @@ -111,10 +111,10 @@ final class AMP_CLI_Validation_Command { public function run( $args, $assoc_args ) { $this->assoc_args = $assoc_args; - $validation_url_provider = $this->get_validation_url_provider(); - $validation_provider = $this->get_validation_provider(); + $scannable_url_provider = $this->get_validation_url_provider(); + $url_validation_provider = $this->get_validation_provider(); - $number_urls_to_crawl = count( $validation_url_provider->get_urls() ); + $number_urls_to_crawl = count( $scannable_url_provider->get_urls() ); if ( ! $number_urls_to_crawl ) { /** @phpstan-ignore-next-line */ if ( ! empty( Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ) ) ) { @@ -141,7 +141,7 @@ public function run( $args, $assoc_args ) { $number_urls_to_crawl ); - $result = $validation_provider->with_lock( + $result = $url_validation_provider->with_lock( function () { $this->validate_urls(); } @@ -159,7 +159,7 @@ function () { $key_validity_rate = 'Validity Rate'; $table_validation_by_type = []; - foreach ( $validation_provider->get_validity_by_type() as $type_name => $validity ) { + foreach ( $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'], @@ -175,9 +175,9 @@ function () { 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.', - $validation_provider->get_number_validated(), - $validation_provider->get_total_errors(), - $validation_provider->get_unaccepted_errors() + $url_validation_provider->get_number_validated(), + $url_validation_provider->get_total_errors(), + $url_validation_provider->get_unaccepted_errors() ) ); @@ -202,8 +202,8 @@ function () { * @return ScannableURLProvider */ private function get_validation_url_provider() { - if ( ! is_null( $this->validation_url_provider ) ) { - return $this->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, [] ); @@ -247,13 +247,13 @@ private function get_validation_url_provider() { } } - $this->validation_url_provider = new ScannableURLProvider( + $this->scannable_url_provider = new ScannableURLProvider( $limit_type_validate_count, $include_conditionals, $force_crawl_urls ); - return $this->validation_url_provider; + return $this->scannable_url_provider; } /** @@ -262,24 +262,24 @@ private function get_validation_url_provider() { * @return URLValidationProvider */ private function get_validation_provider() { - if ( ! is_null( $this->validation_provider ) ) { - return $this->validation_provider; + if ( ! is_null( $this->url_validation_provider ) ) { + return $this->url_validation_provider; } - $this->validation_provider = new URLValidationProvider(); + $this->url_validation_provider = new URLValidationProvider(); - return $this->validation_provider; + return $this->url_validation_provider; } /** * Validates the URLs. */ private function validate_urls() { - $validation_url_provider = $this->get_validation_url_provider(); - $validation_provider = $this->get_validation_provider(); + $scannable_url_provider = $this->get_validation_url_provider(); + $url_validation_provider = $this->get_validation_provider(); - foreach ( $validation_url_provider->get_urls() as $url ) { - $validity = $validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); + foreach ( $scannable_url_provider->get_urls() as $url ) { + $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); if ( $this->wp_cli_progress ) { $this->wp_cli_progress->tick(); diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 9078b8ed3b0..89eb0c441ad 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -22,7 +22,7 @@ final class ScannableURLProviderTest extends WP_UnitTestCase { * * @var ScannableURLProvider */ - private $validation_url_provider; + private $scannable_url_provider; /** * Setup. @@ -31,13 +31,13 @@ final class ScannableURLProviderTest extends WP_UnitTestCase { */ public function setUp() { parent::setUp(); - $this->validation_url_provider = new ScannableURLProvider(); + $this->scannable_url_provider = new ScannableURLProvider(); add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } /** @covers ::__construct */ public function test__construct() { - $this->assertInstanceOf( ScannableURLProvider::class, $this->validation_url_provider ); + $this->assertInstanceOf( ScannableURLProvider::class, $this->scannable_url_provider ); } /** @@ -48,9 +48,9 @@ public function test__construct() { public function test_count_urls_to_validate() { $number_original_urls = 4; - $this->assertCount( $number_original_urls, $this->validation_url_provider->get_urls() ); + $this->assertCount( $number_original_urls, $this->scannable_url_provider->get_urls() ); - $this->validation_url_provider = new ScannableURLProvider( 100 ); + $this->scannable_url_provider = new ScannableURLProvider( 100 ); $category = self::factory()->term->create( [ 'taxonomy' => 'category' ] ); $number_new_posts = 50; @@ -68,9 +68,9 @@ public function test_count_urls_to_validate() { * And ensure that the tested method finds a URL for all of them. */ $expected_url_count = $number_new_posts + $number_original_urls + 1; - $this->assertCount( $expected_url_count, $this->validation_url_provider->get_urls() ); + $this->assertCount( $expected_url_count, $this->scannable_url_provider->get_urls() ); - $this->validation_url_provider = new ScannableURLProvider( 100 ); + $this->scannable_url_provider = new ScannableURLProvider( 100 ); $number_of_new_terms = 20; $expected_url_count += $number_of_new_terms; @@ -92,7 +92,7 @@ public function test_count_urls_to_validate() { ); $this->assertFalse( is_wp_error( $result ) ); - $this->assertCount( $expected_url_count, $this->validation_url_provider->get_urls() ); + $this->assertCount( $expected_url_count, $this->scannable_url_provider->get_urls() ); } /** @@ -108,7 +108,7 @@ public function test_get_posts_that_support_amp() { } // This should count all of the newly-created posts as supporting AMP. - $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + $this->assertEquals( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); // Simulate 'Enable AMP' being unchecked in the post editor, in which case get_url_count() should not count it. $first_id = $ids[0]; @@ -117,7 +117,7 @@ public function test_get_posts_that_support_amp() { AMP_Post_Meta_Box::STATUS_POST_META_KEY, AMP_Post_Meta_Box::DISABLED_STATUS ); - $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ [ $first_id ] ] ) ); + $this->assertEquals( [], $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ [ $first_id ] ] ) ); update_post_meta( $first_id, @@ -126,13 +126,13 @@ public function test_get_posts_that_support_amp() { ); // When the second $force_count_all_urls argument is true, all of the newly-created posts should be part of the URL count. - $this->set_private_property( $this->validation_url_provider, 'include_unsupported', true ); - $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); - $this->set_private_property( $this->validation_url_provider, 'include_unsupported', false ); + $this->set_private_property( $this->scannable_url_provider, 'include_unsupported', true ); + $this->assertEquals( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_unsupported', false ); // 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 ); - $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + $this->assertEquals( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); // In Transitional Mode, the IDs should also include all of the newly-created posts. add_theme_support( @@ -141,22 +141,22 @@ public function test_get_posts_that_support_amp() { AMP_Theme_Support::PAIRED_FLAG => true, ] ); - $this->assertEquals( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + $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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_tag', 'is_category' ] ); - $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ '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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_singular', 'is_category' ] ); - $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->validation_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ) ); - $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ 'is_singular', 'is_category' ] ); + $this->assertEmpty( array_diff( $ids, $this->call_private_method( $this->scannable_url_provider, 'get_posts_that_support_amp', [ $ids ] ) ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [] ); } /** @@ -172,27 +172,27 @@ 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->validation_url_provider, 'get_author_page_urls', [ 0, 1 ] ); + $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. $this->assertEquals( [ $first_author_url ], $actual_urls ); - $actual_urls = $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls', [ 1, 1 ] ); + $actual_urls = $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls', [ 1, 1 ] ); // Passing 1 as the offset argument should get the second author. $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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_category' ] ); - $this->assertEquals( [], $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls' ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ '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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_author' ] ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ 'is_author' ] ); $this->assertEquals( [ $first_author_url, $second_author_url ], - $this->call_private_method( $this->validation_url_provider, 'get_author_page_urls' ) + $this->call_private_method( $this->scannable_url_provider, 'get_author_page_urls' ) ); - $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [] ); } /** @@ -208,27 +208,27 @@ public function test_does_taxonomy_support_amp() { // When these templates are not unchecked in the 'AMP Settings' UI, these should be supported. foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); } // When the user has not checked the boxes for 'Categories' and 'Tags,' this should be false. 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->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + $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->set_private_property( $this->validation_url_provider, 'include_unsupported', true ); + $this->set_private_property( $this->scannable_url_provider, 'include_unsupported', true ); foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); } - $this->set_private_property( $this->validation_url_provider, 'include_unsupported', false ); + $this->set_private_property( $this->scannable_url_provider, 'include_unsupported', false ); // 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 ); foreach ( $taxonomies_to_test as $taxonomy ) { - $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); + $this->assertTrue( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ $taxonomy ] ) ); } AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); @@ -236,12 +236,12 @@ 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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_category', 'is_tag' ] ); - $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'category' ] ) ); - $this->assertTrue( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'tag' ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'author' ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'does_taxonomy_support_amp', [ 'search' ] ) ); - $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ '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' ] ) ); + $this->assertFalse( $this->call_private_method( $this->scannable_url_provider, 'does_taxonomy_support_amp', [ 'search' ] ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [] ); } /** @@ -255,12 +255,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->assertTrue( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $author_conditional ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $search_conditional ] ) ); + $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->assertTrue( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $search_conditional ] ) ); - $this->assertFalse( $this->call_private_method( $this->validation_url_provider, 'is_template_supported', [ $author_conditional ] ) ); + $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 ] ) ); } /** @@ -293,12 +293,12 @@ public function test_get_posts_by_type() { ); } - $actual_posts = $this->call_private_method( $this->validation_url_provider, 'get_posts_by_type', [ $post_type ] ); + $actual_posts = $this->call_private_method( $this->scannable_url_provider, 'get_posts_by_type', [ $post_type ] ); $this->assertEquals( $expected_posts, array_values( $actual_posts ) ); // Test with the $offset and $number arguments. $offset = 0; - $actual_posts = $this->call_private_method( $this->validation_url_provider, 'get_posts_by_type', [ $post_type, $offset, $number_posts_each_post_type ] ); + $actual_posts = $this->call_private_method( $this->scannable_url_provider, 'get_posts_by_type', [ $post_type, $offset, $number_posts_each_post_type ] ); $this->assertEquals( array_slice( $expected_posts, $offset, $number_posts_each_post_type ), $actual_posts ); } } @@ -341,7 +341,7 @@ public function test_get_taxonomy_links() { array_map( 'get_term_link', $terms_for_current_taxonomy ) ); $number_of_links = 100; - $actual_links = $this->call_private_method( $this->validation_url_provider, 'get_taxonomy_links', [ $taxonomy, 0, $number_of_links ] ); + $actual_links = $this->call_private_method( $this->scannable_url_provider, 'get_taxonomy_links', [ $taxonomy, 0, $number_of_links ] ); // The get_terms() call in get_taxonomy_links() returns an array with a first index of 1, so correct for that with array_values(). $this->assertEquals( $expected_links, array_values( $actual_links ) ); @@ -349,7 +349,7 @@ public function test_get_taxonomy_links() { $number_of_links = 5; $offset = 10; - $actual_links_using_offset = $this->call_private_method( $this->validation_url_provider, 'get_taxonomy_links', [ $taxonomy, $offset, $number_of_links ] ); + $actual_links_using_offset = $this->call_private_method( $this->scannable_url_provider, 'get_taxonomy_links', [ $taxonomy, $offset, $number_of_links ] ); $this->assertEquals( array_slice( $expected_links, $offset, $number_of_links ), array_values( $actual_links_using_offset ) ); $this->assertEquals( $number_of_links, count( $actual_links_using_offset ) ); } @@ -362,16 +362,16 @@ public function test_get_taxonomy_links() { */ public function test_get_search_page() { // 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->validation_url_provider, '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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_author' ] ); - $this->assertEquals( null, $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ '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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_search' ] ); - $this->assertTrue( is_string( $this->call_private_method( $this->validation_url_provider, 'get_search_page' ) ) ); - $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ 'is_search' ] ); + $this->assertTrue( is_string( $this->call_private_method( $this->scannable_url_provider, 'get_search_page' ) ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [] ); } /** @@ -383,16 +383,16 @@ public function test_get_date_page() { $year = gmdate( 'Y' ); // Normally, this should return the date page, unless the user has opted out of that template. - $this->assertStringContains( $year, $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + $this->assertStringContains( $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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_search' ] ); - $this->assertEquals( null, $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ '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->set_private_property( $this->validation_url_provider, 'include_conditionals', [ 'is_date' ] ); - $parsed_page_url = wp_parse_url( $this->call_private_method( $this->validation_url_provider, 'get_date_page' ) ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [ 'is_date' ] ); + $parsed_page_url = wp_parse_url( $this->call_private_method( $this->scannable_url_provider, 'get_date_page' ) ); $this->assertStringContains( $year, $parsed_page_url['query'] ); - $this->set_private_property( $this->validation_url_provider, 'include_conditionals', [] ); + $this->set_private_property( $this->scannable_url_provider, 'include_conditionals', [] ); } } diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 3d48540d583..48719e10e24 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -16,7 +16,7 @@ final class URLValidationProviderTest extends WP_UnitTestCase { * * @var URLValidationProvider */ - private $validation_provider; + private $url_validation_provider; /** * Setup. @@ -25,7 +25,7 @@ final class URLValidationProviderTest extends WP_UnitTestCase { */ public function setUp() { parent::setUp(); - $this->validation_provider = new URLValidationProvider(); + $this->url_validation_provider = new URLValidationProvider(); add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } @@ -39,7 +39,7 @@ public function setUp() { */ public function test_get_url_validation() { $single_post_permalink = get_permalink( self::factory()->post->create() ); - $this->validation_provider->get_url_validation( $single_post_permalink, 'post' ); + $this->url_validation_provider->get_url_validation( $single_post_permalink, 'post' ); $this->assertContains( $single_post_permalink, ValidationRequestMocking::get_validated_urls() ); $number_of_posts = 30; @@ -48,19 +48,19 @@ public function test_get_url_validation() { for ( $i = 0; $i < $number_of_posts; $i++ ) { $permalink = get_permalink( self::factory()->post->create() ); $post_permalinks[] = $permalink; - $this->validation_provider->get_url_validation( $permalink, 'post' ); + $this->url_validation_provider->get_url_validation( $permalink, 'post' ); } // All of the posts created should be present in the validated URLs. $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); - $this->assertEquals( 31, $this->validation_provider->get_total_errors() ); - $this->assertEmpty( $this->validation_provider->get_unaccepted_errors() ); - $this->assertEquals( 31, $this->validation_provider->get_number_validated() ); + $this->assertEquals( 31, $this->url_validation_provider->get_total_errors() ); + $this->assertEmpty( $this->url_validation_provider->get_unaccepted_errors() ); + $this->assertEquals( 31, $this->url_validation_provider->get_number_validated() ); $this->assertEquals( [ 'post' ], - array_keys( $this->validation_provider->get_validity_by_type() ) + array_keys( $this->url_validation_provider->get_validity_by_type() ) ); } @@ -77,12 +77,12 @@ public function test_locking() { $this->assertFalse( get_option( URLValidationProvider::LOCK_KEY ) ); $expected_result = 'EXPECTED RESULT'; - $result = $this->validation_provider->with_lock( + $result = $this->url_validation_provider->with_lock( function () use ( $expected_result ) { $this->assertTrue( (bool) get_option( URLValidationProvider::LOCK_KEY ) ); // Expect an error when lock is already in place. - $this->assertWPError( $this->validation_provider->with_lock( '__return_empty_string' ) ); + $this->assertWPError( $this->url_validation_provider->with_lock( '__return_empty_string' ) ); return $expected_result; } From 6f42fd37cb8ddd5b4920cc26af0580ff79bf0690 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 20:38:39 -0600 Subject: [PATCH 31/41] Add stub for wp cli function to avoid phpstan ignore comment --- .../cli/class-amp-cli-validation-command.php | 9 ++++----- tests/php/static-analysis-stubs/wp-cli.php | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 3a1d46040a9..bfecc211bf8 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -111,12 +111,11 @@ final class AMP_CLI_Validation_Command { public function run( $args, $assoc_args ) { $this->assoc_args = $assoc_args; - $scannable_url_provider = $this->get_validation_url_provider(); - $url_validation_provider = $this->get_validation_provider(); + $scannable_url_provider = $this->get_validation_url_provider(); + $url_validation_provider = $this->get_validation_provider(); $number_urls_to_crawl = count( $scannable_url_provider->get_urls() ); if ( ! $number_urls_to_crawl ) { - /** @phpstan-ignore-next-line */ if ( ! empty( Utils\get_flag_value( $this->assoc_args, self::INCLUDE_ARGUMENT, [] ) ) ) { WP_CLI::error( sprintf( @@ -275,8 +274,8 @@ private function get_validation_provider() { * Validates the URLs. */ private function validate_urls() { - $scannable_url_provider = $this->get_validation_url_provider(); - $url_validation_provider = $this->get_validation_provider(); + $scannable_url_provider = $this->get_validation_url_provider(); + $url_validation_provider = $this->get_validation_provider(); foreach ( $scannable_url_provider->get_urls() as $url ) { $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); diff --git a/tests/php/static-analysis-stubs/wp-cli.php b/tests/php/static-analysis-stubs/wp-cli.php index 1ac17e82bb6..00e9768cf9a 100644 --- a/tests/php/static-analysis-stubs/wp-cli.php +++ b/tests/php/static-analysis-stubs/wp-cli.php @@ -83,4 +83,23 @@ function format_items($format, $items, $fields) function make_progress_bar($message, $count, $interval = 100) { } + + /** + * Return the flag value or, if it's not set, the $default value. + * + * Because flags can be negated (e.g. --no-quiet to negate --quiet), this + * function provides a safer alternative to using + * `isset( $assoc_args['quiet'] )` or similar. + * + * @access public + * @category Input + * + * @param array $assoc_args Arguments array. + * @param string $flag Flag to get the value. + * @param mixed $default Default value for the flag. Default: NULL. + * @return mixed + */ + function get_flag_value( $assoc_args, $flag, $default = null ) + { + } } From bf19d9761d739abdc605e639c79218c9b3c0f47a Mon Sep 17 00:00:00 2001 From: John Watkins Date: Tue, 10 Nov 2020 20:40:19 -0600 Subject: [PATCH 32/41] Add parentheses to function in covers tag Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com> --- tests/php/src/Validation/ScannableURLProviderTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 89eb0c441ad..29faac05e9d 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -35,7 +35,7 @@ public function setUp() { add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); } - /** @covers ::__construct */ + /** @covers ::__construct() */ public function test__construct() { $this->assertInstanceOf( ScannableURLProvider::class, $this->scannable_url_provider ); } From de33111e05c1d05ff4ebcb5adda367bf7a328568 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 10:30:37 -0600 Subject: [PATCH 33/41] Combine redundant calls to get_urls method --- .../cli/class-amp-cli-validation-command.php | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index bfecc211bf8..dc7d1515fd7 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -113,8 +113,9 @@ public function run( $args, $assoc_args ) { $scannable_url_provider = $this->get_validation_url_provider(); $url_validation_provider = $this->get_validation_provider(); + $urls = $scannable_url_provider->get_urls(); - $number_urls_to_crawl = count( $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, [] ) ) ) { WP_CLI::error( @@ -141,8 +142,8 @@ public function run( $args, $assoc_args ) { ); $result = $url_validation_provider->with_lock( - function () { - $this->validate_urls(); + function () use ( $urls ) { + $this->validate_urls( $urls ); } ); @@ -272,12 +273,17 @@ private function get_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() { - $scannable_url_provider = $this->get_validation_url_provider(); - $url_validation_provider = $this->get_validation_provider(); + private function validate_urls( $urls = null ) { + if ( ! $urls ) { + $scannable_url_provider = $this->get_validation_url_provider(); + $url_validation_provider = $this->get_validation_provider(); + $urls = $scannable_url_provider->get_urls(); + } - foreach ( $scannable_url_provider->get_urls() as $url ) { + foreach ( $urls as $url ) { $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); if ( $this->wp_cli_progress ) { From f185379651347bf90e5d9f79b41e9b34d34f48ca Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 10:43:59 -0600 Subject: [PATCH 34/41] Fix for line inadvertently placed in conditional --- includes/cli/class-amp-cli-validation-command.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index dc7d1515fd7..0d8828ba6a4 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -277,10 +277,11 @@ private function get_validation_provider() { * @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(); + if ( ! $urls ) { - $scannable_url_provider = $this->get_validation_url_provider(); - $url_validation_provider = $this->get_validation_provider(); - $urls = $scannable_url_provider->get_urls(); + $scannable_url_provider = $this->get_validation_url_provider(); + $urls = $scannable_url_provider->get_urls(); } foreach ( $urls as $url ) { From cc3c9497dc53eabb52bf9ca5fe61b89b6c36bb31 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 10:49:45 -0600 Subject: [PATCH 35/41] Formatting fixes Co-authored-by: Weston Ruter --- src/Validation/URLValidationProvider.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index e976a186657..8216e4636ab 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -226,9 +226,9 @@ private function update_state_from_validity( $validity, $type ) { static function( $error ) { $validation_status = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization( $error ); return ( - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS !== $validation_status['term_status'] - && - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS !== $validation_status['term_status'] + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS !== $validation_status['term_status'] + && + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS !== $validation_status['term_status'] ); } ) @@ -249,7 +249,7 @@ static function( $error ) { 'total' => 0, ]; } - $this->validity_by_type[ $type ]['total']++; + $this->validity_by_type[ $type ]['total']++; if ( 0 === $unaccepted_error_count ) { $this->validity_by_type[ $type ]['valid']++; } From d87ad9d978a63df37fa7007c45f6ad1a6047311b Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 11:56:21 -0600 Subject: [PATCH 36/41] Eliminate the use of a flag in validate URL function --- .../cli/class-amp-cli-validation-command.php | 2 +- src/Validation/URLValidationProvider.php | 22 ++++--------------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index 0d8828ba6a4..b2b36207807 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -285,7 +285,7 @@ private function validate_urls( $urls = null ) { } foreach ( $urls as $url ) { - $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], URLValidationProvider::FLAG_FORCE_REVALIDATE ); + $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], true ); if ( $this->wp_cli_progress ) { $this->wp_cli_progress->tick(); diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 8216e4636ab..fd793491275 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -27,20 +27,6 @@ final class URLValidationProvider { */ const LOCK_KEY = 'amp_validation_locked'; - /** - * Flag to pass to get_url_validation to force revalidation. - * - * @param string - */ - const FLAG_FORCE_REVALIDATE = 'amp_force_revalidate'; - - /** - * Flag to pass to get_url_validation to skip revalidation. - * - * @param string - */ - const FLAG_NO_REVALIDATE = 'amp_no_revalidate'; - /** * The total number of validation errors, regardless of whether they were accepted. * @@ -181,14 +167,14 @@ public function get_validity_by_type() { * * @param string $url The URL to validate. * @param string $type The type of template, post, or taxonomy. - * @param string $flag Flag determining whether the URL should be revalidated. + * @param bool $force_revalidate Whether to force revalidation regardless of whether the current results are stale. * @return array|WP_Error Associative array containing validity result and whether the URL was revalidated, or a WP_Error on failure. */ - public function get_url_validation( $url, $type, $flag = null ) { + public function get_url_validation( $url, $type, $force_revalidate = false ) { $validity = null; $revalidated = true; - if ( self::FLAG_FORCE_REVALIDATE !== $flag ) { + if ( ! $force_revalidate ) { $url_post = AMP_Validated_URL_Post_Type::get_invalid_url_post( $url ); if ( $url_post && empty( AMP_Validated_URL_Post_Type::get_post_staleness( $url_post ) ) ) { @@ -197,7 +183,7 @@ public function get_url_validation( $url, $type, $flag = null ) { } } - if ( self::FLAG_NO_REVALIDATE !== $flag && ( is_null( $validity ) || self::FLAG_FORCE_REVALIDATE === $flag ) ) { + if ( is_null( $validity ) ) { $validity = AMP_Validation_Manager::validate_url_and_store( $url ); } From e34d6f580637b8c4bdf688ed680155144da488b5 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 12:23:02 -0600 Subject: [PATCH 37/41] Add leading slash to unnamespaced class used within namespace Co-authored-by: Weston Ruter --- tests/php/test-class-amp-cli-validation-command.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index 4a13af9d0c6..8c13dadf92b 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -16,7 +16,7 @@ * * @coversDefaultClass AMP_CLI_Validation_Command */ -class Test_AMP_CLI_Validation_Command extends \WP_UnitTestCase { +class Test_AMP_CLI_Validation_Command extends WP_UnitTestCase { use PrivateAccess; From 2e484276cea28486f5366917ee7a989b84a26958 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 12:36:23 -0600 Subject: [PATCH 38/41] Convert ValidationRequestMocking into trait --- tests/php/src/Helpers/ValidationRequestMocking.php | 8 ++++---- tests/php/src/Validation/ScannableURLProviderTest.php | 4 ++-- tests/php/src/Validation/URLValidationProviderTest.php | 8 ++++---- tests/php/test-class-amp-cli-validation-command.php | 10 +++++----- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/php/src/Helpers/ValidationRequestMocking.php b/tests/php/src/Helpers/ValidationRequestMocking.php index b3c65bf0cf2..79b7b7563a5 100644 --- a/tests/php/src/Helpers/ValidationRequestMocking.php +++ b/tests/php/src/Helpers/ValidationRequestMocking.php @@ -6,18 +6,18 @@ use WP_Query; /** - * Class ValidationRequestMocking + * Trait ValidationRequestMocking * * Helpers for validation tests. */ -final class ValidationRequestMocking { +trait ValidationRequestMocking { /** * Gets all of the validated URLs. * * @return string[] $urls The validated URLs. */ - public static function get_validated_urls() { + public function get_validated_urls() { $query = new WP_Query( [ 'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, @@ -39,7 +39,7 @@ static function( $post ) { * * @return array The response. */ - public static function get_validate_response() { + public function get_validate_response() { $mock_validation = [ 'results' => [ [ diff --git a/tests/php/src/Validation/ScannableURLProviderTest.php b/tests/php/src/Validation/ScannableURLProviderTest.php index 29faac05e9d..55499ef71dc 100644 --- a/tests/php/src/Validation/ScannableURLProviderTest.php +++ b/tests/php/src/Validation/ScannableURLProviderTest.php @@ -15,7 +15,7 @@ /** @coversDefaultClass \AmpProject\AmpWP\Validation\ScannableURLProvider */ final class ScannableURLProviderTest extends WP_UnitTestCase { - use PrivateAccess, AssertContainsCompatibility; + use PrivateAccess, AssertContainsCompatibility, ValidationRequestMocking; /** * Validation URL provider instance to use. @@ -32,7 +32,7 @@ final class ScannableURLProviderTest extends WP_UnitTestCase { public function setUp() { parent::setUp(); $this->scannable_url_provider = new ScannableURLProvider(); - add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); + add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } /** @covers ::__construct() */ diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 48719e10e24..601f2877b82 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -9,7 +9,7 @@ /** @coversDefaultClass \AmpProject\AmpWP\Validation\URLValidationProvider */ final class URLValidationProviderTest extends WP_UnitTestCase { - use AssertContainsCompatibility; + use AssertContainsCompatibility, ValidationRequestMocking; /** * Validation provider instance to use. @@ -26,7 +26,7 @@ final class URLValidationProviderTest extends WP_UnitTestCase { public function setUp() { parent::setUp(); $this->url_validation_provider = new URLValidationProvider(); - add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); + add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } /** @@ -40,7 +40,7 @@ public function setUp() { public function test_get_url_validation() { $single_post_permalink = get_permalink( self::factory()->post->create() ); $this->url_validation_provider->get_url_validation( $single_post_permalink, 'post' ); - $this->assertContains( $single_post_permalink, ValidationRequestMocking::get_validated_urls() ); + $this->assertContains( $single_post_permalink, $this->get_validated_urls() ); $number_of_posts = 30; $post_permalinks = []; @@ -52,7 +52,7 @@ public function test_get_url_validation() { } // All of the posts created should be present in the validated URLs. - $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); + $this->assertEmpty( array_diff( $post_permalinks, $this->get_validated_urls() ) ); $this->assertEquals( 31, $this->url_validation_provider->get_total_errors() ); $this->assertEmpty( $this->url_validation_provider->get_unaccepted_errors() ); diff --git a/tests/php/test-class-amp-cli-validation-command.php b/tests/php/test-class-amp-cli-validation-command.php index 4a13af9d0c6..5a25633f8bb 100644 --- a/tests/php/test-class-amp-cli-validation-command.php +++ b/tests/php/test-class-amp-cli-validation-command.php @@ -18,7 +18,7 @@ */ class Test_AMP_CLI_Validation_Command extends \WP_UnitTestCase { - use PrivateAccess; + use PrivateAccess, ValidationRequestMocking; /** * Store a reference to the validation command object. @@ -37,7 +37,7 @@ public function setUp() { AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG ); $this->validation = new AMP_CLI_Validation_Command(); - add_filter( 'pre_http_request', [ ValidationRequestMocking::class, 'get_validate_response' ] ); + add_filter( 'pre_http_request', [ $this, 'get_validate_response' ] ); } /** @@ -62,7 +62,7 @@ public function test_validate_urls() { $this->call_private_method( $this->validation, 'validate_urls' ); // All of the posts created above should be present in $validated_urls. - $this->assertEmpty( array_diff( $post_permalinks, ValidationRequestMocking::get_validated_urls() ) ); + $this->assertEmpty( array_diff( $post_permalinks, $this->get_validated_urls() ) ); $this->validation = new AMP_CLI_Validation_Command(); for ( $i = 0; $i < $number_of_terms; $i++ ) { @@ -73,10 +73,10 @@ public function test_validate_urls() { wp_set_post_terms( $posts[0], $terms, 'category' ); $this->call_private_method( $this->validation, 'validate_urls' ); $expected_validated_urls = array_map( 'get_term_link', $terms ); - $actual_validated_urls = ValidationRequestMocking::get_validated_urls(); + $actual_validated_urls = $this->get_validated_urls(); // All of the terms created above should be present in $validated_urls. $this->assertEmpty( array_diff( $expected_validated_urls, $actual_validated_urls ) ); - $this->assertContains( home_url( '/' ), ValidationRequestMocking::get_validated_urls() ); + $this->assertContains( home_url( '/' ), $this->get_validated_urls() ); } } From 8ea22d6be72bb616daacbd3a43d0d6648609d559 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 13:27:24 -0600 Subject: [PATCH 39/41] Reset lock timeout from inside CLI script --- includes/cli/class-amp-cli-validation-command.php | 7 ++++++- src/Validation/URLValidationProvider.php | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/includes/cli/class-amp-cli-validation-command.php b/includes/cli/class-amp-cli-validation-command.php index b2b36207807..aa37acb9851 100644 --- a/includes/cli/class-amp-cli-validation-command.php +++ b/includes/cli/class-amp-cli-validation-command.php @@ -284,7 +284,12 @@ private function validate_urls( $urls = null ) { $urls = $scannable_url_provider->get_urls(); } - foreach ( $urls as $url ) { + foreach ( $urls as $index => $url ) { + // Reset lock between every five URLs. + if ( 0 === $index % 5 ) { + $this->url_validation_provider->reset_lock(); + } + $validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], true ); if ( $this->wp_cli_progress ) { diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index fd793491275..56ddbbf0987 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -126,6 +126,13 @@ public function with_lock( $callback ) { return $result; } + /** + * Resets the lock timeout. This allows long-running processes to keep running beyond the lock timeout. + */ + public function reset_lock() { + $this->lock(); + } + /** * Provides the total number of validation errors found. * From 13917b3b4f9b8869ba6870dae308e09c04a68d50 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 13:29:56 -0600 Subject: [PATCH 40/41] Eliminate lock timeout filter --- src/Validation/URLValidationProvider.php | 23 +++++++------------ .../Validation/URLValidationProviderTest.php | 1 - 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 56ddbbf0987..837e98d0854 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -27,6 +27,13 @@ final class URLValidationProvider { */ const LOCK_KEY = 'amp_validation_locked'; + /** + * The length of time to keep the lock in place if a process fails to unlock. + * + * @var int + */ + const LOCK_TIMEOUT = 5 * MINUTE_IN_SECONDS; + /** * The total number of validation errors, regardless of whether they were accepted. * @@ -88,21 +95,7 @@ public function is_locked() { $lock_time = (int) get_option( self::LOCK_KEY, 0 ); // It's locked if the difference between the lock time and the current time is less than the lockout time. - return time() - $lock_time < $this->get_lock_timeout(); - } - - /** - * Provides the length of time, in seconds, to lock validation when this runs. - * - * @return int - */ - private function get_lock_timeout() { - /** - * Filters the length of time to lock URL validation when a process starts. - * - * @param int $timeout Time in seconds. Default 300 seconds. - */ - return apply_filters( 'amp_validation_lock_timeout', 5 * MINUTE_IN_SECONDS ); + return time() - $lock_time < self::LOCK_TIMEOUT; } /** diff --git a/tests/php/src/Validation/URLValidationProviderTest.php b/tests/php/src/Validation/URLValidationProviderTest.php index 601f2877b82..ec7388186e4 100644 --- a/tests/php/src/Validation/URLValidationProviderTest.php +++ b/tests/php/src/Validation/URLValidationProviderTest.php @@ -70,7 +70,6 @@ public function test_get_url_validation() { * @covers ::lock() * @covers ::unlock() * @covers ::is_locked() - * @covers ::get_lock_timeout() * @covers ::with_lock() */ public function test_locking() { From 9c627b3c7b347d9027cc63b83ce864a1af922c47 Mon Sep 17 00:00:00 2001 From: John Watkins Date: Mon, 23 Nov 2020 13:32:56 -0600 Subject: [PATCH 41/41] PHPCS fix --- src/Validation/URLValidationProvider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Validation/URLValidationProvider.php b/src/Validation/URLValidationProvider.php index 837e98d0854..d3fffd39cf5 100644 --- a/src/Validation/URLValidationProvider.php +++ b/src/Validation/URLValidationProvider.php @@ -29,7 +29,7 @@ final class URLValidationProvider { /** * The length of time to keep the lock in place if a process fails to unlock. - * + * * @var int */ const LOCK_TIMEOUT = 5 * MINUTE_IN_SECONDS;