From a2dbae473fb848f66b9df14c2f4e2daf762534c9 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 19 Jan 2018 17:02:15 -0800 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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();