Increase timeout for validate_url requests; show errors when fetching…

… fails See #1166.
ampproject · Jun 7, 2018 · 236be4b · 236be4b
1 parent 16441b4
commit 236be4b
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 8 deletions.
diff --git a/includes/validation/class-amp-invalid-url-post-type.php b/includes/validation/class-amp-invalid-url-post-type.php
@@ -137,6 +137,7 @@ public static function add_admin_hooks() {
 			$query_vars[] = 'amp_taxonomy_terms_updated';
 			$query_vars[] = self::REMAINING_ERRORS;
 			$query_vars[] = 'amp_urls_tested';
+			$query_vars[] = 'amp_validate_error';
 			return $query_vars;
 		} );
 	}
@@ -681,6 +682,9 @@ public static function handle_bulk_action( $redirect, $action, $items ) {
 			return $redirect;
 		}
 		$remaining_invalid_urls = array();
+
+		$errors = array();
+
 		foreach ( $items as $item ) {
 			$post = get_post( $item );
 			if ( empty( $post ) ) {
@@ -693,8 +697,9 @@ public static function handle_bulk_action( $redirect, $action, $items ) {
 			}
 
 			$validity = AMP_Validation_Manager::validate_url( $url );
-			if ( ! is_array( $validity ) ) {
-				continue; // @todo Count this error.
+			if ( is_wp_error( $validity ) ) {
+				$errors[] = $validity->get_error_code();
+				continue;
 			}
 
 			self::store_validation_errors( $validity['validation_errors'], $validity['url'], $post );
@@ -705,10 +710,15 @@ public static function handle_bulk_action( $redirect, $action, $items ) {
 
 		// Get the URLs that still have errors after rechecking.
 		$args = array(
-			self::URLS_TESTED      => count( $items ),
-			self::REMAINING_ERRORS => empty( $remaining_invalid_urls ) ? '0' : '1',
+			self::URLS_TESTED => count( $items ),
 		);
+		if ( ! empty( $errors ) ) {
+			$args['amp_validate_error'] = $errors;
+		} else {
+			$args[ self::REMAINING_ERRORS ] = count( $remaining_invalid_urls );
+		}
 
+		$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
 		return add_query_arg( $args, $redirect );
 	}
 
@@ -722,6 +732,31 @@ public static function print_admin_notice() {
 			return;
 		}
 
+		if ( isset( $_GET['amp_validate_error'] ) ) { // WPCS: CSRF OK.
+			$error_codes = array_unique( array_map( 'sanitize_key', (array) $_GET['amp_validate_error'] ) ); // WPCS: CSRF OK.
+			foreach ( $error_codes as $error_code ) {
+				switch ( $error_code ) {
+					case 'http_request_failed':
+						$message = __( 'Failed to fetch URL(s) to validate. This may be due to a request timeout.', 'amp' );
+						break;
+					case '404':
+						$message = __( 'The fetched URL(s) was not found. It may have been deleted. If so, you can trash this.', 'amp' );
+						break;
+					case '500':
+						$message = __( 'An internal server error occurred when fetching the URL.', 'amp' );
+						break;
+					default:
+						/* translators: %s is error code */
+						$message = sprintf( __( 'Unable to validate the URL(s); error code is %s.', 'amp' ), $error_code ); // Note that $error_code has been sanitized with sanitize_key(); will be escaped below as well.
+				}
+				printf(
+					'<div class="notice is-dismissible error"><p>%s</p><button type="button" class="notice-dismiss"><span class="screen-reader-text">%s</span></button></div>',
+					esc_html( $message ),
+					esc_html__( 'Dismiss this notice.', 'amp' )
+				);
+			}
+		}
+
 		if ( isset( $_GET[ self::REMAINING_ERRORS ] ) ) {
 			$count_urls_tested = isset( $_GET[ self::URLS_TESTED ] ) ? intval( $_GET[ self::URLS_TESTED ] ) : 1; // WPCS: CSRF ok.
 			$errors_remain     = ! empty( $_GET[ self::REMAINING_ERRORS ] ); // WPCS: CSRF ok.
@@ -777,7 +812,7 @@ public static function handle_inline_recheck( $post_id ) {
 			$redirect = remove_query_arg( wp_removable_query_args(), $redirect );
 		}
 
-		if ( ! $redirect || is_wp_error( $validation_results ) || empty( $validation_results ) ) {
+		if ( ! $redirect || empty( $validation_results ) ) {
 			// If there are no remaining errors and the post was deleted, redirect to edit.php instead of post.php.
 			$redirect = add_query_arg(
 				'post_type',
@@ -789,8 +824,9 @@ public static function handle_inline_recheck( $post_id ) {
 			self::URLS_TESTED => '1',
 		);
 
-		// @todo For WP_Error case, see <https://github.com/Automattic/amp-wp/issues/1166>.
-		if ( ! is_wp_error( $validation_results ) ) {
+		if ( is_wp_error( $validation_results ) ) {
+			$args['amp_validate_error'] = $validation_results->get_error_code();
+		} else {
 			$args[ self::REMAINING_ERRORS ] = count( array_filter(
 				$validation_results,
 				function( $result ) {

diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php
@@ -1320,8 +1320,9 @@ public static function validate_url( $url ) {
 		for ( $redirect_count = 0; $redirect_count < $allowed_redirects; $redirect_count++ ) {
 			$r = wp_remote_get( $validation_url, array(
 				'cookies'     => wp_unslash( $_COOKIE ), // Pass along cookies so private pages and drafts can be accessed.
+				'timeout'     => 15, // Increase from default of 5 to give extra time for the plugin to identify the sources for any given validation errors; also, response caching is disabled when validating.
 				'sslverify'   => false,
-				'redirection' => 0,
+				'redirection' => 0, // Because we're in a loop for redirection.
 				'headers'     => array(
 					'Cache-Control' => 'no-cache',
 				),