Fix the order in which ensure_required_markup() is called (#1041)

* Fix the order in which ensure_required_markup() is called * Check is defined and add test case to check sanitizing of non-AMP themes * Fix test case * Skip printing paths-scope because it causes push builds to be truncated (and fail) for unknown reason See xwp/wp-dev-lib#270
ampproject · Mar 28, 2018 · d53af32 · d53af32
1 parent b2e24ce
commit d53af32
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 18 deletions.
diff --git a/.dev-lib b/.dev-lib
@@ -2,6 +2,7 @@ PATH_EXCLUDES_PATTERN=includes/lib/
 DEFAULT_BASE_BRANCH=develop
 ASSETS_DIR=wp-assets
 PROJECT_SLUG=amp
+SKIP_ECHO_PATHS_SCOPE=1
 
 function after_wp_install {
     echo "Installing REST API..."

diff --git a/dev-lib b/dev-lib
diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
@@ -885,13 +885,6 @@ public static function print_amp_styles() {
 	 * @param DOMDocument $dom Doc.
 	 */
 	public static function ensure_required_markup( DOMDocument $dom ) {
-		$xpath = new DOMXPath( $dom );
-
-		// First ensure the mandatory amp attribute is present on the html element, as otherwise it will be stripped entirely.
-		if ( ! $dom->documentElement->hasAttribute( 'amp' ) && ! $dom->documentElement->hasAttribute( '⚡️' ) ) {
-			$dom->documentElement->setAttribute( 'amp', '' );
-		}
-
 		$head = $dom->getElementsByTagName( 'head' )->item( 0 );
 		if ( ! $head ) {
 			$head = $dom->createElement( 'head' );
@@ -953,15 +946,6 @@ public static function ensure_required_markup( DOMDocument $dom ) {
 			) );
 			$head->appendChild( $rel_canonical );
 		}
-
-		// Make sure scripts from the body get moved to the head.
-		$scripts = array();
-		foreach ( $xpath->query( '//body//script[ @custom-element or @custom-template ]' ) as $script ) {
-			$scripts[] = $script;
-		}
-		foreach ( $scripts as $script ) {
-			$head->appendChild( $script );
-		}
 	}
 
 	/**
@@ -1098,10 +1082,26 @@ public static function prepare_response( $response, $args = array() ) {
 		}
 		$dom = AMP_DOM_Utils::get_dom( $response );
 
-		self::ensure_required_markup( $dom );
+		$xpath = new DOMXPath( $dom );
+
+		$head = $dom->getElementsByTagName( 'head' )->item( 0 );
+
+		if ( isset( $head ) ) {
+			// Make sure scripts from the body get moved to the head.
+			foreach ( $xpath->query( '//body//script[ @custom-element or @custom-template ]' ) as $script ) {
+				$head->appendChild( $script );
+			}
+		}
+
+		// Ensure the mandatory amp attribute is present on the html element, as otherwise it will be stripped entirely.
+		if ( ! $dom->documentElement->hasAttribute( 'amp' ) && ! $dom->documentElement->hasAttribute( '⚡️' ) ) {
+			$dom->documentElement->setAttribute( 'amp', '' );
+		}
 
 		$assets = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args );
 
+		self::ensure_required_markup( $dom );
+
 		// @todo If 'utf-8' is not the blog charset, then we'll need to do some character encoding conversation or "entityification".
 		if ( 'utf-8' !== strtolower( get_bloginfo( 'charset' ) ) ) {
 			/* translators: %s is the charset of the current site */

diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php
@@ -212,6 +212,45 @@ public function test_prepare_response() {
 		$this->assertInstanceOf( 'DOMAttr', $removed_nodes['onclick'] );
 	}
 
+	/**
+	 * Test validate_non_amp_theme.
+	 *
+	 * @global WP_Widget_Factory $wp_widget_factory
+	 * @global WP_Scripts $wp_scripts
+	 * @covers AMP_Theme_Support::prepare_response()
+	 */
+	public function test_validate_non_amp_theme() {
+		add_theme_support( 'amp' );
+		AMP_Theme_Support::init();
+		AMP_Theme_Support::finish_init();
+
+		ob_start();
+		?>
+		<!DOCTYPE html>
+		<html lang="en-US" class="no-js">
+			<head>
+				<meta charset="UTF-8">
+				<meta name="viewport" content="width=device-width">
+				<?php wp_head(); ?>
+			</head>
+			<body>
+				<?php wp_print_scripts( 'amp-mathml' ); ?>
+			</body>
+		</html>
+		<?php
+		$original_html  = trim( ob_get_clean() );
+		$sanitized_html = AMP_Theme_Support::prepare_response( $original_html );
+
+		// Invalid viewport meta tag is not present.
+		$this->assertNotContains( '<meta name="viewport" content="width=device-width">', $sanitized_html );
+
+		// Correct viewport meta tag was added.
+		$this->assertContains( '<meta name="viewport" content="width=device-width,minimum-scale=1">', $sanitized_html );
+
+		// MathML script was added.
+		$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0/amp-mathml-latest.js" async custom-element="amp-mathml"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
+	}
+
 	/**
 	 * Test prepare_response for bad/non-HTML.
 	 *