From 82c274b0a84228205757e4e78b041f9fc07b776c Mon Sep 17 00:00:00 2001 From: ThierryA Date: Fri, 19 Jan 2018 00:33:34 +0100 Subject: [PATCH 01/11] Bumped version to 0.6-rc2 --- amp.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/amp.php b/amp.php index 68cbdb4b8db..580c8cec7b7 100644 --- a/amp.php +++ b/amp.php @@ -5,7 +5,7 @@ * Plugin URI: https://github.com/automattic/amp-wp * Author: Automattic * Author URI: https://automattic.com - * Version: 0.6-rc1 + * Version: 0.6-rc2 * Text Domain: amp * Domain Path: /languages/ * License: GPLv2 or later @@ -15,7 +15,7 @@ define( 'AMP__FILE__', __FILE__ ); define( 'AMP__DIR__', dirname( __FILE__ ) ); -define( 'AMP__VERSION', '0.6-rc1' ); +define( 'AMP__VERSION', '0.6-rc2' ); require_once AMP__DIR__ . '/includes/class-amp-autoloader.php'; AMP_Autoloader::register(); From 91ef3867dc1f523dad09a735119fb7140b8f439d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 19 Jan 2018 10:59:27 -0800 Subject: [PATCH 02/11] Remove erroneous additional allowance of script[type=text/javascript] Issue introduced in #828 via 5c1fa2086 --- includes/sanitizers/class-amp-rule-spec.php | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/includes/sanitizers/class-amp-rule-spec.php b/includes/sanitizers/class-amp-rule-spec.php index 694b7dcf7df..6bec8aa167a 100644 --- a/includes/sanitizers/class-amp-rule-spec.php +++ b/includes/sanitizers/class-amp-rule-spec.php @@ -87,22 +87,18 @@ abstract class AMP_Rule_Spec { */ public static $additional_allowed_tags = array( - /** - * An experimental tag with no protoascii - */ + // An experimental tag with no protoascii. 'amp-share-tracking' => array( 'attr_spec_list' => array(), 'tag_spec' => array(), ), - /** - * Needed for some tags such as analytics - */ + // Needed for some tags such as analytics. 'script' => array( 'attr_spec_list' => array( 'type' => array( 'mandatory' => true, - 'value_casei' => 'text/javascript', + 'value_casei' => 'application/json', ), ), 'tag_spec' => array(), From a2dbae473fb848f66b9df14c2f4e2daf762534c9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 19 Jan 2018 17:02:15 -0800 Subject: [PATCH 03/11] Introduce AMP_WP_Styles for doing enqueued scripts inline --- includes/class-amp-autoloader.php | 1 + includes/class-amp-theme-support.php | 48 ++++--- includes/class-amp-wp-styles.php | 180 +++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 16 deletions(-) create mode 100644 includes/class-amp-wp-styles.php diff --git a/includes/class-amp-autoloader.php b/includes/class-amp-autoloader.php index 05bbf021b9d..1376d157e48 100644 --- a/includes/class-amp-autoloader.php +++ b/includes/class-amp-autoloader.php @@ -30,6 +30,7 @@ class AMP_Autoloader { */ private static $_classmap = array( 'AMP_Theme_Support' => 'includes/class-amp-theme-support', + 'AMP_WP_Styles' => 'includes/class-amp-wp-styles', 'AMP_Template_Customizer' => 'includes/admin/class-amp-customizer', 'AMP_Post_Meta_Box' => 'includes/admin/class-amp-post-meta-box', 'AMP_Post_Type_Support' => 'includes/class-amp-post-type-support', diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 7f5c1a3c631..879586375b9 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -151,14 +151,16 @@ public static function register_paired_hooks() { public static function register_hooks() { // Remove core actions which are invalid AMP. - remove_action( 'wp_head', 'locale_stylesheet' ); + remove_action( 'wp_head', 'locale_stylesheet' ); // Replaced below in add_amp_custom_style_placeholder() method. remove_action( 'wp_head', 'print_emoji_detection_script', 7 ); - remove_action( 'wp_head', 'wp_print_styles', 8 ); + remove_action( 'wp_head', 'wp_print_styles', 8 ); // Replaced below in add_amp_custom_style_placeholder() method. remove_action( 'wp_head', 'wp_print_head_scripts', 9 ); - remove_action( 'wp_head', 'wp_custom_css_cb', 101 ); + remove_action( 'wp_head', 'wp_custom_css_cb', 101 ); // Replaced below in add_amp_custom_style_placeholder() method. remove_action( 'wp_footer', 'wp_print_footer_scripts', 20 ); remove_action( 'wp_print_styles', 'print_emoji_styles' ); + add_action( 'wp_enqueue_scripts', array( __CLASS__, 'override_wp_styles' ), -1 ); + /* * Replace core's canonical link functionality with one that outputs links for non-singular queries as well. * See WP Core #18660. @@ -169,11 +171,11 @@ public static function register_hooks() { // @todo Add add_schemaorg_metadata(), add_analytics_data(), etc. // Add additional markup required by AMP . add_action( 'wp_head', array( __CLASS__, 'add_meta_charset' ), 0 ); - add_action( 'wp_head', array( __CLASS__, 'add_meta_viewport' ), 2 ); - add_action( 'wp_head', 'amp_print_boilerplate_code', 3 ); - add_action( 'wp_head', array( __CLASS__, 'add_amp_component_scripts' ), 4 ); - add_action( 'wp_head', array( __CLASS__, 'add_amp_custom_style_placeholder' ), 5 ); - add_action( 'wp_head', 'amp_add_generator_metadata', 6 ); + add_action( 'wp_head', array( __CLASS__, 'add_meta_viewport' ), 5 ); + add_action( 'wp_head', 'amp_print_boilerplate_code', 7 ); + add_action( 'wp_head', array( __CLASS__, 'add_amp_custom_style_placeholder' ), 8 ); // Because wp_print_styles() normally happens at 8. + add_action( 'wp_head', array( __CLASS__, 'add_amp_component_scripts' ), 10 ); + add_action( 'wp_head', 'amp_add_generator_metadata', 20 ); /* * Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone @@ -192,6 +194,17 @@ public static function register_hooks() { // @todo Add character conversion. } + /** + * Preemtively define $wp_styles as AMP_WP_Styles. + * + * @see wp_styles() + * @global AMP_WP_Styles $wp_styles + */ + public static function override_wp_styles() { + global $wp_styles; + $wp_styles = new AMP_WP_Styles(); // WPCS: global override ok. + } + /** * Register content embed handlers. * @@ -376,6 +389,16 @@ public static function add_amp_custom_style_placeholder() { echo ''; + + $wp_styles = wp_styles(); + if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) { + trigger_error( esc_html__( 'wp_styles() does not return an instance of AMP_WP_Styles as required.', 'amp' ), E_USER_WARNING ); // phpcs:ignore + return; + } + + $wp_styles->do_items(); // Normally done at wp_head priority 8. + $wp_styles->do_locale_stylesheet(); // Normally done at wp_head priority 10. + $wp_styles->do_custom_css(); // Normally done at wp_head priority 101. } /** @@ -385,11 +408,7 @@ public static function add_amp_custom_style_placeholder() { * @return string Styles. */ public static function get_amp_custom_styles() { - - // @todo Grab source of all enqueued styles and concatenate here? - // @todo Print contents of get_locale_stylesheet_uri()? - $path = get_template_directory() . '/style.css'; // @todo Honor filter in get_stylesheet_directory_uri()? Style must be local. - $css = file_get_contents( $path ); // phpcs:ignore WordPress.WP.AlternativeFunctions -- It's not a remote file. + $css = wp_styles()->print_code; // Add styles gleaned from sanitizers. foreach ( self::$amp_styles as $selector => $properties ) { @@ -400,9 +419,6 @@ public static function get_amp_custom_styles() { ); } - // Do AMP version of wp_custom_css_cb(). - $css .= wp_get_custom_css(); - /** * Filters AMP custom CSS before it is injected onto the output buffer for the response. * diff --git a/includes/class-amp-wp-styles.php b/includes/class-amp-wp-styles.php new file mode 100644 index 00000000000..e47af1dde0f --- /dev/null +++ b/includes/class-amp-wp-styles.php @@ -0,0 +1,180 @@ +content_url && 0 === strpos( $src, $this->content_url ) ) ) { + $src = $this->base_url . $src; + } + + /** This filter is documented in wp-includes/class.wp-styles.php */ + $src = apply_filters( 'style_loader_src', $src, $handle ); + + // Strip query and fragment from URL. + $src = preg_replace( ':[\?#].+:', '', $src ); + + $src = esc_url_raw( $src ); + + // @todo Explicitly not using includes_url() or content_url() since filters may point outside filesystem? + $includes_url = includes_url( '/' ); + $content_url = content_url( '/' ); + $admin_url = get_admin_url( null, '/' ); + $css_path = null; + if ( 0 === strpos( $src, $content_url ) ) { + $css_path = WP_CONTENT_DIR . substr( $src, strlen( $content_url ) - 1 ); + } elseif ( 0 === strpos( $src, $includes_url ) ) { + $css_path = ABSPATH . WPINC . substr( $src, strlen( $includes_url ) - 1 ); + } elseif ( 0 === strpos( $src, $admin_url ) ) { + $css_path = ABSPATH . 'wp-admin' . substr( $src, strlen( $admin_url ) - 1 ); + } + + if ( ! preg_match( '/\.(css|less|scss|sass)$/i', $css_path ) ) { + /* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */ + return new WP_Error( 'amp_css_path_not_found', sprintf( __( 'Skipped stylesheet %1$s which does not have recognized CSS file extension (%2$s).', 'amp' ), $handle, $src ) ); + } + + if ( ! $css_path || false !== strpos( '../', $css_path ) || 0 !== validate_file( $css_path ) || ! file_exists( $css_path ) ) { + /* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */ + return new WP_Error( 'amp_css_path_not_found', sprintf( __( 'Unable to locate filesystem path for stylesheet %1$s (%2$s).', 'amp' ), $handle, $src ) ); + } + + return $css_path; + } + + /** + * Processes a style dependency. + * + * @since 0.7 + * @see WP_Styles::do_item() + * + * @param string $handle The style's registered handle. + * @return bool True on success, false on failure. + */ + public function do_item( $handle ) { + if ( ! WP_Dependencies::do_item( $handle ) ) { + return false; + } + $obj = $this->registered[ $handle ]; + + // Conditional styles and alternate styles aren't supported in AMP. + if ( isset( $obj->extra['conditional'] ) || isset( $obj->extra['alt'] ) ) { + return false; + } + + if ( isset( $obj->args ) ) { + $media = esc_attr( $obj->args ); + } else { + $media = 'all'; + } + + // A single item may alias a set of items, by having dependencies, but no source. + if ( ! $obj->src ) { + $inline_style = $this->print_inline_style( $handle, false ); + if ( $inline_style ) { + $this->print_code .= $inline_style; + } + return true; + } + + $css_file_path = $this->get_validated_css_file_path( $obj->src, $handle ); + if ( is_wp_error( $css_file_path ) ) { + trigger_error( esc_html( $css_file_path->get_error_message() ), E_USER_WARNING ); // phpcs:ignore + return false; + } + $css_rtl_file_path = ''; + + // Handle RTL styles. + if ( 'rtl' === $this->text_direction && isset( $obj->extra['rtl'] ) && $obj->extra['rtl'] ) { + if ( is_bool( $obj->extra['rtl'] ) || 'replace' === $obj->extra['rtl'] ) { + $suffix = isset( $obj->extra['suffix'] ) ? $obj->extra['suffix'] : ''; + $css_rtl_file_path = $this->get_validated_css_file_path( + str_replace( "{$suffix}.css", "-rtl{$suffix}.css", $obj->src ), + "$handle-rtl" + ); + } else { + $css_rtl_file_path = $this->get_validated_css_file_path( $obj->extra['rtl'], "$handle-rtl" ); + } + + if ( is_wp_error( $css_rtl_file_path ) ) { + trigger_error( esc_html( $css_rtl_file_path->get_error_message() ), E_USER_WARNING ); // phpcs:ignore + $css_rtl_file_path = null; + } elseif ( 'replace' === $obj->extra['rtl'] ) { + $css_file_path = null; + } + } + + // Load the CSS from the filesystem. + foreach ( array_filter( array( $css_file_path, $css_rtl_file_path ) ) as $css_path ) { + $css = file_get_contents( $css_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. + if ( 'all' !== $media ) { + $css .= sprintf( '@media %s { %s }', $media, $css ); + } + $this->print_code .= $css; + } + + // Add inline styles. + $inline_style = $this->print_inline_style( $handle, false ); + if ( $inline_style ) { + $this->print_code .= $inline_style; + } + + return true; + } + + /** + * Get the locale stylesheet if it exists. + * + * @since 0.7 + * @return string CSS. + */ + public function do_locale_stylesheet() { + $src = get_locale_stylesheet_uri(); + if ( ! $src ) { + return; + } + $path = $this->get_validated_css_file_path( $src, get_stylesheet() . '-' . get_locale() ); + if ( is_wp_error( $path ) ) { + return; + } + $this->print_code .= file_get_contents( $path ); // phpcs:ignore -- The path has been validated, and it is not a remote path. + } + + /** + * Append Customizer Custom CSS. + * + * @since 0.7 + * @see wp_custom_css() + * @see wp_custom_css_cb() + */ + public function do_custom_css() { + $this->print_code .= wp_get_custom_css(); + } +} From ed7d0d7da5e3a9bc875f91832796d493d7cd5eab Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 19 Jan 2018 20:33:51 -0800 Subject: [PATCH 04/11] Prevent stylesheets from being done again; add tests for AMP_WP_Styles --- includes/class-amp-wp-styles.php | 48 ++++++- tests/test-class-amp-wp-styles.php | 210 +++++++++++++++++++++++++++++ 2 files changed, 252 insertions(+), 6 deletions(-) create mode 100644 tests/test-class-amp-wp-styles.php diff --git a/includes/class-amp-wp-styles.php b/includes/class-amp-wp-styles.php index e47af1dde0f..d7808d13176 100644 --- a/includes/class-amp-wp-styles.php +++ b/includes/class-amp-wp-styles.php @@ -58,7 +58,7 @@ public function get_validated_css_file_path( $src, $handle ) { if ( ! preg_match( '/\.(css|less|scss|sass)$/i', $css_path ) ) { /* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */ - return new WP_Error( 'amp_css_path_not_found', sprintf( __( 'Skipped stylesheet %1$s which does not have recognized CSS file extension (%2$s).', 'amp' ), $handle, $src ) ); + return new WP_Error( 'amp_css_bad_file_extension', sprintf( __( 'Skipped stylesheet %1$s which does not have recognized CSS file extension (%2$s).', 'amp' ), $handle, $src ) ); } if ( ! $css_path || false !== strpos( '../', $css_path ) || 0 !== validate_file( $css_path ) || ! file_exists( $css_path ) ) { @@ -135,7 +135,7 @@ public function do_item( $handle ) { foreach ( array_filter( array( $css_file_path, $css_rtl_file_path ) ) as $css_path ) { $css = file_get_contents( $css_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. if ( 'all' !== $media ) { - $css .= sprintf( '@media %s { %s }', $media, $css ); + $css = sprintf( '@media %s { %s }', $media, $css ); } $this->print_code .= $css; } @@ -149,32 +149,68 @@ public function do_item( $handle ) { return true; } + /** + * Whether the locale stylesheet was done. + * + * @since 0.7 + * @var bool + */ + protected $did_locale_stylesheet = false; + /** * Get the locale stylesheet if it exists. * * @since 0.7 - * @return string CSS. + * @see locale_stylesheet() + * @return bool Whether locale stylesheet was done. */ public function do_locale_stylesheet() { + if ( $this->did_locale_stylesheet ) { + return true; + } + $src = get_locale_stylesheet_uri(); if ( ! $src ) { - return; + return false; } $path = $this->get_validated_css_file_path( $src, get_stylesheet() . '-' . get_locale() ); if ( is_wp_error( $path ) ) { - return; + return false; } $this->print_code .= file_get_contents( $path ); // phpcs:ignore -- The path has been validated, and it is not a remote path. + $this->did_locale_stylesheet = true; + return true; } + /** + * Whether the Custom CSS was done. + * + * @since 0.7 + * @var bool + */ + protected $did_custom_css = false; + /** * Append Customizer Custom CSS. * * @since 0.7 * @see wp_custom_css() * @see wp_custom_css_cb() + * @return bool Whether locale Custom CSS was done. */ public function do_custom_css() { - $this->print_code .= wp_get_custom_css(); + if ( $this->did_custom_css ) { + return true; + } + + $css = trim( wp_get_custom_css() ); + if ( ! $css ) { + return false; + } + + $this->print_code .= $css; + + $this->did_custom_css = true; + return true; } } diff --git a/tests/test-class-amp-wp-styles.php b/tests/test-class-amp-wp-styles.php new file mode 100644 index 00000000000..df1d64eb2cd --- /dev/null +++ b/tests/test-class-amp-wp-styles.php @@ -0,0 +1,210 @@ +amp_wp_styles(); + $this->assertInstanceOf( 'AMP_WP_Styles', $this->amp_wp_styles() ); + $this->assertInstanceOf( 'AMP_WP_Styles', wp_styles() ); + } + + /** + * Return bad URL. + * + * @return string Bad URL. + */ + public function return_bad_style_loader_src() { + return site_url( 'wp-config.php' ); + } + + /** + * Tests get_validated_css_file_path. + * + * @covers AMP_WP_Styles::get_validated_css_file_path() + */ + public function test_get_validated_css_file_path() { + $wp_styles = $this->amp_wp_styles(); + + // Theme. + $expected = WP_CONTENT_DIR . '/themes/twentyseventeen/style.css'; + $path = $wp_styles->get_validated_css_file_path( '/wp-content/themes/twentyseventeen/style.css', 'twentyseventeen-style' ); + $this->assertEquals( $expected, $path ); + $path = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/style.css' ), 'dashicons' ); + $this->assertEquals( $expected, $path ); + + add_filter( 'style_loader_src', array( $this, 'return_bad_style_loader_src' ) ); + $r = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/style.css' ), 'dashicons' ); + $this->assertInstanceOf( 'WP_Error', $r ); + $this->assertEquals( 'amp_css_bad_file_extension', $r->get_error_code() ); + remove_filter( 'style_loader_src', array( $this, 'return_bad_style_loader_src' ) ); + + // Includes. + $expected = ABSPATH . WPINC . '/css/dashicons.css'; + $path = $wp_styles->get_validated_css_file_path( '/wp-includes/css/dashicons.css', 'dashicons' ); + $this->assertEquals( $expected, $path ); + $path = $wp_styles->get_validated_css_file_path( includes_url( 'css/dashicons.css' ), 'dashicons' ); + $this->assertEquals( $expected, $path ); + + // Admin. + $expected = ABSPATH . 'wp-admin/css/common.css'; + $path = $wp_styles->get_validated_css_file_path( '/wp-admin/css/common.css', 'dashicons' ); + $this->assertEquals( $expected, $path ); + $path = $wp_styles->get_validated_css_file_path( admin_url( 'css/common.css' ), 'common' ); + $this->assertEquals( $expected, $path ); + + // Bad URLs. + $r = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/index.php' ), 'bad' ); + $this->assertInstanceOf( 'WP_Error', $r ); + $this->assertEquals( 'amp_css_bad_file_extension', $r->get_error_code() ); + + $r = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/404.css' ), 'bad' ); + $this->assertInstanceOf( 'WP_Error', $r ); + $this->assertEquals( 'amp_css_path_not_found', $r->get_error_code() ); + } + + /** + * Tests test_do_item. + * + * @covers AMP_WP_Styles::do_item() + */ + public function test_do_item() { + $wp_styles = $this->amp_wp_styles(); + $this->assertFalse( $wp_styles->do_item( 'non-existent' ) ); + + // Conditional stylesheets are ignored. + $wp_styles->registered['buttons-conditional'] = clone $wp_styles->registered['buttons']; + $wp_styles->registered['buttons-conditional']->extra['conditional'] = 'IE8'; + $this->assertFalse( $wp_styles->do_item( 'buttons-conditional' ) ); + + // Alt stylesheets are ignored. + $wp_styles->registered['buttons-alt'] = clone $wp_styles->registered['buttons']; + $wp_styles->registered['buttons-alt']->extra['alt'] = true; + $this->assertFalse( $wp_styles->do_item( 'buttons-alt' ) ); + + // Media. + $wp_styles->registered['admin-bar-print'] = clone $wp_styles->registered['admin-bar']; + $wp_styles->registered['admin-bar-print']->args = 'x_virtual_reality'; + $wp_styles->print_code = ''; + $this->assertTrue( $wp_styles->do_item( 'admin-bar-print' ) ); + $this->assertStringStartsWith( '@media x_virtual_reality {', $wp_styles->print_code ); + + // RTL. + $wp_styles->print_code = ''; + $wp_styles->registered['buttons-arabic'] = clone $wp_styles->registered['buttons']; + $this->assertTrue( $wp_styles->do_item( 'buttons-arabic' ) ); + $this->assertContains( 'text-align: left;', $wp_styles->print_code ); + $wp_styles->print_code = ''; + $wp_styles->text_direction = 'rtl'; + $this->assertTrue( $wp_styles->do_item( 'buttons-arabic' ) ); + $this->assertContains( 'text-align: right;', $wp_styles->print_code ); + $wp_styles->text_direction = 'ltr'; + + // Inline style. + $wp_styles->print_code = ''; + $inline = '/* INLINE STYLE FOR BUTTONS */'; + wp_add_inline_style( 'buttons', $inline ); + $this->assertTrue( $wp_styles->do_item( 'buttons' ) ); + $wp_styles->do_item( 'buttons' ); + $this->assertStringEndsWith( $inline, $wp_styles->print_code ); + $this->assertContains( '.wp-core-ui .button-link', $wp_styles->print_code ); + + // Buttons bad. + $this->setExpectedException( 'PHPUnit_Framework_Error_Warning', 'Skipped stylesheet buttons-bad which does not have recognized CSS file extension (http://example.org/wp-config.php).' ); + $wp_styles->base_url = 'http://example.org'; + $wp_styles->print_code = ''; + $wp_styles->registered['buttons-bad'] = clone $wp_styles->registered['buttons']; + $wp_styles->registered['buttons-bad']->src = $this->return_bad_style_loader_src(); + $this->assertFalse( $wp_styles->do_item( 'buttons-bad' ) ); + $this->assertEmpty( $wp_styles->print_code ); + } + + /** + * Tests do_locale_stylesheet. + * + * @covers AMP_WP_Styles::do_locale_stylesheet() + */ + public function test_do_locale_stylesheet() { + $wp_styles = $this->amp_wp_styles(); + add_filter( 'locale_stylesheet_uri', '__return_false' ); + $this->assertFalse( $wp_styles->do_locale_stylesheet() ); + $this->assertEmpty( $wp_styles->print_code ); + remove_filter( 'locale_stylesheet_uri', '__return_false' ); + + add_filter( 'locale_stylesheet_uri', array( $this, 'return_css_url' ) ); + $this->assertTrue( $wp_styles->do_locale_stylesheet() ); + $this->assertNotEmpty( $wp_styles->print_code ); + } + + /** + * Tests do_custom_css. + * + * @covers AMP_WP_Styles::do_custom_css() + */ + public function test_do_custom_css() { + $wp_styles = $this->amp_wp_styles(); + $this->assertFalse( $wp_styles->do_custom_css() ); + $this->assertEmpty( $wp_styles->print_code ); + + add_filter( 'wp_get_custom_css', array( $this, 'return_css_rule' ) ); + $wp_styles = $this->amp_wp_styles(); + $wp_styles->do_custom_css(); + $this->assertEquals( $this->return_css_rule(), $wp_styles->print_code ); + $wp_styles->do_custom_css(); + $this->assertEquals( $this->return_css_rule(), $wp_styles->print_code ); + remove_filter( 'wp_get_custom_css', array( $this, 'return_css_rule' ) ); + } + + /** + * Return sample CSS rule. + * + * @return string + */ + public function return_css_rule() { + return 'body { color:black; }'; + } + + /** + * Return URL to valid CSS file. + * + * @return string + */ + public function return_css_url() { + return includes_url( 'css/buttons.css' ); + } +} From 9eeab515d4d2f18c431fa654d4fe397d7c08143f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 20 Jan 2018 23:42:04 -0800 Subject: [PATCH 05/11] Remove RTL test since requires a build --- tests/test-class-amp-wp-styles.php | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/test-class-amp-wp-styles.php b/tests/test-class-amp-wp-styles.php index df1d64eb2cd..7108f430245 100644 --- a/tests/test-class-amp-wp-styles.php +++ b/tests/test-class-amp-wp-styles.php @@ -124,17 +124,6 @@ public function test_do_item() { $this->assertTrue( $wp_styles->do_item( 'admin-bar-print' ) ); $this->assertStringStartsWith( '@media x_virtual_reality {', $wp_styles->print_code ); - // RTL. - $wp_styles->print_code = ''; - $wp_styles->registered['buttons-arabic'] = clone $wp_styles->registered['buttons']; - $this->assertTrue( $wp_styles->do_item( 'buttons-arabic' ) ); - $this->assertContains( 'text-align: left;', $wp_styles->print_code ); - $wp_styles->print_code = ''; - $wp_styles->text_direction = 'rtl'; - $this->assertTrue( $wp_styles->do_item( 'buttons-arabic' ) ); - $this->assertContains( 'text-align: right;', $wp_styles->print_code ); - $wp_styles->text_direction = 'ltr'; - // Inline style. $wp_styles->print_code = ''; $inline = '/* INLINE STYLE FOR BUTTONS */'; From 49c121e5c2ad333fc61c8d7651ac9b1f44108014 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 22 Jan 2018 14:29:38 -0800 Subject: [PATCH 06/11] Apply changes for code review * Let AMP_Theme_Support::override_wp_styles() re-set global if not already instantiated. * Move class variable definitions. * Break up conditional into multiple lines. --- includes/class-amp-theme-support.php | 8 ++++-- includes/class-amp-wp-styles.php | 43 ++++++++++++++++------------ tests/test-class-amp-wp-styles.php | 25 +++++----------- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 879586375b9..9e83e6ccfbf 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -195,14 +195,18 @@ public static function register_hooks() { } /** - * Preemtively define $wp_styles as AMP_WP_Styles. + * Override $wp_styles as AMP_WP_Styles, ideally before first instantiated as WP_Styles. * * @see wp_styles() * @global AMP_WP_Styles $wp_styles + * @return AMP_WP_Styles Instance. */ public static function override_wp_styles() { global $wp_styles; - $wp_styles = new AMP_WP_Styles(); // WPCS: global override ok. + if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) { + $wp_styles = new AMP_WP_Styles(); // WPCS: global override ok. + } + return $wp_styles; } /** diff --git a/includes/class-amp-wp-styles.php b/includes/class-amp-wp-styles.php index d7808d13176..f81603070e2 100644 --- a/includes/class-amp-wp-styles.php +++ b/includes/class-amp-wp-styles.php @@ -20,6 +20,22 @@ class AMP_WP_Styles extends WP_Styles { */ public $do_concat = true; + /** + * Whether the locale stylesheet was done. + * + * @since 0.7 + * @var bool + */ + protected $did_locale_stylesheet = false; + + /** + * Whether the Custom CSS was done. + * + * @since 0.7 + * @var bool + */ + protected $did_custom_css = false; + /** * Generates an enqueued style's fully-qualified file path. * @@ -31,7 +47,14 @@ class AMP_WP_Styles extends WP_Styles { * @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error. */ public function get_validated_css_file_path( $src, $handle ) { - if ( ! is_bool( $src ) && ! preg_match( '|^(https?:)?//|', $src ) && ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) ) { + $needs_base_url = ( + ! is_bool( $src ) + && + ! preg_match( '|^(https?:)?//|', $src ) + && + ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) + ); + if ( $needs_base_url ) { $src = $this->base_url . $src; } @@ -40,10 +63,8 @@ public function get_validated_css_file_path( $src, $handle ) { // Strip query and fragment from URL. $src = preg_replace( ':[\?#].+:', '', $src ); - $src = esc_url_raw( $src ); - // @todo Explicitly not using includes_url() or content_url() since filters may point outside filesystem? $includes_url = includes_url( '/' ); $content_url = content_url( '/' ); $admin_url = get_admin_url( null, '/' ); @@ -149,14 +170,6 @@ public function do_item( $handle ) { return true; } - /** - * Whether the locale stylesheet was done. - * - * @since 0.7 - * @var bool - */ - protected $did_locale_stylesheet = false; - /** * Get the locale stylesheet if it exists. * @@ -182,14 +195,6 @@ public function do_locale_stylesheet() { return true; } - /** - * Whether the Custom CSS was done. - * - * @since 0.7 - * @var bool - */ - protected $did_custom_css = false; - /** * Append Customizer Custom CSS. * diff --git a/tests/test-class-amp-wp-styles.php b/tests/test-class-amp-wp-styles.php index 7108f430245..b2f2c9f25a4 100644 --- a/tests/test-class-amp-wp-styles.php +++ b/tests/test-class-amp-wp-styles.php @@ -22,25 +22,13 @@ public function tearDown() { $wp_styles = null; } - /** - * Get wp_styles(). - * - * @return WP_Styles|AMP_WP_Styles Styles. - */ - protected function amp_wp_styles() { - global $wp_styles; - $wp_styles = new AMP_WP_Styles(); // phpcs:ignore - return $wp_styles; - } - /** * Test that wp_styles() returns AMP_WP_Styles. * * @covers wp_styles() */ public function test_wp_styles() { - $this->amp_wp_styles(); - $this->assertInstanceOf( 'AMP_WP_Styles', $this->amp_wp_styles() ); + AMP_Theme_Support::override_wp_styles(); $this->assertInstanceOf( 'AMP_WP_Styles', wp_styles() ); } @@ -59,7 +47,7 @@ public function return_bad_style_loader_src() { * @covers AMP_WP_Styles::get_validated_css_file_path() */ public function test_get_validated_css_file_path() { - $wp_styles = $this->amp_wp_styles(); + $wp_styles = AMP_Theme_Support::override_wp_styles(); // Theme. $expected = WP_CONTENT_DIR . '/themes/twentyseventeen/style.css'; @@ -104,7 +92,7 @@ public function test_get_validated_css_file_path() { * @covers AMP_WP_Styles::do_item() */ public function test_do_item() { - $wp_styles = $this->amp_wp_styles(); + $wp_styles = AMP_Theme_Support::override_wp_styles(); $this->assertFalse( $wp_styles->do_item( 'non-existent' ) ); // Conditional stylesheets are ignored. @@ -149,7 +137,7 @@ public function test_do_item() { * @covers AMP_WP_Styles::do_locale_stylesheet() */ public function test_do_locale_stylesheet() { - $wp_styles = $this->amp_wp_styles(); + $wp_styles = AMP_Theme_Support::override_wp_styles(); add_filter( 'locale_stylesheet_uri', '__return_false' ); $this->assertFalse( $wp_styles->do_locale_stylesheet() ); $this->assertEmpty( $wp_styles->print_code ); @@ -166,12 +154,13 @@ public function test_do_locale_stylesheet() { * @covers AMP_WP_Styles::do_custom_css() */ public function test_do_custom_css() { - $wp_styles = $this->amp_wp_styles(); + $wp_styles = AMP_Theme_Support::override_wp_styles(); $this->assertFalse( $wp_styles->do_custom_css() ); $this->assertEmpty( $wp_styles->print_code ); add_filter( 'wp_get_custom_css', array( $this, 'return_css_rule' ) ); - $wp_styles = $this->amp_wp_styles(); + $wp_styles = null; + $wp_styles = AMP_Theme_Support::override_wp_styles(); $wp_styles->do_custom_css(); $this->assertEquals( $this->return_css_rule(), $wp_styles->print_code ); $wp_styles->do_custom_css(); From a6662896c66699ea2c726ecb25e9369918fd497d Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 17 Jan 2018 09:31:12 +0200 Subject: [PATCH 07/11] Make add `get_dom` method to AMP_DOM_Utils --- includes/utils/class-amp-dom-utils.php | 47 +++++++++++++++++++------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php index d5f54bcbbf4..87122347667 100644 --- a/includes/utils/class-amp-dom-utils.php +++ b/includes/utils/class-amp-dom-utils.php @@ -13,17 +13,15 @@ class AMP_DOM_Utils { /** - * Return a valid DOMDocument representing arbitrary HTML content passed as a parameter. - * - * @see Reciprocal function get_content_from_dom() + * Return a valid DOMDocument representing HTML document passed as a parameter. * - * @since 0.2 + * @since 0.7 * - * @param string $content Valid HTML content to be represented by a DOMDocument. + * @param string $document Valid HTML document to be represented by a DOMDocument. * * @return DOMDocument|false Returns DOMDocument, or false if conversion failed. */ - public static function get_dom_from_content( $content ) { + public static function get_dom( $document ) { $libxml_previous_state = libxml_use_internal_errors( true ); $dom = new DOMDocument(); @@ -34,13 +32,7 @@ public static function get_dom_from_content( $content ) { * We can later use this to extract our nodes. * Add charset so loadHTML does not have problems parsing it. */ - $result = $dom->loadHTML( - sprintf( - '%s', - get_bloginfo( 'charset' ), - $content - ) - ); + $result = $dom->loadHTML( $document ); libxml_clear_errors(); libxml_use_internal_errors( $libxml_previous_state ); @@ -52,6 +44,35 @@ public static function get_dom_from_content( $content ) { return $dom; } + /** + * Return a valid DOMDocument representing arbitrary HTML content passed as a parameter. + * + * @see Reciprocal function get_content_from_dom() + * + * @since 0.2 + * + * @param string $content Valid HTML content to be represented by a DOMDocument. + * + * @return DOMDocument|false Returns DOMDocument, or false if conversion failed. + */ + public static function get_dom_from_content( $content ) { + /* + * Wrap in dummy tags, since XML needs one parent node. + * It also makes it easier to loop through nodes. + * We can later use this to extract our nodes. + * Add utf-8 charset so loadHTML does not have problems parsing it. + * See: http://php.net/manual/en/domdocument.loadhtml.php#78243 + */ + $document = sprintf( + '%s', + get_bloginfo( 'charset' ), + $content + ); + + return self::get_dom( $document ); + + } + /** * Return valid HTML content extracted from the DOMDocument passed as a parameter. * From 70a444e10f23aa8bf6b1d500edda9648548150c9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 22 Jan 2018 19:22:09 -0800 Subject: [PATCH 08/11] Sanitize the output-buffered HTML document response --- includes/class-amp-theme-support.php | 47 +++++++++--------- .../templates/class-amp-content-sanitizer.php | 17 +++++-- includes/utils/class-amp-dom-utils.php | 2 + tests/test-class-amp-theme-support.php | 49 +++++++++++++++++++ 4 files changed, 87 insertions(+), 28 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index c5100e90128..0dc51a7a7d1 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -191,8 +191,6 @@ public static function register_hooks() { */ add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ), 0 ); - add_filter( 'the_content', array( __CLASS__, 'filter_the_content' ), PHP_INT_MAX ); - // @todo Add character conversion. } @@ -422,32 +420,12 @@ public static function get_amp_custom_styles() { return $css; } - /** - * Filter the content to be valid AMP. - * - * @param string $content Content. - * @return string Amplified content. - */ - public static function filter_the_content( $content ) { - $args = array( - 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat. - ); - - list( $sanitized_content, $scripts, $styles ) = AMP_Content_Sanitizer::sanitize( $content, self::$sanitizer_classes, $args ); - - self::$amp_scripts = array_merge( self::$amp_scripts, $scripts ); - self::$amp_styles = array_merge( self::$amp_styles, $styles ); - - return $sanitized_content; - } - /** * Determine required AMP scripts. * - * @param string $html Output HTML. * @return string Scripts to inject into the HEAD. */ - public static function get_amp_component_scripts( $html ) { + public static function get_amp_component_scripts() { $amp_scripts = self::$amp_scripts; foreach ( self::$embed_handlers as $embed_handler ) { @@ -492,10 +470,32 @@ public static function start_output_buffering() { * Finish output buffering. * * @todo Do this in shutdown instead of output buffering callback? + * @global int $content_width * @param string $output Buffered output. * @return string Finalized output. */ public static function finish_output_buffering( $output ) { + global $content_width; + + $dom = AMP_DOM_Utils::get_dom( $output ); + $args = array( + 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat. + ); + + /* + * @todo The sanitize method needs to be updated to sanitize the entire HTML element and not just the BODY. + * This will require updating mandatory_parent_blacklist in amphtml-update.py to include elements that appear in the HEAD. + * This will ensure that the scripts and styles that plugins output via wp_head() will be sanitized as well. However, + * since the the old paired mode is sending content from the *body* we'll need to be able to filter out the elements + * from outside the body from being part of the whitelist sanitizer when it runs when theme support is not present, + * as otherwise elements from the HEAD could get added to the BODY. + */ + list( $sanitized_inner_body, $scripts, $styles ) = AMP_Content_Sanitizer::sanitize( $dom, self::$sanitizer_classes, $args ); + + self::$amp_scripts = array_merge( self::$amp_scripts, $scripts ); + self::$amp_styles = array_merge( self::$amp_styles, $styles ); + + $output = preg_replace( '#()(.+)()#si', '$1' . $sanitized_inner_body . '$3', $output ); // Inject required scripts. $output = preg_replace( @@ -513,7 +513,6 @@ public static function finish_output_buffering( $output ) { 1 ); - // @todo Add more validation checking and potentially the whitelist sanitizer. return $output; } } diff --git a/includes/templates/class-amp-content-sanitizer.php b/includes/templates/class-amp-content-sanitizer.php index dcbd62097ef..eabfefe74bf 100644 --- a/includes/templates/class-amp-content-sanitizer.php +++ b/includes/templates/class-amp-content-sanitizer.php @@ -13,16 +13,20 @@ class AMP_Content_Sanitizer { /** * Sanitize. * - * @param string $content Content. - * @param string[] $sanitizer_classes Sanitizer classes. - * @param array $global_args Global args. + * @param string|DOMDocument $content HTML content string or DOM document. + * @param string[] $sanitizer_classes Sanitizer classes. + * @param array $global_args Global args. * * @return array */ public static function sanitize( $content, array $sanitizer_classes, $global_args = array() ) { $scripts = array(); $styles = array(); - $dom = AMP_DOM_Utils::get_dom_from_content( $content ); + if ( $content instanceof DOMDocument ) { + $dom = $content; + } else { + $dom = AMP_DOM_Utils::get_dom_from_content( $content ); + } foreach ( $sanitizer_classes as $sanitizer_class => $args ) { if ( ! class_exists( $sanitizer_class ) ) { @@ -31,6 +35,11 @@ public static function sanitize( $content, array $sanitizer_classes, $global_arg continue; } + /** + * Sanitizer. + * + * @type AMP_Base_Sanitizer $sanitizer + */ $sanitizer = new $sanitizer_class( $dom, array_merge( $global_args, $args ) ); if ( ! is_subclass_of( $sanitizer, 'AMP_Base_Sanitizer' ) ) { diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php index 87122347667..e6a8b96918f 100644 --- a/includes/utils/class-amp-dom-utils.php +++ b/includes/utils/class-amp-dom-utils.php @@ -88,6 +88,8 @@ public static function get_content_from_dom( $dom ) { /** * We only want children of the body tag, since we have a subset of HTML. + * + * @todo We will want to get the full HTML eventually. */ $body = $dom->getElementsByTagName( 'body' )->item( 0 ); diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 5a10663bfb7..0b15e7f1165 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -67,4 +67,53 @@ public function test_is_paired_available() { $this->assertTrue( is_search() ); $this->assertFalse( AMP_Theme_Support::is_paired_available() ); } + + /** + * Test finish_output_buffering. + * + * @covers AMP_Theme_Support::finish_output_buffering() + */ + public function test_finish_output_buffering() { + add_theme_support( 'amp' ); + AMP_Theme_Support::init(); + ob_start(); + ?> + + > + + + + + + + + + + + + assertContains( '', $sanitized_html ); + $this->assertContains( '', $sanitized_html ); + $this->assertContains( '