From 218fc9da717b08a5b07da160f05c3f14a3799457 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 31 Jan 2018 00:44:17 -0600 Subject: [PATCH 01/11] Issue #864: Address an issue in 'Video' widgets for YouTube and Vimeo. These widgets have different logic in: WP_Widget_Media_Video::render() So override the value of wp_video_shortcode(), which renders these. Produce 'youtube' or 'vimeo' shortcodes. And this plugin's handlers will create and . Another plugin is also free to override those shortcodes. --- includes/class-amp-theme-support.php | 31 ++++++++++++++++++++++ tests/test-class-amp-theme-support.php | 36 ++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 8d5529afc19..e7b0decce29 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -177,6 +177,7 @@ public static function register_hooks() { 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 ); + add_filter( 'wp_video_shortcode_override', array( __CLASS__, 'video_override' ), 10, 2 ); /* * Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone @@ -470,6 +471,36 @@ public static function add_amp_component_scripts() { echo self::COMPONENT_SCRIPTS_PLACEHOLDER; // phpcs:ignore WordPress.Security.EscapeOutput, WordPress.XSS.EscapeOutput } + /** + * Overrides the output of 'Video' widgets for YouTube and Vimeo. + * + * WP_Widget_Media_Video::render() has a different output for YouTube and Vimeo. + * It calls wp_video_shortcode(), instead of wp_oembed_get() like the rest of the videos. + * So this callback overrides the output of wp_video_shortcode(). + * It produces 'youtube' or 'vimeo' shortcodes, + * which this plugin's embed handlers render as or . + * This sometimes returns an empty string: ''. + * If the value is anything other than '', wp_video_shortcode() exits and returns the value. + * + * @param string $html Empty string. + * @param array $attr The shortcode attributes. + * @return string $markup The markup to output. + */ + public static function video_override( $html, $attr ) { + if ( ! isset( $attr['src'] ) ) { + return ''; + } + $src = $attr['src']; + $youtube_pattern = '#^https?://(?:www\.)?(?:youtube\.com/watch|youtu\.be/)#'; + $vimeo_pattern = '#^https?://(.+\.)?vimeo\.com/.*#'; + if ( 1 === preg_match( $youtube_pattern, $src ) ) { + return do_shortcode( sprintf( '[youtube %s]', $src ) ); + } elseif ( 1 === preg_match( $vimeo_pattern, $src ) ) { + return do_shortcode( sprintf( '[vimeo %s]', $src ) ); + } + return ''; + } + /** * Get canonical URL for current request. * diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 00269e5b657..c1b4694d7d9 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -87,6 +87,42 @@ public function test_register_widgets() { $this->assertArrayHasKey( 'AMP_Widget_Categories', $wp_widget_factory->widgets ); } + /** + * Test video_override(). + * + * @covers AMP_Theme_Support::video_override() + */ + public function test_video_override() { + AMP_Theme_Support::register_content_embed_handlers(); + $youtube_id = 'XOY3ZUO6P0k'; + $youtube_src = 'https://youtu.be/' . $youtube_id; + $attr_youtube = array( + 'src' => $youtube_src, + ); + $youtube_shortcode = AMP_Theme_Support::video_override( '', $attr_youtube ); + $this->assertContains( 'assertContains( $youtube_id, $youtube_shortcode ); + + $vimeo_id = '64086087'; + $vimeo_src = 'https://vimeo.com/' . $vimeo_id; + $attr_vimeo = array( + 'src' => $vimeo_src, + ); + $yimeo_shortcode = AMP_Theme_Support::video_override( '', $attr_vimeo ); + $this->assertContains( 'assertContains( $vimeo_id, $yimeo_shortcode ); + + $daily_motion_id = 'x6bacgf'; + $daily_motion_src = 'http://www.dailymotion.com/video/' . $daily_motion_id; + $attr_daily_motion = array( + 'src' => $daily_motion_src, + ); + $daily_motion_shortcode = AMP_Theme_Support::video_override( '', $attr_daily_motion ); + $this->assertEquals( '', $daily_motion_shortcode ); + $no_attributes = AMP_Theme_Support::video_override( '', array() ); + $this->assertEquals( '', $no_attributes ); + } + /** * Test finish_output_buffering. * From cb0f071b8d95bae91b164d5ee26f2065402aab9b Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 31 Jan 2018 00:59:54 -0600 Subject: [PATCH 02/11] Issue #864: Only override video widget output if is_amp_endpoint(). Similarly to how the other widget overrides work. Also, adjust tests for this. --- includes/class-amp-theme-support.php | 2 +- tests/test-class-amp-theme-support.php | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index e7b0decce29..404919d054d 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -487,7 +487,7 @@ public static function add_amp_component_scripts() { * @return string $markup The markup to output. */ public static function video_override( $html, $attr ) { - if ( ! isset( $attr['src'] ) ) { + if ( ! is_amp_endpoint() || ! isset( $attr['src'] ) ) { return ''; } $src = $attr['src']; diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index c1b4694d7d9..df45ff6d1ea 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -93,12 +93,19 @@ public function test_register_widgets() { * @covers AMP_Theme_Support::video_override() */ public function test_video_override() { + global $_wp_theme_features; AMP_Theme_Support::register_content_embed_handlers(); $youtube_id = 'XOY3ZUO6P0k'; $youtube_src = 'https://youtu.be/' . $youtube_id; $attr_youtube = array( 'src' => $youtube_src, ); + + // This isn't an AMP endpoint, so there should be no filtering of the shortcode. + $non_overridden = AMP_Theme_Support::video_override( '', $attr_youtube ); + $this->assertEquals( '', $non_overridden ); + + $_wp_theme_features['amp'] = true; // WPCS: global override ok. $youtube_shortcode = AMP_Theme_Support::video_override( '', $attr_youtube ); $this->assertContains( 'assertContains( $youtube_id, $youtube_shortcode ); From 6608df58705b5c9f709d141c76c36b78a8269b2e Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 31 Jan 2018 01:25:07 -0600 Subject: [PATCH 03/11] Issue #864: Align equals signs in response to Travis error. --- tests/test-class-amp-theme-support.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index df45ff6d1ea..b7b2f10f3f5 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -95,9 +95,9 @@ public function test_register_widgets() { public function test_video_override() { global $_wp_theme_features; AMP_Theme_Support::register_content_embed_handlers(); - $youtube_id = 'XOY3ZUO6P0k'; - $youtube_src = 'https://youtu.be/' . $youtube_id; - $attr_youtube = array( + $youtube_id = 'XOY3ZUO6P0k'; + $youtube_src = 'https://youtu.be/' . $youtube_id; + $attr_youtube = array( 'src' => $youtube_src, ); @@ -106,7 +106,7 @@ public function test_video_override() { $this->assertEquals( '', $non_overridden ); $_wp_theme_features['amp'] = true; // WPCS: global override ok. - $youtube_shortcode = AMP_Theme_Support::video_override( '', $attr_youtube ); + $youtube_shortcode = AMP_Theme_Support::video_override( '', $attr_youtube ); $this->assertContains( 'assertContains( $youtube_id, $youtube_shortcode ); From 76cf75fed741d9dc7971d6f86399ace538a23324 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 31 Jan 2018 01:51:06 -0600 Subject: [PATCH 04/11] Issue #864: Simply return the initial value, instead of ''. This will allow another plugin to override this. Inspired by the Jetpack plugin's return of the initial value. --- includes/class-amp-theme-support.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 404919d054d..7de0cb7dbd7 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -488,7 +488,7 @@ public static function add_amp_component_scripts() { */ public static function video_override( $html, $attr ) { if ( ! is_amp_endpoint() || ! isset( $attr['src'] ) ) { - return ''; + return $html; } $src = $attr['src']; $youtube_pattern = '#^https?://(?:www\.)?(?:youtube\.com/watch|youtu\.be/)#'; @@ -498,7 +498,7 @@ public static function video_override( $html, $attr ) { } elseif ( 1 === preg_match( $vimeo_pattern, $src ) ) { return do_shortcode( sprintf( '[vimeo %s]', $src ) ); } - return ''; + return $html; } /** From 9fab09261a4ddf91e1602c4c83bc00d125b0601f Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 31 Jan 2018 02:27:07 -0600 Subject: [PATCH 05/11] Issue #864: Remove the 'Gallery' widget subclass, in favor of a filter. Thanks to Weston for mentioning how we could use 'post_gallery.' Using this removes the need to subclass that widget. The widget calls gallery_shortcode(), which has the filter: 'post_gallery' As Weston mentioned, Jetpack filters 'post_gallery'. --- includes/class-amp-autoloader.php | 1 - includes/class-amp-theme-support.php | 2 +- includes/embeds/class-amp-gallery-embed.php | 20 ++++- .../class-amp-widget-media-gallery.php | 67 ---------------- tests/test-class-amp-widget-media-gallery.php | 77 ------------------- 5 files changed, 17 insertions(+), 150 deletions(-) delete mode 100644 includes/widgets/class-amp-widget-media-gallery.php delete mode 100644 tests/test-class-amp-widget-media-gallery.php diff --git a/includes/class-amp-autoloader.php b/includes/class-amp-autoloader.php index 5c51b42af09..6ea255378b2 100644 --- a/includes/class-amp-autoloader.php +++ b/includes/class-amp-autoloader.php @@ -88,7 +88,6 @@ class AMP_Autoloader { 'AMP_WP_Utils' => 'includes/utils/class-amp-wp-utils', 'AMP_Widget_Archives' => 'includes/widgets/class-amp-widget-archives', 'AMP_Widget_Categories' => 'includes/widgets/class-amp-widget-categories', - 'AMP_Widget_Media_Gallery' => 'includes/widgets/class-amp-widget-media-gallery', 'AMP_Widget_Recent_Comments' => 'includes/widgets/class-amp-widget-recent-comments', 'WPCOM_AMP_Polldaddy_Embed' => 'wpcom/class-amp-polldaddy-embed', 'AMP_Test_Stub_Sanitizer' => 'tests/stubs', diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 7de0cb7dbd7..41f369e3c99 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -482,7 +482,7 @@ public static function add_amp_component_scripts() { * This sometimes returns an empty string: ''. * If the value is anything other than '', wp_video_shortcode() exits and returns the value. * - * @param string $html Empty string. + * @param string $html Markup to possibly override. * @param array $attr The shortcode attributes. * @return string $markup The markup to output. */ diff --git a/includes/embeds/class-amp-gallery-embed.php b/includes/embeds/class-amp-gallery-embed.php index 1261d38747b..46d44fd6951 100644 --- a/includes/embeds/class-amp-gallery-embed.php +++ b/includes/embeds/class-amp-gallery-embed.php @@ -16,15 +16,13 @@ class AMP_Gallery_Embed_Handler extends AMP_Base_Embed_Handler { * Register embed. */ public function register_embed() { - add_shortcode( 'gallery', array( $this, 'shortcode' ) ); + add_filter( 'post_gallery', array( $this, 'override_gallery' ), 10, 2 ); } /** * Unregister embed. */ - public function unregister_embed() { - remove_shortcode( 'gallery' ); - } + public function unregister_embed() {} /** * Shortcode handler. @@ -120,6 +118,20 @@ public function shortcode( $attr ) { ) ); } + /** + * Override the output of gallery_shortcode(). + * + * The 'Gallery' widget also uses this function. + * This ensures that it outputs an . + * + * @param string $html Markup to filter, possibly ''. + * @param array $attributes Shortcode attributes. + * @return string $html Markup for the gallery. + */ + public function override_gallery( $html, $attributes ) { + return $this->shortcode( $attributes ); + } + /** * Render. * diff --git a/includes/widgets/class-amp-widget-media-gallery.php b/includes/widgets/class-amp-widget-media-gallery.php deleted file mode 100644 index e836025a986..00000000000 --- a/includes/widgets/class-amp-widget-media-gallery.php +++ /dev/null @@ -1,67 +0,0 @@ -get_instance_schema(), 'default' ), $instance ); - $shortcode_atts = array_merge( - $instance, - array( - 'link' => $instance['link_type'], - ) - ); - - if ( isset( $instance['orderby_random'] ) && ( true === $instance['orderby_random'] ) ) { - $shortcode_atts['orderby'] = 'rand'; - } - - /* - * The following calls do_shortcode() in case another plugin overrides the gallery shortcode. - * The AMP_Gallery_Embed_Handler in the plugin is doing this itself, but other plugins may - * do it as well, so this ensures that a plugin has the option to override the gallery behavior - * via registering a different gallery shortcode handler. The shortcode serialization can be - * eliminated once WP Trac #25435 is merged and the Gallery widget uses the proposed do_single_shortcode(). - */ - $shortcode_atts_str = ''; - if ( is_array( $shortcode_atts['ids'] ) ) { - $shortcode_atts['ids'] = join( ',', $shortcode_atts['ids'] ); - } - foreach ( $shortcode_atts as $key => $value ) { - $shortcode_atts_str .= sprintf( ' %s="%s"', $key, esc_attr( $value ) ); - } - echo do_shortcode( "[gallery $shortcode_atts_str]" ); // WPCS: XSS ok. - } - - } -} diff --git a/tests/test-class-amp-widget-media-gallery.php b/tests/test-class-amp-widget-media-gallery.php deleted file mode 100644 index 281dd205adb..00000000000 --- a/tests/test-class-amp-widget-media-gallery.php +++ /dev/null @@ -1,77 +0,0 @@ -markTestSkipped( 'This WordPress version does not have a Gallery widget.' ); - } - parent::setUp(); - AMP_Theme_Support::register_widgets(); - $this->instance = new AMP_Widget_Media_Gallery(); - } - - /** - * Test render_media(). - * - * @see AMP_Widget_Media_Gallery::widget(). - */ - public function test_render_media() { - $first_test_image = '/tmp/test-image.jpg'; - copy( DIR_TESTDATA . '/images/test-image.jpg', $first_test_image ); - $first_attachment_id = self::factory()->attachment->create_object( array( - 'file' => $first_test_image, - 'post_parent' => 0, - 'post_mime_type' => 'image/jpeg', - 'post_title' => 'Test Image', - ) ); - wp_update_attachment_metadata( $first_attachment_id, wp_generate_attachment_metadata( $first_attachment_id, $first_test_image ) ); - $ids[] = $first_attachment_id; - - $second_test_image = '/tmp/test-image.jpg'; - copy( DIR_TESTDATA . '/images/test-image.jpg', $second_test_image ); - $second_attachment_id = self::factory()->attachment->create_object( array( - 'file' => $second_test_image, - 'post_parent' => 0, - 'post_mime_type' => 'image/jpeg', - 'post_title' => 'Test Image', - ) ); - wp_update_attachment_metadata( $second_attachment_id, wp_generate_attachment_metadata( $second_attachment_id, $second_test_image ) ); - $ids[] = $second_attachment_id; - $instance = array( - 'title' => 'Test Gallery Widget', - 'ids' => $ids, - ); - - ob_start(); - $this->instance->render_media( $instance ); - $output = ob_get_clean(); - - $this->assertContains( 'amp-carousel', $output ); - $this->assertContains( $first_test_image, $output ); - $this->assertContains( $second_test_image, $output ); - } - -} From 7c1f8fe9f80fc7c948433596e60f602639843093 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Feb 2018 16:55:56 -0600 Subject: [PATCH 06/11] Issue #864: Move video override logic into embed handlers. These are now in the embed classes. Also, create test classes for them. --- includes/class-amp-theme-support.php | 31 --------- includes/embeds/class-amp-vimeo-embed.php | 24 +++++++ includes/embeds/class-amp-youtube-embed.php | 26 ++++++- tests/test-class-amp-theme-support.php | 43 ------------ tests/test-class-amp-vimeo-embed-handler.php | 69 +++++++++++++++++++ .../test-class-amp-youtube-embed-handler.php | 69 +++++++++++++++++++ 6 files changed, 187 insertions(+), 75 deletions(-) create mode 100644 tests/test-class-amp-vimeo-embed-handler.php create mode 100644 tests/test-class-amp-youtube-embed-handler.php diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 41f369e3c99..8d5529afc19 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -177,7 +177,6 @@ public static function register_hooks() { 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 ); - add_filter( 'wp_video_shortcode_override', array( __CLASS__, 'video_override' ), 10, 2 ); /* * Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone @@ -471,36 +470,6 @@ public static function add_amp_component_scripts() { echo self::COMPONENT_SCRIPTS_PLACEHOLDER; // phpcs:ignore WordPress.Security.EscapeOutput, WordPress.XSS.EscapeOutput } - /** - * Overrides the output of 'Video' widgets for YouTube and Vimeo. - * - * WP_Widget_Media_Video::render() has a different output for YouTube and Vimeo. - * It calls wp_video_shortcode(), instead of wp_oembed_get() like the rest of the videos. - * So this callback overrides the output of wp_video_shortcode(). - * It produces 'youtube' or 'vimeo' shortcodes, - * which this plugin's embed handlers render as or . - * This sometimes returns an empty string: ''. - * If the value is anything other than '', wp_video_shortcode() exits and returns the value. - * - * @param string $html Markup to possibly override. - * @param array $attr The shortcode attributes. - * @return string $markup The markup to output. - */ - public static function video_override( $html, $attr ) { - if ( ! is_amp_endpoint() || ! isset( $attr['src'] ) ) { - return $html; - } - $src = $attr['src']; - $youtube_pattern = '#^https?://(?:www\.)?(?:youtube\.com/watch|youtu\.be/)#'; - $vimeo_pattern = '#^https?://(.+\.)?vimeo\.com/.*#'; - if ( 1 === preg_match( $youtube_pattern, $src ) ) { - return do_shortcode( sprintf( '[youtube %s]', $src ) ); - } elseif ( 1 === preg_match( $vimeo_pattern, $src ) ) { - return do_shortcode( sprintf( '[vimeo %s]', $src ) ); - } - return $html; - } - /** * Get canonical URL for current request. * diff --git a/includes/embeds/class-amp-vimeo-embed.php b/includes/embeds/class-amp-vimeo-embed.php index 6e0754b227e..6b334ce6441 100644 --- a/includes/embeds/class-amp-vimeo-embed.php +++ b/includes/embeds/class-amp-vimeo-embed.php @@ -32,6 +32,7 @@ function __construct( $args = array() ) { function register_embed() { wp_embed_register_handler( 'amp-vimeo', self::URL_PATTERN, array( $this, 'oembed' ), -1 ); add_shortcode( 'vimeo', array( $this, 'shortcode' ) ); + add_filter( 'wp_video_shortcode_override', array( $this, 'video_override' ), 10, 2 ); } public function unregister_embed() { @@ -107,4 +108,27 @@ private function get_video_id_from_url( $url ) { return $video_id; } + + /** + * Override the output of Vimeo videos. + * + * This overrides the value in wp_video_shortcode(). + * The pattern matching is copied from WP_Widget_Media_Video::render(). + * + * @param string $html Empty variable to be replaced with shortcode markup. + * @param array $attr The shortcode attributes. + * @return string|null $markup The markup to output. + */ + public function video_override( $html, $attr ) { + if ( ! isset( $attr['src'] ) ) { + return $html; + } + $src = $attr['src']; + $vimeo_pattern = '#^https?://(.+\.)?vimeo\.com/.*#'; + if ( 1 === preg_match( $vimeo_pattern, $src ) ) { + return $this->shortcode( array( $src ) ); + } + return $html; + } + } diff --git a/includes/embeds/class-amp-youtube-embed.php b/includes/embeds/class-amp-youtube-embed.php index be1b3217a5a..a019a6a6334 100644 --- a/includes/embeds/class-amp-youtube-embed.php +++ b/includes/embeds/class-amp-youtube-embed.php @@ -24,7 +24,7 @@ function __construct( $args = array() ) { if ( isset( $this->args['content_max_width'] ) ) { $max_width = $this->args['content_max_width']; - $this->args['width'] = $max_width; + $this->args['width'] = $max_width; $this->args['height'] = round( $max_width * self::RATIO ); } } @@ -32,6 +32,7 @@ function __construct( $args = array() ) { function register_embed() { wp_embed_register_handler( 'amp-youtube', self::URL_PATTERN, array( $this, 'oembed' ), -1 ); add_shortcode( 'youtube', array( $this, 'shortcode' ) ); + add_filter( 'wp_video_shortcode_override', array( $this, 'video_override' ), 10, 2 ); } public function unregister_embed() { @@ -125,4 +126,27 @@ private function sanitize_v_arg( $value ) { return $value; } + + /** + * Override the output of YouTube videos. + * + * This overrides the value in wp_video_shortcode(). + * The pattern matching is copied from WP_Widget_Media_Video::render(). + * + * @param string $html Empty variable to be replaced with shortcode markup. + * @param array $attr The shortcode attributes. + * @return string|null $markup The markup to output. + */ + public function video_override( $html, $attr ) { + if ( ! isset( $attr['src'] ) ) { + return $html; + } + $src = $attr['src']; + $youtube_pattern = '#^https?://(?:www\.)?(?:youtube\.com/watch|youtu\.be/)#'; + if ( 1 === preg_match( $youtube_pattern, $src ) ) { + return $this->shortcode( array( $src ) ); + } + return $html; + } + } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index b7b2f10f3f5..00269e5b657 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -87,49 +87,6 @@ public function test_register_widgets() { $this->assertArrayHasKey( 'AMP_Widget_Categories', $wp_widget_factory->widgets ); } - /** - * Test video_override(). - * - * @covers AMP_Theme_Support::video_override() - */ - public function test_video_override() { - global $_wp_theme_features; - AMP_Theme_Support::register_content_embed_handlers(); - $youtube_id = 'XOY3ZUO6P0k'; - $youtube_src = 'https://youtu.be/' . $youtube_id; - $attr_youtube = array( - 'src' => $youtube_src, - ); - - // This isn't an AMP endpoint, so there should be no filtering of the shortcode. - $non_overridden = AMP_Theme_Support::video_override( '', $attr_youtube ); - $this->assertEquals( '', $non_overridden ); - - $_wp_theme_features['amp'] = true; // WPCS: global override ok. - $youtube_shortcode = AMP_Theme_Support::video_override( '', $attr_youtube ); - $this->assertContains( 'assertContains( $youtube_id, $youtube_shortcode ); - - $vimeo_id = '64086087'; - $vimeo_src = 'https://vimeo.com/' . $vimeo_id; - $attr_vimeo = array( - 'src' => $vimeo_src, - ); - $yimeo_shortcode = AMP_Theme_Support::video_override( '', $attr_vimeo ); - $this->assertContains( 'assertContains( $vimeo_id, $yimeo_shortcode ); - - $daily_motion_id = 'x6bacgf'; - $daily_motion_src = 'http://www.dailymotion.com/video/' . $daily_motion_id; - $attr_daily_motion = array( - 'src' => $daily_motion_src, - ); - $daily_motion_shortcode = AMP_Theme_Support::video_override( '', $attr_daily_motion ); - $this->assertEquals( '', $daily_motion_shortcode ); - $no_attributes = AMP_Theme_Support::video_override( '', array() ); - $this->assertEquals( '', $no_attributes ); - } - /** * Test finish_output_buffering. * diff --git a/tests/test-class-amp-vimeo-embed-handler.php b/tests/test-class-amp-vimeo-embed-handler.php new file mode 100644 index 00000000000..15e14b1f37f --- /dev/null +++ b/tests/test-class-amp-vimeo-embed-handler.php @@ -0,0 +1,69 @@ +handler = new AMP_Vimeo_Embed_Handler(); + } + + /** + * Test video_override(). + * + * @covers AMP_Vimeo_Embed_Handler::video_override() + */ + public function test_video_override() { + remove_all_filters( 'wp_video_shortcode_override' ); + $this->handler->register_embed(); + $youtube_id = 'XOY3ZUO6P0k'; + $youtube_src = 'https://youtu.be/' . $youtube_id; + $attr_youtube = array( + 'src' => $youtube_src, + ); + + $youtube_shortcode = $this->handler->video_override( '', $attr_youtube ); + $this->assertEquals( '', $youtube_shortcode ); + + $vimeo_id = '64086087'; + $vimeo_src = 'https://vimeo.com/' . $vimeo_id; + $attr_vimeo = array( + 'src' => $vimeo_src, + ); + $yimeo_shortcode = $this->handler->video_override( '', $attr_vimeo ); + $this->assertContains( 'assertContains( $vimeo_id, $yimeo_shortcode ); + + $daily_motion_id = 'x6bacgf'; + $daily_motion_src = 'http://www.dailymotion.com/video/' . $daily_motion_id; + $attr_daily_motion = array( + 'src' => $daily_motion_src, + ); + $daily_motion_shortcode = $this->handler->video_override( '', $attr_daily_motion ); + $this->assertEquals( '', $daily_motion_shortcode ); + $no_attributes = $this->handler->video_override( '', array() ); + $this->assertEquals( '', $no_attributes ); + remove_all_filters( 'wp_video_shortcode_override' ); + } + +} diff --git a/tests/test-class-amp-youtube-embed-handler.php b/tests/test-class-amp-youtube-embed-handler.php new file mode 100644 index 00000000000..eb82838f15c --- /dev/null +++ b/tests/test-class-amp-youtube-embed-handler.php @@ -0,0 +1,69 @@ +handler = new AMP_YouTube_Embed_Handler(); + } + + /** + * Test video_override(). + * + * @covers AMP_YouTube_Embed_Handler::video_override() + */ + public function test_video_override() { + remove_all_filters( 'wp_video_shortcode_override' ); + $this->handler->register_embed(); + $youtube_id = 'XOY3ZUO6P0k'; + $youtube_src = 'https://youtu.be/' . $youtube_id; + $attr_youtube = array( + 'src' => $youtube_src, + ); + + $youtube_shortcode = $this->handler->video_override( '', $attr_youtube ); + $this->assertContains( 'assertContains( $youtube_id, $youtube_shortcode ); + + $vimeo_id = '64086087'; + $vimeo_src = 'https://vimeo.com/' . $vimeo_id; + $attr_vimeo = array( + 'src' => $vimeo_src, + ); + $yimeo_shortcode = $this->handler->video_override( '', $attr_vimeo ); + $this->assertEquals( '', $yimeo_shortcode ); + + $daily_motion_id = 'x6bacgf'; + $daily_motion_src = 'http://www.dailymotion.com/video/' . $daily_motion_id; + $attr_daily_motion = array( + 'src' => $daily_motion_src, + ); + $daily_motion_shortcode = $this->handler->video_override( '', $attr_daily_motion ); + $this->assertEquals( '', $daily_motion_shortcode ); + $no_attributes = $this->handler->video_override( '', array() ); + $this->assertEquals( '', $no_attributes ); + remove_all_filters( 'wp_video_shortcode_override' ); + } + +} From a3303424eee4271601717f3f13bb845827778ec8 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Fri, 2 Feb 2018 17:59:18 -0600 Subject: [PATCH 07/11] Issue #864: Handle a 'fixed-height' issue with 'Video' widgets. When they use 'Media Library' videos, they output . The AMP_Video_Sanitizer added a 'layout' value of 'fixed-height'. The height of 400px didn't look good, as the aspect ratio was wrong. Instead, make video widgets that use use layout=responsive. Detect this by checking that neight 'width' or 'height' are present. inject_video_max_width_style() removes these attributes for the widget. Otherwise, retain the previous logic. --- .../sanitizers/class-amp-video-sanitizer.php | 46 +++++++++++++++++-- tests/test-amp-video-sanitizer.php | 32 ++++++++----- 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index 804a2a6ceb8..e8849d4c5b1 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -23,6 +23,15 @@ class AMP_Video_Sanitizer extends AMP_Base_Sanitizer { */ const FALLBACK_HEIGHT = 400; + /** + * 16:9 ratio for videos. + * + * @since 0.7 + * + * @const float + */ + const ASPECT_RATIO = 1.7777; + /** * Tag. * @@ -49,9 +58,7 @@ public function sanitize() { $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); $new_attributes = $this->filter_attributes( $old_attributes ); - - $new_attributes = $this->enforce_fixed_height( $new_attributes ); - $new_attributes = $this->enforce_sizes_attribute( $new_attributes ); + $new_attributes = $this->size_attributes( $new_attributes ); $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes ); @@ -159,4 +166,37 @@ private function filter_attributes( $attributes ) { return $out; } + + /** + * Handles the size attributes of . + * + * Makes the responsive if it lacks 'height' and 'width' attributes. + * Video widgets don't have 'height' or 'width' attributes, + * because inject_video_max_width_style() removes them. + * So if there are no width or height attributes, + * Set them and make the layout responsive. + * The 'else' block handles the 'video' shortcode, not video widgets. + * wp_video_shortcode() always outputs a height and width, unless it's filtered. + * + * @since 0.7 + * + * @param array $attributes The element's attributes. + * @return array $attributes The attributes, possibly changed. + */ + public function size_attributes( $attributes ) { + if ( ! isset( $attributes['width'] ) && ! isset( $attributes['height'] ) ) { + $width = isset( $this->args['content_max_width'] ) ? $this->args['content_max_width'] : round( self::FALLBACK_HEIGHT * self::ASPECT_RATIO ); + $attributes = array_merge( $attributes, array( + 'width' => $width, + 'height' => round( $width / self::ASPECT_RATIO ), + 'layout' => 'responsive', + ) ); + } else { + $attributes = $this->enforce_fixed_height( $attributes ); + $attributes = $this->enforce_sizes_attribute( $attributes ); + } + + return $attributes; + } + } diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index 140b3584025..18004326ecf 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -8,27 +8,37 @@ public function get_data() { '

Lorem Ipsum Demet Delorit.

', ), - 'simple_video' => array( + 'simple_video' => array( '', '', ), - 'video_without_dimensions' => array( + 'video_without_dimensions_like_video_widget' => array( '', + '', + ), + + 'video_only_width' => array( + '', '', ), - 'autoplay_attribute' => array( + 'video_only_height' => array( + '', + '', + ), + + 'autoplay_attribute' => array( '', '', ), - 'autoplay_attribute__false' => array( + 'autoplay_attribute__false' => array( '', '', ), - 'video_with_whitelisted_attributes__enabled' => array( + 'video_with_whitelisted_attributes__enabled' => array( '', '', ), @@ -38,17 +48,17 @@ public function get_data() { '', ), - 'video_with_blacklisted_attribute' => array( + 'video_with_blacklisted_attribute' => array( '', '', ), - 'video_with_sizes_attribute_is_overridden' => array( + 'video_with_sizes_attribute_is_overridden' => array( '', '', ), - 'video_with_children' => array( + 'video_with_children' => array( '