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

amedina · 2018-03-23T01:40:23Z

The order in which ensure_required_markup was being called was causing the viewport meta tag to be stripped out by the sanitizer.

westonruter · 2018-03-23T16:26:01Z

This is a regression introduced by me in 0154620

westonruter · 2018-03-23T16:44:20Z

includes/class-amp-theme-support.php

+			$scripts[] = $script;
+		}
+		foreach ( $scripts as $script ) {
+			$head->appendChild( $script );


The $head is undefined. We should grab it with DOMXPath and only do the script gathering if it is defined.

westonruter · 2018-03-23T18:29:40Z

It would be good to add a new test case for the bug this is fixed so that it doesn't re-surface. For example another test like \Test_AMP_Theme_Support::test_prepare_response() could be made which specifically tests the validation of a non-AMP theme like Twenty Fifteen. Take some of the actual output of Twenty Fifteen (in particular the AMP-invalid meta viewport) and running it through the sanitizer to ensure the expected viewport comes out the other end. Similarly, inside the body there could be a call to <?php wp_print_scripts( 'amp-mathml' ); ?> and then test to make sure that the script tag gets moved to the head properly.

westonruter · 2018-03-23T23:46:38Z

tests/test-class-amp-theme-support.php

+		$original_html  = trim( ob_get_clean() );
+		$removed_nodes  = array();
+		$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array(
+			'validation_error_callback' => function( $removed ) use ( &$removed_nodes ) {


I don't think we need validation_error_callback here since $removed_nodes is never checked. Alternative, you could $this->assertEmpty( $removed_nodes ) to verify that nothing got stripped out.

westonruter · 2018-03-23T23:47:12Z

tests/test-class-amp-theme-support.php

+			},
+		) );
+
+		$this->assertContains( '<meta charset="' . get_bloginfo( 'charset' ) . '">', $sanitized_html );


Since AMP must be UTF-8, should this use not be a literal UTF-8?

Agree; changed to $this->assertContains( '<meta charset="utf-8">', $sanitized_html );

westonruter · 2018-03-23T23:48:14Z

tests/test-class-amp-theme-support.php

+	 *
+	 * @global WP_Widget_Factory $wp_widget_factory
+	 * @global WP_Scripts $wp_scripts
+	 * @covers AMP_Theme_Support::validate_non_amp_theme()


There is no such method as AMP_Theme_Support::validate_non_amp_theme(). It should be AMP_Theme_Support:: prepare_response().

westonruter · 2018-03-23T23:48:49Z

tests/test-class-amp-theme-support.php

+		AMP_Theme_Support::init();
+		AMP_Theme_Support::finish_init();
+		$wp_widget_factory = new WP_Widget_Factory();
+		wp_widgets_init();


Since no widgets are being tested in this response, I think the references to $wp_widget_factory and wp_widgets_init() in this PR can be removed.

…ed (and fail) for unknown reason See xwp/wp-dev-lib#270

ThierryA

@amedina thanks for fixing the viewport bug 👍I left my CR below.

ThierryA · 2018-03-27T08:55:07Z

tests/test-class-amp-theme-support.php

+	 * @covers AMP_Theme_Support::prepare_response()
+	 */
+	public function test_validate_non_amp_theme() {
+		global $wp_widget_factory, $wp_scripts;


$wp_widget_factory doesn't seem to be used in this method and the $wp_scripts are already reset in the tearDown() method.

ThierryA · 2018-03-27T08:56:28Z

tests/test-class-amp-theme-support.php

+		</html>
+		<?php
+		$original_html  = trim( ob_get_clean() );
+		$removed_nodes  = array();


$removed_nodes doesn't seem to be used in this method.

ThierryA · 2018-03-27T09:01:12Z

includes/class-amp-theme-support.php

+
+		if ( isset( $head ) ) {
+			// Make sure scripts from the body get moved to the head.
+			$scripts = array();


Any reason to store the scripts in a var and run two loops rather than doing the following:

foreach ( $xpath->query( '//body//script[ @custom-element or @custom-template ]' ) as $script ) { $head->appendChild( $script ); }

Makes sense :)

ThierryA · 2018-03-28T07:44:29Z

tests/test-class-amp-theme-support.php

@@ -220,7 +220,6 @@ public function test_prepare_response() {
 	 * @covers AMP_Theme_Support::prepare_response()
 	 */
 	public function test_validate_non_amp_theme() {
-		global $wp_widget_factory, $wp_scripts;
 		$wp_scripts = null;


This line still needs to be removed and this PR will be good to go. Thanks @amedina, great stuff!

Fix the order in which ensure_required_markup() is called

bd3a877

amedina requested a review from westonruter March 23, 2018 01:40

westonruter reviewed Mar 23, 2018

View reviewed changes

westonruter added this to the v0.7 milestone Mar 23, 2018

amedina added 3 commits March 23, 2018 14:42

Check is defined and add test case to check sanitizing of non-AMP themes

c5b9e1d

Check is defined and add test case to check sanitizing of non-AMP themes

ea8a2d7

Fix merge conflict.

41e2258

westonruter reviewed Mar 23, 2018

View reviewed changes

amedina self-assigned this Mar 24, 2018

amedina force-pushed the amedina/fix-order-of-ensure-required-markup branch 3 times, most recently from 0c6c205 to 6ce2206 Compare March 24, 2018 23:09

MackenzieHartung added the Sprint 2 label Mar 26, 2018

Fix test case

d61de17

amedina force-pushed the amedina/fix-order-of-ensure-required-markup branch from 6ce2206 to d61de17 Compare March 26, 2018 22:38

Skip printing paths-scope because it causes push builds to be truncat…

6cef7e6

…ed (and fail) for unknown reason See xwp/wp-dev-lib#270

amedina force-pushed the amedina/fix-order-of-ensure-required-markup branch from e7f90a7 to 6cef7e6 Compare March 26, 2018 23:27

Merge branch '0.7' into amedina/fix-order-of-ensure-required-markup

945d583

ThierryA requested changes Mar 27, 2018

View reviewed changes

Improve handling of scripts and remove unnecessary variables

65422e0

ThierryA requested changes Mar 28, 2018

View reviewed changes

Remove unnecessary line.

38edc74

amedina force-pushed the amedina/fix-order-of-ensure-required-markup branch from e4c7388 to 38edc74 Compare March 28, 2018 16:48

amedina merged commit d53af32 into 0.7 Mar 28, 2018

westonruter deleted the amedina/fix-order-of-ensure-required-markup branch March 30, 2018 04:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

amedina commented Mar 23, 2018

westonruter commented Mar 23, 2018

westonruter Mar 23, 2018

westonruter commented Mar 23, 2018

westonruter Mar 23, 2018

westonruter Mar 23, 2018

amedina Mar 24, 2018

westonruter Mar 23, 2018

westonruter Mar 23, 2018

ThierryA left a comment

ThierryA Mar 27, 2018 •

edited

Loading

amedina Mar 27, 2018

ThierryA Mar 27, 2018

amedina Mar 28, 2018

ThierryA Mar 27, 2018 •

edited

Loading

amedina Mar 28, 2018

ThierryA Mar 28, 2018

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

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

Conversation

amedina commented Mar 23, 2018

westonruter commented Mar 23, 2018

Choose a reason for hiding this comment

westonruter commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThierryA left a comment

Choose a reason for hiding this comment

ThierryA Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThierryA Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThierryA Mar 27, 2018 •

edited

Loading

ThierryA Mar 27, 2018 •

edited

Loading