Merge pull request #971 from Automattic/add/842-plugin-validation

Add validation error reporting for plugins and themes

.dev-lib

@@ -1,6 +1,7 @@
 PATH_EXCLUDES_PATTERN=includes/lib/
 DEFAULT_BASE_BRANCH=develop
 ASSETS_DIR=wp-assets
+PROJECT_SLUG=amp
 
 function after_wp_install {
     echo "Installing REST API..."

amp.php

@@ -109,9 +109,9 @@ function amp_init() {
 
 	add_rewrite_endpoint( AMP_QUERY_VAR, EP_PERMALINK );
 
+	AMP_Validation_Utils::init();
 	AMP_Theme_Support::init();
 	AMP_Post_Type_Support::add_post_type_support();
-	AMP_Validation_Utils::init();
 	add_filter( 'request', 'amp_force_query_var_value' );
 	add_action( 'admin_init', 'AMP_Options_Manager::register_settings' );
 	add_action( 'wp_loaded', 'amp_post_meta_box' );

includes/amp-helper-functions.php

@@ -32,12 +32,16 @@ function amp_get_permalink( $post_id ) {
 		return $pre_url;
 	}
 
-	$parsed_url = wp_parse_url( get_permalink( $post_id ) );
-	$structure  = get_option( 'permalink_structure' );
-	if ( empty( $structure ) || ! empty( $parsed_url['query'] ) || is_post_type_hierarchical( get_post_type( $post_id ) ) ) {
-		$amp_url = add_query_arg( AMP_QUERY_VAR, '', get_permalink( $post_id ) );
+	if ( amp_is_canonical() ) {
+		$amp_url = get_permalink( $post_id );
 	} else {
-		$amp_url = trailingslashit( get_permalink( $post_id ) ) . user_trailingslashit( AMP_QUERY_VAR, 'single_amp' );
+		$parsed_url = wp_parse_url( get_permalink( $post_id ) );
+		$structure  = get_option( 'permalink_structure' );
+		if ( empty( $structure ) || ! empty( $parsed_url['query'] ) || is_post_type_hierarchical( get_post_type( $post_id ) ) ) {
+			$amp_url = add_query_arg( AMP_QUERY_VAR, '', get_permalink( $post_id ) );
+		} else {
+			$amp_url = trailingslashit( get_permalink( $post_id ) ) . user_trailingslashit( AMP_QUERY_VAR, 'single_amp' );
+		}
 	}
 
 	/**
@@ -51,6 +55,25 @@ function amp_get_permalink( $post_id ) {
 	return apply_filters( 'amp_get_permalink', $amp_url, $post_id );
 }
 
+/**
+ * Remove the AMP endpoint (and query var) from a given URL.
+ *
+ * @since 0.7
+ *
+ * @param string $url URL.
+ * @return string URL with AMP stripped.
+ */
+function amp_remove_endpoint( $url ) {
+
+	// Strip endpoint.
+	$url = preg_replace( ':/' . preg_quote( AMP_QUERY_VAR, ':' ) . '(?=/?(\?|#|$)):', '', $url );
+
+	// Strip query var.
+	$url = remove_query_arg( AMP_QUERY_VAR, $url );
+
+	return $url;
+}
+
 /**
  * Determine whether a given post supports AMP.
  *

includes/class-amp-theme-support.php

@@ -77,13 +77,7 @@ public static function init() {
 
 		self::purge_amp_query_vars();
 		self::handle_xhr_request();
-
-		if ( ! is_amp_endpoint() ) {
-			amp_add_frontend_actions();
-		} else {
-			self::setup_commenting();
-			add_action( 'widgets_init', array( __CLASS__, 'register_widgets' ) );
-		}
+		self::add_temporary_discussion_restrictions();
 
 		require_once AMP__DIR__ . '/includes/amp-post-template-actions.php';
 
@@ -98,20 +92,55 @@ public static function init() {
 			}
 		}
 
-		if ( amp_is_canonical() ) {
+		add_action( 'widgets_init', array( __CLASS__, 'register_widgets' ) );
 
-			// Redirect to canonical URL if the AMP URL was loaded, since canonical is now AMP.
-			if ( false !== get_query_var( AMP_QUERY_VAR, false ) ) { // Because is_amp_endpoint() now returns true if amp_is_canonical().
-				wp_safe_redirect( self::get_current_canonical_url(), 302 ); // Temporary redirect because canonical may change in future.
-				exit;
-			}
+		/*
+		 * Note that wp action is use instead of template_redirect because some themes/plugins output
+		 * the response at this action and then short-circuit with exit. So this is why the the preceding
+		 * action to template_redirect--the wp action--is used instead.
+		 */
+		add_action( 'wp', array( __CLASS__, 'finish_init' ), PHP_INT_MAX );
+	}
+
+	/**
+	 * Finish initialization once query vars are set.
+	 *
+	 * @since 0.7
+	 */
+	public static function finish_init() {
+		if ( ! is_amp_endpoint() ) {
+			amp_add_frontend_actions();
+			return;
+		}
+
+		if ( amp_is_canonical() ) {
+			self::redirect_canonical_amp();
 		} else {
 			self::register_paired_hooks();
 		}
 
-		self::register_hooks();
-		self::$embed_handlers    = self::register_content_embed_handlers();
+		self::add_hooks();
 		self::$sanitizer_classes = amp_get_content_sanitizers();
+		self::$embed_handlers    = self::register_content_embed_handlers();
+	}
+
+	/**
+	 * Redirect to canonical URL if the AMP URL was loaded, since canonical is now AMP.
+	 *
+	 * @since 0.7
+	 */
+	public static function redirect_canonical_amp() {
+		if ( false !== get_query_var( AMP_QUERY_VAR, false ) ) { // Because is_amp_endpoint() now returns true if amp_is_canonical().
+			$url = preg_replace( '#^(https?://.+?)(/.*)$#', '$1', home_url( '/' ) );
+			if ( isset( $_SERVER['REQUEST_URI'] ) ) {
+				$url .= wp_unslash( $_SERVER['REQUEST_URI'] );
+			}
+
+			$url = amp_remove_endpoint( $url );
+
+			wp_safe_redirect( $url, 302 ); // Temporary redirect because canonical may change in future.
+			exit;
+		}
 	}
 
 	/**
@@ -166,7 +195,7 @@ public static function register_paired_hooks() {
 	/**
 	 * Register hooks.
 	 */
-	public static function register_hooks() {
+	public static function add_hooks() {
 
 		// Remove core actions which are invalid AMP.
 		remove_action( 'wp_head', 'wp_post_preview_js', 1 );
@@ -216,6 +245,10 @@ public static function register_hooks() {
 		add_action( 'comment_form', array( __CLASS__, 'add_amp_comment_form_templates' ), 100 );
 		remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' );
 
+		if ( AMP_Validation_Utils::should_validate_response() ) {
+			AMP_Validation_Utils::add_validation_hooks();
+		}
+
 		// @todo Add character conversion.
 	}
 
@@ -279,7 +312,7 @@ public static function purge_amp_query_vars() {
 	}
 
 	/**
-	 * Hook into a form submissions, such as comment the form or some other .
+	 * Hook into a form submissions, such as the comment form or some other form submission.
 	 *
 	 * @since 0.7.0
 	 * @global string $pagenow
@@ -370,14 +403,14 @@ public static function intercept_post_request_redirect( $location ) {
 	}
 
 	/**
-	 * Set up commenting.
+	 * Set up some restrictions for commenting based on amp-live-list limitations.
+	 *
+	 * Temporarily force comments to be listed in descending order.
+	 * The following hooks are temporary while waiting for amphtml#5396 to be resolved.
+	 *
+	 * @link https://github.com/ampproject/amphtml/issues/5396
 	 */
-	public static function setup_commenting() {
-		/*
-		 * Temporarily force comments to be listed in descending order.
-		 *
-		 * The following hooks are temporary while waiting for amphtml#5396 to be resolved.
-		 */
+	protected static function add_temporary_discussion_restrictions() {
 		add_filter( 'option_comment_order', function() {
 			return 'desc';
 		}, PHP_INT_MAX );
@@ -584,13 +617,7 @@ public static function get_current_canonical_url() {
 			$url = add_query_arg( $added_query_vars, $url );
 		}
 
-		// Strip endpoint.
-		$url = preg_replace( ':/' . preg_quote( AMP_QUERY_VAR, ':' ) . '(?=/?(\?|#|$)):', '', $url );
-
-		// Strip query var.
-		$url = remove_query_arg( AMP_QUERY_VAR, $url );
-
-		return $url;
+		return amp_remove_endpoint( $url );
 	}
 
 	/**
@@ -953,13 +980,15 @@ public static function prepare_response( $response, $args = array() ) {
 			return $response;
 		}
 
+		$is_validation_debug_mode = ! empty( $_REQUEST[ AMP_Validation_Utils::DEBUG_QUERY_VAR ] ); // WPCS: csrf ok.
+
 		$args = array_merge(
 			array(
 				'content_max_width'       => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
 				'use_document_element'    => true,
-				'remove_invalid_callback' => null,
 				'allow_dirty_styles'      => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
 				'allow_dirty_scripts'     => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
+				'disable_invalid_removal' => $is_validation_debug_mode,
 			),
 			$args
 		);
@@ -994,6 +1023,12 @@ public static function prepare_response( $response, $args = array() ) {
 			trigger_error( esc_html( sprintf( __( 'The database has the %s encoding when it needs to be utf-8 to work with AMP.', 'amp' ), get_bloginfo( 'charset' ) ) ), E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
 		}
 
+		if ( AMP_Validation_Utils::should_validate_response() ) {
+			AMP_Validation_Utils::finalize_validation( $dom, array(
+				'remove_source_comments' => ! $is_validation_debug_mode,
+			) );
+		}
+
 		$response  = "<!DOCTYPE html>\n";
 		$response .= AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement );
 

includes/options/class-amp-options-menu.php

@@ -22,7 +22,7 @@ class AMP_Options_Menu {
 	 */
 	public function init() {
 		add_action( 'admin_post_amp_analytics_options', 'AMP_Options_Manager::handle_analytics_submit' );
-		add_action( 'admin_menu', array( $this, 'add_menu_items' ) );
+		add_action( 'admin_menu', array( $this, 'add_menu_items' ), 9 );
 	}
 
 	/**

includes/sanitizers/class-amp-base-sanitizer.php

@@ -54,6 +54,7 @@ abstract class AMP_Base_Sanitizer {
 	 *      @type array $amp_bind_placeholder_prefix
 	 *      @type bool $allow_dirty_styles
 	 *      @type bool $allow_dirty_scripts
+	 *      @type bool $disable_invalid_removal
 	 *      @type callable $remove_invalid_callback
 	 * }
 	 */
@@ -328,17 +329,19 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {
 	 *
 	 * @since 0.7
 	 *
-	 * @param DOMNode|DOMElement $child The node to remove.
+	 * @param DOMNode|DOMElement $node The node to remove.
+	 * @param array              $args Additional args to pass to validation error callback.
+	 *
 	 * @return void
 	 */
-	public function remove_invalid_child( $child ) {
-		$parent = $child->parentNode;
-		$child->parentNode->removeChild( $child );
-		if ( isset( $this->args['remove_invalid_callback'] ) ) {
-			call_user_func( $this->args['remove_invalid_callback'], array(
-				'node'   => $child,
-				'parent' => $parent,
-			) );
+	public function remove_invalid_child( $node, $args = array() ) {
+		if ( isset( $this->args['validation_error_callback'] ) ) {
+			call_user_func( $this->args['validation_error_callback'],
+				array_merge( compact( 'node' ), $args )
+			);
+		}
+		if ( empty( $this->args['disable_invalid_removal'] ) ) {
+			$node->parentNode->removeChild( $node );
 		}
 	}
 
@@ -352,25 +355,33 @@ public function remove_invalid_child( $child ) {
 	 *
 	 * @param DOMElement     $element   The node for which to remove the attribute.
 	 * @param DOMAttr|string $attribute The attribute to remove from the element.
+	 * @param array          $args      Additional args to pass to validation error callback.
 	 * @return void
 	 */
-	public function remove_invalid_attribute( $element, $attribute ) {
-		if ( isset( $this->args['remove_invalid_callback'] ) ) {
+	public function remove_invalid_attribute( $element, $attribute, $args = array() ) {
+		if ( isset( $this->args['validation_error_callback'] ) ) {
 			if ( is_string( $attribute ) ) {
 				$attribute = $element->getAttributeNode( $attribute );
 			}
 			if ( $attribute ) {
+				call_user_func( $this->args['validation_error_callback'],
+					array_merge(
+						array(
+							'node' => $attribute,
+						),
+						$args
+					)
+				);
+				if ( empty( $this->args['disable_invalid_removal'] ) ) {
+					$element->removeAttributeNode( $attribute );
+				}
+			}
+		} elseif ( empty( $this->args['disable_invalid_removal'] ) ) {
+			if ( is_string( $attribute ) ) {
+				$element->removeAttribute( $attribute );
+			} else {
 				$element->removeAttributeNode( $attribute );
-				call_user_func( $this->args['remove_invalid_callback'], array(
-					'node'   => $attribute,
-					'parent' => $element,
-				) );
 			}
-		} elseif ( is_string( $attribute ) ) {
-			$element->removeAttribute( $attribute );
-		} else {
-			$element->removeAttributeNode( $attribute );
 		}
 	}
-
 }