From 2b006097ea7ae60c700a9afda5e26941a0967dea Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 24 Jun 2021 13:55:51 +0530 Subject: [PATCH 01/43] Transfer seek time to AMP youtube component from youtube url --- .../class-amp-youtube-embed-handler.php | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 074d272773b..0b2153bc701 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -132,6 +132,12 @@ private function parse_props( $html, $url, $video_id ) { $img ); + // Find start time of video. + $start_time = $this->get_start_time_from_url( $url ); + if ( ! empty( $start_time ) && 0 < intval( $start_time ) ) { + $props['start'] = intval( $start_time ); + } + return $props; } @@ -151,6 +157,7 @@ public function render( $args, $url ) { 'width' => $this->args['width'], 'height' => $this->args['height'], 'placeholder' => '', + 'start' => 0, ] ); @@ -175,6 +182,10 @@ public function render( $args, $url ) { $attributes['title'] = $args['title']; } + if ( ! empty( $args['start'] ) && 0 < intval( $args['start'] ) ) { + $attributes['data-param-start'] = intval( $args['start'] ); + } + return AMP_HTML_Utils::build_tag( 'amp-youtube', $attributes, $args['placeholder'] ); } @@ -236,6 +247,50 @@ private function get_video_id_from_url( $url ) { return false; } + /** + * To get start time of youtube video in second. + * + * @param string $url Youtube URL. + * + * @return int Start time in second. + */ + private function get_start_time_from_url( $url ) { + + if ( empty( $url ) ) { + return 0; + } + + $start_time = 0; + $parsed_url = wp_parse_url( $url ); + + if ( ! empty( $parsed_url['query'] ) ) { + wp_parse_str( $parsed_url['query'], $query_vars ); + + if ( ! empty( $query_vars['start'] ) && 0 < intval( $query_vars['start'] ) ) { + $start_time = intval( $query_vars['start'] ); + } + } + + if ( empty( $start_time ) && ! empty( $parsed_url['fragment'] ) ) { + $regex = '/^t=(?[0-9])+m(?[0-9]+)s$/iU'; + + preg_match( $regex, $parsed_url['fragment'], $matches ); + + if ( ! empty( $matches ) ) { + $matches = wp_parse_args( + $matches, + [ + 'minutes' => 0, + 'seconds' => 0, + ] + ); + $start_time = ( intval( $matches['seconds'] ) + ( intval( $matches['minutes'] ) * 60 ) ); + } + } + + return $start_time; + } + /** * Override the output of YouTube videos. * From d5f56f726d54df3b426fabaf31aad3ede186537b Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 24 Jun 2021 15:23:14 +0530 Subject: [PATCH 02/43] Add php unit test cases --- .../test-class-amp-youtube-embed-handler.php | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 189b2d4b86d..67225d03fd6 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -174,6 +174,12 @@ public function get_conversion_data() { '

Rebecca Black - Friday

' . PHP_EOL, '

' . PHP_EOL, ], + + 'with_start_time' => [ + 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m10s' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, + ], ]; } @@ -301,6 +307,52 @@ public function test_get_video_id_from_url( $url, $expected ) { ); } + /** + * Gets the test data for test_get_start_time_from_url(). + * + * @return array The test data. + */ + public function get_start_time_data() { + + return [ + 'empty_because_no_start_data' => [ + 'http://youtube.com/?wrong=XOY3ZUO6P0k', + 0, + ], + 'data_in_query_string' => [ + 'http://www.youtube.com/watch?v=0zM3nApSvMg&start=90', + 90, + ], + 'data_in_fragment' => [ + 'http://www.youtube.com/watch?v=0zM3nApSvMg#t=0m10s', + 10, + ], + 'data_in_fragment_with_minutes' => [ + 'http://www.youtube.com/watch?v=0zM3nApSvMg#t=1m10s', + 70, + ], + ]; + } + + /** + * Tests get_start_time_from_url. + * + * @dataProvider get_start_time_data + * @covers AMP_YouTube_Embed_Handler::get_start_time_from_url + * + * @param string $url The URL to test. + * @param string|false $expected The expected result. + * + * @throws ReflectionException If a reflection of the object is not possible. + */ + public function test_get_start_time_from_url( $url, $expected ) { + + $this->assertEquals( + $expected, + $this->call_private_method( $this->handler, 'get_start_time_from_url', [ $url ] ) + ); + } + public function get_scripts_data() { return [ 'not_converted' => [ From 080ded274e106a8c93070c7196d49cea9dbe9462 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Tue, 29 Jun 2021 16:28:50 +0530 Subject: [PATCH 03/43] Convert youtube iframe into amp-youtube --- .../class-amp-youtube-embed-handler.php | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 0b2153bc701..41b4e64b6a4 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -5,6 +5,8 @@ * @package AMP */ +use AmpProject\Dom\Document; + /** * Class AMP_YouTube_Embed_Handler * @@ -77,6 +79,93 @@ public function unregister_embed() { remove_filter( 'embed_oembed_html', [ $this, 'filter_embed_oembed_html' ], 10 ); } + /** + * Sanitize HTML that are not added via Gutenberg. + * + * @param Document $dom Document. + * + * @return void + */ + public function sanitize_raw_embeds( Document $dom ) { + + $nodes = $dom->xpath->query( '//iframe[ contains( @src, "youtu" ) ]' ); + + foreach ( $nodes as $node ) { + + $html = $dom->saveHTML( $node ); + $url = $node->getAttribute( 'src' ); + $amp_youtube_node = $this->process_embed( $dom, $html, $url ); + + if ( ! empty( $amp_youtube_node ) ) { + $node->parentNode->replaceChild( $amp_youtube_node, $node ); + } + } + + } + + /** + * To AMP youtube component from DOM Document. + * + * @param Document $dom Document DOM. + * @param string $html HTML markup of youtube iframe. + * @param string $url Youtube URL. + * + * @return DOMElement|false DOMElement on success, Otherwise false. + */ + public function process_embed( Document $dom, $html, $url ) { + + $id = $this->get_video_id_from_url( $url ); + + if ( ! $id ) { + return false; + } + + $args = $this->parse_props( $html, $url, $id ); + if ( empty( $args ) ) { + return false; + } + + $args['video_id'] = $id; + + $args = wp_parse_args( + $args, + [ + 'video_id' => false, + 'layout' => 'responsive', + 'width' => $this->args['width'], + 'height' => $this->args['height'], + 'placeholder' => '', + ] + ); + + if ( empty( $args['video_id'] ) ) { + return AMP_DOM_Utils::create_node( + $dom, + 'a', + [ + 'href' => esc_url_raw( $url ), + 'class' => 'amp-wp-embed-fallback', + ] + ); + } + + $this->did_convert_elements = true; + + $attributes = array_merge( + [ 'data-videoid' => $args['video_id'] ], + wp_array_slice_assoc( $args, [ 'layout', 'width', 'height' ] ) + ); + if ( ! empty( $args['title'] ) ) { + $attributes['title'] = $args['title']; + } + + return AMP_DOM_Utils::create_node( + $dom, + 'amp-youtube', + $attributes + ); + } + /** * Filter oEmbed HTML for YouTube to convert to AMP. * From a4e329067e001a83948b30218bed2ad868a266da Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Tue, 29 Jun 2021 18:14:32 +0530 Subject: [PATCH 04/43] Add additional data params for amp-youtube components --- .../class-amp-youtube-embed-handler.php | 109 ++++++++++++------ 1 file changed, 72 insertions(+), 37 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 41b4e64b6a4..3ca261dd832 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -127,18 +127,9 @@ public function process_embed( Document $dom, $html, $url ) { $args['video_id'] = $id; - $args = wp_parse_args( - $args, - [ - 'video_id' => false, - 'layout' => 'responsive', - 'width' => $this->args['width'], - 'height' => $this->args['height'], - 'placeholder' => '', - ] - ); + $attributes = $this->prepare_attributes( $args, $url ); - if ( empty( $args['video_id'] ) ) { + if ( empty( $attributes['data-videoid'] ) ) { return AMP_DOM_Utils::create_node( $dom, 'a', @@ -151,14 +142,6 @@ public function process_embed( Document $dom, $html, $url ) { $this->did_convert_elements = true; - $attributes = array_merge( - [ 'data-videoid' => $args['video_id'] ], - wp_array_slice_assoc( $args, [ 'layout', 'width', 'height' ] ) - ); - if ( ! empty( $args['title'] ) ) { - $attributes['title'] = $args['title']; - } - return AMP_DOM_Utils::create_node( $dom, 'amp-youtube', @@ -231,13 +214,14 @@ private function parse_props( $html, $url, $video_id ) { } /** - * Render embed. + * Prepare attributes for amp-youtube component. * - * @param array $args Args. - * @param string $url URL. - * @return string Rendered. + * @param array $args amp-youtube component arguments. + * @param string $url Youtube URL. + * + * @return array prepared arguments for amp-youtube component. */ - public function render( $args, $url ) { + public function prepare_attributes( $args, $url ) { $args = wp_parse_args( $args, [ @@ -250,19 +234,6 @@ public function render( $args, $url ) { ] ); - if ( empty( $args['video_id'] ) ) { - return AMP_HTML_Utils::build_tag( - 'a', - [ - 'href' => esc_url_raw( $url ), - 'class' => 'amp-wp-embed-fallback', - ], - esc_html( $url ) - ); - } - - $this->did_convert_elements = true; - $attributes = array_merge( [ 'data-videoid' => $args['video_id'] ], wp_array_slice_assoc( $args, [ 'layout', 'width', 'height' ] ) @@ -271,10 +242,74 @@ public function render( $args, $url ) { $attributes['title'] = $args['title']; } + $allowed_data_params = [ + 'cc_lang_pref', + 'cc_load_policy', + 'color', + 'controls', + 'disablekb', + 'enablejsapi', + 'end', + 'fs', + 'hl', + 'iv_load_policy', + 'list', + 'listType', + 'modestbranding', + 'origin', + 'playlist', + 'playsinline', + 'rel', + 'widget_referrer', + ]; + + $query_vars = []; + $query_param = wp_parse_url( $url, PHP_URL_QUERY ); + wp_parse_str( $query_param, $query_vars ); + + foreach ( $allowed_data_params as $allowed_data_param ) { + if ( isset( $query_vars[ $allowed_data_param ] ) ) { + $attributes[ "data-param-$allowed_data_param" ] = $query_vars[ $allowed_data_param ]; + } + } + + foreach ( [ 'autoplay', 'loop' ] as $param ) { + if ( isset( $query_vars[ $param ] ) ) { + $attributes[ $param ] = $query_vars[ $param ]; + } + } + if ( ! empty( $args['start'] ) && 0 < intval( $args['start'] ) ) { $attributes['data-param-start'] = intval( $args['start'] ); } + return $attributes; + } + + /** + * Render embed. + * + * @param array $args Args. + * @param string $url URL. + * @return string Rendered. + */ + public function render( $args, $url ) { + + $attributes = $this->prepare_attributes( $args, $url ); + + if ( empty( $attributes['data-videoid'] ) ) { + return AMP_HTML_Utils::build_tag( + 'a', + [ + 'href' => esc_url_raw( $url ), + 'class' => 'amp-wp-embed-fallback', + ], + esc_html( $url ) + ); + } + + $this->did_convert_elements = true; + return AMP_HTML_Utils::build_tag( 'amp-youtube', $attributes, $args['placeholder'] ); } From 2e47705e3641cf48e48e0abc97c2b732b7e4fa8b Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Tue, 29 Jun 2021 19:20:43 +0530 Subject: [PATCH 05/43] Add unit test case for youtube embeds --- .../test-class-amp-youtube-embed-handler.php | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 67225d03fd6..3606ee7dd0d 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -100,6 +100,50 @@ public function mock_http_request( $preempt, $r, $url ) { ]; } + /** + * data provider for $this->test_sanitize_raw_embeds() + * + * @return string[][] + */ + public function sanitize_raw_embeds_data_provider() { + + return [ + 'youtube-embed' => [ + 'source' => '', + 'expected' => '', + ], + 'short-url' => [ + 'source' => '', + 'expected' => '', + ], + 'none-youtube' => [ + 'source' => '', + 'expected' => '', + ], + ]; + } + + /** + * @dataProvider sanitize_raw_embeds_data_provider + * + * @covers AMP_YouTube_Embed_Handler::sanitize_raw_embeds + */ + public function test_sanitize_raw_embeds( $source, $expected ) { + + $embed = new AMP_YouTube_Embed_Handler(); + $embed->register_embed(); + + $dom = AMP_DOM_Utils::get_dom_from_content( $source ); + $embed->sanitize_raw_embeds( $dom ); + + $layout_sanitizer = new AMP_Layout_Sanitizer( $dom ); + $layout_sanitizer->sanitize(); + + $content = AMP_DOM_Utils::get_content_from_dom( $dom ); + + $this->assertEquals( $expected, trim( $content ) ); + } + /** * Test video_override(). * @@ -159,7 +203,7 @@ public function get_conversion_data() { 'url_with_querystring' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0&hl=en&fs=1&w=425&h=349' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, '

' . PHP_EOL, ], From 55fff1fa5c40381ce80a328c61366fb6059031a3 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Tue, 29 Jun 2021 19:40:43 +0530 Subject: [PATCH 06/43] Fix the issues from unit test cases --- includes/embeds/class-amp-youtube-embed-handler.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 3ca261dd832..c91024f13d1 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -310,6 +310,8 @@ public function render( $args, $url ) { $this->did_convert_elements = true; + $args['placeholder'] = ( ! empty( $args['placeholder'] ) ) ? $args['placeholder'] : ''; + return AMP_HTML_Utils::build_tag( 'amp-youtube', $attributes, $args['placeholder'] ); } From e5fba98f55b6dcd19b4815235461bb4b72af106b Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Tue, 29 Jun 2021 19:50:14 +0530 Subject: [PATCH 07/43] Fix the issues from unit test cases --- tests/php/test-class-amp-youtube-embed-handler.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 3606ee7dd0d..2953d7c74fa 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -204,7 +204,7 @@ public function get_conversion_data() { 'url_with_querystring' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0&hl=en&fs=1&w=425&h=349' . PHP_EOL, '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

' . PHP_EOL, ], // Several reports of invalid URLs that have multiple `?` in the URL. @@ -222,7 +222,7 @@ public function get_conversion_data() { 'with_start_time' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m10s' . PHP_EOL, '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

' . PHP_EOL, ], ]; } From 92ab4be1d21a21c0cdf4f7a1de586cec03c8a887 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 30 Jun 2021 12:43:28 +0530 Subject: [PATCH 08/43] Use type casting instead of intval() --- includes/embeds/class-amp-youtube-embed-handler.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index c91024f13d1..817a301d4c4 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -279,8 +279,8 @@ public function prepare_attributes( $args, $url ) { } } - if ( ! empty( $args['start'] ) && 0 < intval( $args['start'] ) ) { - $attributes['data-param-start'] = intval( $args['start'] ); + if ( ! empty( $args['start'] ) && 0 < (int) $args['start'] ) { + $attributes['data-param-start'] = (int) $args['start']; } return $attributes; @@ -392,8 +392,8 @@ private function get_start_time_from_url( $url ) { if ( ! empty( $parsed_url['query'] ) ) { wp_parse_str( $parsed_url['query'], $query_vars ); - if ( ! empty( $query_vars['start'] ) && 0 < intval( $query_vars['start'] ) ) { - $start_time = intval( $query_vars['start'] ); + if ( ! empty( $query_vars['start'] ) && 0 < (int) $query_vars['start'] ) { + $start_time = (int) $query_vars['start']; } } @@ -410,7 +410,7 @@ private function get_start_time_from_url( $url ) { 'seconds' => 0, ] ); - $start_time = ( intval( $matches['seconds'] ) + ( intval( $matches['minutes'] ) * 60 ) ); + $start_time = ( (int) $matches['seconds'] + ( (int) $matches['minutes'] * 60 ) ); } } From e9ad832402bf98e8ca911560a55c99ba75fefd90 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 30 Jun 2021 13:10:35 +0530 Subject: [PATCH 09/43] Add inline comments, remove unwanted code --- includes/embeds/class-amp-youtube-embed-handler.php | 4 ++-- tests/php/test-class-amp-youtube-embed-handler.php | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 817a301d4c4..74cdbd415fa 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -6,6 +6,7 @@ */ use AmpProject\Dom\Document; +use AmpProject\Dom\Element; /** * Class AMP_YouTube_Embed_Handler @@ -90,6 +91,7 @@ public function sanitize_raw_embeds( Document $dom ) { $nodes = $dom->xpath->query( '//iframe[ contains( @src, "youtu" ) ]' ); + /** @var Element $node */ foreach ( $nodes as $node ) { $html = $dom->saveHTML( $node ); @@ -140,8 +142,6 @@ public function process_embed( Document $dom, $html, $url ) { ); } - $this->did_convert_elements = true; - return AMP_DOM_Utils::create_node( $dom, 'amp-youtube', diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 2953d7c74fa..117babea125 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -112,13 +112,17 @@ public function sanitize_raw_embeds_data_provider() { 'source' => '', 'expected' => '', ], + 'with_http' => [ + 'source' => '', + 'expected' => '', + ], 'short-url' => [ 'source' => '', 'expected' => '', ], 'none-youtube' => [ - 'source' => '', - 'expected' => '', + 'source' => '', + 'expected' => '', ], ]; } From a5d73e0b3a240ca275715c052ca9c2747e163ed4 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 30 Jun 2021 14:22:13 +0530 Subject: [PATCH 10/43] Update data-param for amp-youtube --- .../class-amp-youtube-embed-handler.php | 31 +++++-------------- 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 74cdbd415fa..b59abe54071 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -242,35 +242,18 @@ public function prepare_attributes( $args, $url ) { $attributes['title'] = $args['title']; } - $allowed_data_params = [ - 'cc_lang_pref', - 'cc_load_policy', - 'color', - 'controls', - 'disablekb', - 'enablejsapi', - 'end', - 'fs', - 'hl', - 'iv_load_policy', - 'list', - 'listType', - 'modestbranding', - 'origin', - 'playlist', - 'playsinline', - 'rel', - 'widget_referrer', - ]; - $query_vars = []; $query_param = wp_parse_url( $url, PHP_URL_QUERY ); wp_parse_str( $query_param, $query_vars ); + $query_vars = ( ! empty( $query_vars ) && is_array( $query_vars ) ) ? $query_vars : []; - foreach ( $allowed_data_params as $allowed_data_param ) { - if ( isset( $query_vars[ $allowed_data_param ] ) ) { - $attributes[ "data-param-$allowed_data_param" ] = $query_vars[ $allowed_data_param ]; + foreach ( $query_vars as $key => $value ) { + + if ( in_array( $key, [ 'autoplay', 'loop', 'start' ], true ) ) { + continue; } + + $attributes[ "data-param-$key" ] = sanitize_text_field( $value ); } foreach ( [ 'autoplay', 'loop' ] as $param ) { From eceb4871952b6000c39bc42daaafd3bd0420964b Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 1 Jul 2021 12:57:42 +0530 Subject: [PATCH 11/43] Update XPath query to fetch youtube iframe --- .../embeds/class-amp-youtube-embed-handler.php | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index b59abe54071..ef941a6c63c 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -89,7 +89,22 @@ public function unregister_embed() { */ public function sanitize_raw_embeds( Document $dom ) { - $nodes = $dom->xpath->query( '//iframe[ contains( @src, "youtu" ) ]' ); + $applicable_domains = [ 'youtu.be', 'youtube.com', 'youtube-nocookie.com' ]; + + $query_segments = array_map( + static function ( $domain ) { + + return sprintf( + 'starts-with( @src, "https://www.%1$s/" ) or starts-with( @src, "https://%1$s/" ) or starts-with( @src, "http://www.%1$s/" ) or starts-with( @src, "http://%1$s/" )', + $domain + ); + }, + $applicable_domains + ); + + $query = implode( ' or ', $query_segments ); + + $nodes = $dom->xpath->query( sprintf( '//iframe[ %s ]', $query ) ); /** @var Element $node */ foreach ( $nodes as $node ) { From 8804f83b838cbe3c71eeeed0e4d811eb489317b2 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 1 Jul 2021 13:06:45 +0530 Subject: [PATCH 12/43] Fix php unit test cases --- includes/embeds/class-amp-youtube-embed-handler.php | 4 +++- tests/php/test-class-amp-youtube-embed-handler.php | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index ef941a6c63c..d011a5a40e4 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -262,9 +262,11 @@ public function prepare_attributes( $args, $url ) { wp_parse_str( $query_param, $query_vars ); $query_vars = ( ! empty( $query_vars ) && is_array( $query_vars ) ) ? $query_vars : []; + $excluded_param = [ 'autoplay', 'loop', 'start', 'v', 'vi', 'w', 'h' ]; + foreach ( $query_vars as $key => $value ) { - if ( in_array( $key, [ 'autoplay', 'loop', 'start' ], true ) ) { + if ( in_array( $key, $excluded_param, true ) ) { continue; } diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 117babea125..0cf08447c47 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -207,8 +207,8 @@ public function get_conversion_data() { 'url_with_querystring' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0&hl=en&fs=1&w=425&h=349' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], // Several reports of invalid URLs that have multiple `?` in the URL. From 220af8903f317ad11e21bf66ebc7f01478f74ad6 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 7 Jul 2021 17:08:09 +0530 Subject: [PATCH 13/43] Apply suggestions from PR --- .../class-amp-youtube-embed-handler.php | 71 +++++++++++-------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index d011a5a40e4..48214fe0c0e 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -49,6 +49,13 @@ class AMP_YouTube_Embed_Handler extends AMP_Base_Embed_Handler { */ protected $DEFAULT_HEIGHT = 338; + /** + * List of domains that are applicable for this embed. + * + * @var string[] + */ + protected $applicable_domains = [ 'youtu.be', 'youtube.com', 'youtube-nocookie.com' ]; + /** * AMP_YouTube_Embed_Handler constructor. * @@ -81,7 +88,7 @@ public function unregister_embed() { } /** - * Sanitize HTML that are not added via Gutenberg. + * Sanitize YouTube raw embeds. * * @param Document $dom Document. * @@ -89,8 +96,6 @@ public function unregister_embed() { */ public function sanitize_raw_embeds( Document $dom ) { - $applicable_domains = [ 'youtu.be', 'youtube.com', 'youtube-nocookie.com' ]; - $query_segments = array_map( static function ( $domain ) { @@ -99,7 +104,7 @@ static function ( $domain ) { $domain ); }, - $applicable_domains + $this->applicable_domains ); $query = implode( ' or ', $query_segments ); @@ -121,13 +126,13 @@ static function ( $domain ) { } /** - * To AMP youtube component from DOM Document. + * Parse embed attributes and return an AMP YouTube component. * * @param Document $dom Document DOM. - * @param string $html HTML markup of youtube iframe. - * @param string $url Youtube URL. + * @param string $html HTML markup of YouTube iframe. + * @param string $url YouTube URL. * - * @return DOMElement|false DOMElement on success, Otherwise false. + * @return DOMElement|false AMP component, otherwise `false`. */ public function process_embed( Document $dom, $html, $url ) { @@ -232,7 +237,7 @@ private function parse_props( $html, $url, $video_id ) { * Prepare attributes for amp-youtube component. * * @param array $args amp-youtube component arguments. - * @param string $url Youtube URL. + * @param string $url YouTube URL. * * @return array prepared arguments for amp-youtube component. */ @@ -249,10 +254,13 @@ public function prepare_attributes( $args, $url ) { ] ); - $attributes = array_merge( - [ 'data-videoid' => $args['video_id'] ], - wp_array_slice_assoc( $args, [ 'layout', 'width', 'height' ] ) - ); + $attributes = [ + 'data-videoid' => $args['video_id'], + 'layout' => $args['layout'], + 'width' => $args['width'], + 'height' => $args['height'], + ]; + if ( ! empty( $args['title'] ) ) { $attributes['title'] = $args['title']; } @@ -260,9 +268,9 @@ public function prepare_attributes( $args, $url ) { $query_vars = []; $query_param = wp_parse_url( $url, PHP_URL_QUERY ); wp_parse_str( $query_param, $query_vars ); - $query_vars = ( ! empty( $query_vars ) && is_array( $query_vars ) ) ? $query_vars : []; + $query_vars = ( is_array( $query_vars ) ) ? $query_vars : []; - $excluded_param = [ 'autoplay', 'loop', 'start', 'v', 'vi', 'w', 'h' ]; + $excluded_param = [ 'v', 'vi', 'w', 'h' ]; foreach ( $query_vars as $key => $value ) { @@ -270,17 +278,17 @@ public function prepare_attributes( $args, $url ) { continue; } - $attributes[ "data-param-$key" ] = sanitize_text_field( $value ); - } + if ( in_array( $key, [ 'autoplay', 'loop' ], true ) ) { + $attributes[ $key ] = $value; + continue; + } - foreach ( [ 'autoplay', 'loop' ] as $param ) { - if ( isset( $query_vars[ $param ] ) ) { - $attributes[ $param ] = $query_vars[ $param ]; + if ( 'start' === $key && 0 < (int) $args['start'] ) { + $attributes['data-param-start'] = (int) $args['start']; + continue; } - } - if ( ! empty( $args['start'] ) && 0 < (int) $args['start'] ) { - $attributes['data-param-start'] = (int) $args['start']; + $attributes[ "data-param-$key" ] = esc_attr( $value ); } return $attributes; @@ -329,7 +337,7 @@ private function get_video_id_from_url( $url ) { } $domain = implode( '.', array_slice( explode( '.', $parsed_url['host'] ), -2 ) ); - if ( ! in_array( $domain, [ 'youtu.be', 'youtube.com', 'youtube-nocookie.com' ], true ) ) { + if ( ! in_array( $domain, $this->applicable_domains, true ) ) { return false; } @@ -374,11 +382,11 @@ private function get_video_id_from_url( $url ) { } /** - * To get start time of youtube video in second. + * Get the start time of the YouTube video in seconds. * - * @param string $url Youtube URL. + * @param string $url YouTube URL. * - * @return int Start time in second. + * @return int Start time in seconds. */ private function get_start_time_from_url( $url ) { @@ -390,19 +398,20 @@ private function get_start_time_from_url( $url ) { $parsed_url = wp_parse_url( $url ); if ( ! empty( $parsed_url['query'] ) ) { + $query_vars = []; wp_parse_str( $parsed_url['query'], $query_vars ); if ( ! empty( $query_vars['start'] ) && 0 < (int) $query_vars['start'] ) { - $start_time = (int) $query_vars['start']; + return (int) $query_vars['start']; } } - if ( empty( $start_time ) && ! empty( $parsed_url['fragment'] ) ) { - $regex = '/^t=(?[0-9])+m(?[0-9]+)s$/iU'; + if ( ! empty( $parsed_url['fragment'] ) ) { + $regex = '/^t=(?\d+)m(?\d+)s$/'; preg_match( $regex, $parsed_url['fragment'], $matches ); - if ( ! empty( $matches ) ) { + if ( isset( $matches['minutes'], $matches['seconds'] ) ) { $matches = wp_parse_args( $matches, [ From cae32c03b0ab3873b1f63b88e5a4339a877210e4 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 7 Jul 2021 18:46:50 +0530 Subject: [PATCH 14/43] Update amp youtibe embed handler --- .../class-amp-youtube-embed-handler.php | 318 +++++++++++------- 1 file changed, 190 insertions(+), 128 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 48214fe0c0e..99cdaa1182f 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -7,6 +7,8 @@ use AmpProject\Dom\Document; use AmpProject\Dom\Element; +use AmpProject\Tag; +use AmpProject\Attribute; /** * Class AMP_YouTube_Embed_Handler @@ -87,10 +89,67 @@ public function unregister_embed() { remove_filter( 'embed_oembed_html', [ $this, 'filter_embed_oembed_html' ], 10 ); } + /** + * Filter oEmbed HTML for YouTube to convert to AMP. + * + * @param string $cache Cache for oEmbed. + * @param string $url Embed URL. + * + * @return string Embed. + */ + public function filter_embed_oembed_html( $cache, $url ) { + + if ( empty( $cache ) || empty( $url ) ) { + return ''; + } + + $video_id = $this->get_video_id_from_url( $url ); + + if ( ! $video_id ) { + return $cache; + } + + return $this->render( $cache, $url ); + } + + /** + * Convert YouTube iframe into AMP YouTube component. + * + * @param string $html HTML markup of YouTube iframe. + * @param string $url YouTube URL. + * + * @return string HTML markup of AMP YouTube component. + */ + public function render( $html, $url ) { + + $props = $this->match_element_attributes( $html, 'iframe', [ 'title' ] ); + $attributes = $this->prepare_attributes( $url ); + + if ( ! empty( $props['title'] ) ) { + $attributes['title'] = $props['title']; + } + + if ( empty( $attributes['data-videoid'] ) ) { + + return AMP_HTML_Utils::build_tag( + Tag::A, + [ + Attribute::HREF => esc_url_raw( $url ), + Attribute::CLASS_ => 'amp-wp-embed-fallback', + ], + esc_url( $url ) + ); + } + + $placeholder = $this->get_placeholder( $url, $attributes ); + + return AMP_HTML_Utils::build_tag( 'amp-youtube', $attributes, $placeholder ); + } + /** * Sanitize YouTube raw embeds. * - * @param Document $dom Document. + * @param Document $dom Document. * * @return void */ @@ -114,155 +173,106 @@ static function ( $domain ) { /** @var Element $node */ foreach ( $nodes as $node ) { - $html = $dom->saveHTML( $node ); - $url = $node->getAttribute( 'src' ); - $amp_youtube_node = $this->process_embed( $dom, $html, $url ); + $amp_youtube_component = $this->get_amp_component( $dom, $node ); - if ( ! empty( $amp_youtube_node ) ) { - $node->parentNode->replaceChild( $amp_youtube_node, $node ); + if ( ! empty( $amp_youtube_component ) ) { + $node->parentNode->replaceChild( $amp_youtube_component, $node ); } } } /** - * Parse embed attributes and return an AMP YouTube component. + * Parse YouTube iframe element and return an AMP YouTube component. * * @param Document $dom Document DOM. - * @param string $html HTML markup of YouTube iframe. - * @param string $url YouTube URL. + * @param Element $node DOMElement of YouTube iframe. * - * @return DOMElement|false AMP component, otherwise `false`. + * @return DOMElement|false DOMElement on success, Otherwise false. */ - public function process_embed( Document $dom, $html, $url ) { + private function get_amp_component( Document $dom, Element $node ) { - $id = $this->get_video_id_from_url( $url ); + $url = $node->getAttribute( 'src' ); + $video_id = $this->get_video_id_from_url( $url ); - if ( ! $id ) { + if ( ! $video_id ) { return false; } - $args = $this->parse_props( $html, $url, $id ); - if ( empty( $args ) ) { - return false; + $url = $node->getAttribute( 'src' ); + $attributes = $this->prepare_attributes( $url ); + + if ( ! empty( $node->getAttribute( 'title' ) ) ) { + $attributes['title'] = $node->getAttribute( 'title' ); } - $args['video_id'] = $id; + if ( ! empty( $node->getAttribute( 'width' ) ) ) { + $attributes['width'] = $node->getAttribute( 'width' ); + } - $attributes = $this->prepare_attributes( $args, $url ); + if ( ! empty( $node->getAttribute( 'height' ) ) ) { + $attributes['height'] = $node->getAttribute( 'height' ); + } if ( empty( $attributes['data-videoid'] ) ) { - return AMP_DOM_Utils::create_node( + $link_node = AMP_DOM_Utils::create_node( $dom, - 'a', + Tag::A, [ - 'href' => esc_url_raw( $url ), - 'class' => 'amp-wp-embed-fallback', + Attribute::HREF => esc_url_raw( $url ), + Attribute::CLASS_ => 'amp-wp-embed-fallback', ] ); + $link_node->appendChild( $dom->createTextNode( $url ) ); + + return $link_node; } - return AMP_DOM_Utils::create_node( + $video_title = ( ! empty( $attributes['title'] ) ) ? $attributes['title'] : null; + + $amp_node = AMP_DOM_Utils::create_node( $dom, 'amp-youtube', $attributes ); - } - - /** - * Filter oEmbed HTML for YouTube to convert to AMP. - * - * @param string $cache Cache for oEmbed. - * @param string $url Embed URL. - * @return string Embed. - */ - public function filter_embed_oembed_html( $cache, $url ) { - $id = $this->get_video_id_from_url( $url ); - if ( ! $id ) { - return $cache; - } - - $props = $this->parse_props( $cache, $url, $id ); - if ( empty( $props ) ) { - return $cache; - } - - $props['video_id'] = $id; - return $this->render( $props, $url ); - } - - /** - * Parse AMP component from iframe. - * - * @param string $html HTML. - * @param string $url Embed URL, for fallback purposes. - * @param string $video_id YouTube video ID. - * @return array|null Props for rendering the component, or null if unable to parse. - */ - private function parse_props( $html, $url, $video_id ) { - $props = $this->match_element_attributes( $html, 'iframe', [ 'title', 'height', 'width' ] ); - if ( ! isset( $props ) ) { - return null; - } - $img_attributes = [ - 'src' => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $video_id ) ), - 'layout' => 'fill', - 'object-fit' => 'cover', - ]; - if ( ! empty( $props['title'] ) ) { - $img_attributes['alt'] = $props['title']; - } - $img = AMP_HTML_Utils::build_tag( 'img', $img_attributes ); + if ( ! empty( $amp_node ) && is_a( $amp_node, 'DOMElement' ) ) { + $placeholder = $this->get_placeholder_component( $amp_node, $attributes ); - $props['placeholder'] = AMP_HTML_Utils::build_tag( - 'a', - [ - 'placeholder' => '', - 'href' => esc_url_raw( $url ), - ], - $img - ); - - // Find start time of video. - $start_time = $this->get_start_time_from_url( $url ); - if ( ! empty( $start_time ) && 0 < intval( $start_time ) ) { - $props['start'] = intval( $start_time ); + if ( $placeholder ) { + $amp_node->appendChild( $placeholder ); + } } - return $props; + return $amp_node; } /** * Prepare attributes for amp-youtube component. * - * @param array $args amp-youtube component arguments. - * @param string $url YouTube URL. + * @param string $url YouTube video URL. * * @return array prepared arguments for amp-youtube component. */ - public function prepare_attributes( $args, $url ) { - $args = wp_parse_args( - $args, - [ - 'video_id' => false, - 'layout' => 'responsive', - 'width' => $this->args['width'], - 'height' => $this->args['height'], - 'placeholder' => '', - 'start' => 0, - ] - ); + private function prepare_attributes( $url ) { + + $video_id = $this->get_video_id_from_url( $url ); + + if ( ! $video_id ) { + return []; + } $attributes = [ - 'data-videoid' => $args['video_id'], - 'layout' => $args['layout'], - 'width' => $args['width'], - 'height' => $args['height'], + 'data-videoid' => $video_id, + 'layout' => 'responsive', + 'width' => $this->args['width'], + 'height' => $this->args['height'], ]; - if ( ! empty( $args['title'] ) ) { - $attributes['title'] = $args['title']; + // Find start time of video. + $start_time = $this->get_start_time_from_url( $url ); + if ( ! empty( $start_time ) && 0 < (int) $start_time ) { + $attributes['data-param-start'] = (int) $start_time; } $query_vars = []; @@ -270,7 +280,7 @@ public function prepare_attributes( $args, $url ) { wp_parse_str( $query_param, $query_vars ); $query_vars = ( is_array( $query_vars ) ) ? $query_vars : []; - $excluded_param = [ 'v', 'vi', 'w', 'h' ]; + $excluded_param = [ 'start', 'v', 'vi', 'w', 'h' ]; foreach ( $query_vars as $key => $value ) { @@ -283,11 +293,6 @@ public function prepare_attributes( $args, $url ) { continue; } - if ( 'start' === $key && 0 < (int) $args['start'] ) { - $attributes['data-param-start'] = (int) $args['start']; - continue; - } - $attributes[ "data-param-$key" ] = esc_attr( $value ); } @@ -295,48 +300,103 @@ public function prepare_attributes( $args, $url ) { } /** - * Render embed. + * Placeholder component for amp YouTube component.. + * + * @param DOMElement $amp_component DOM Element of AMP component. + * @param array $attributes YouTube Attributes. * - * @param array $args Args. - * @param string $url URL. - * @return string Rendered. + * @return DOMElement|false DOMElement on success otherwise false. */ - public function render( $args, $url ) { + private function get_placeholder_component( DOMElement $amp_component, $attributes ) { + + if ( empty( $attributes['data-videoid'] ) ) { + return false; + } + + $video_id = $attributes['data-videoid']; + + $img_attributes = [ + Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $video_id ) ), + Attribute::LAYOUT => 'fill', + Attribute::OBJECT_FIT => 'cover', + ]; - $attributes = $this->prepare_attributes( $args, $url ); + if ( $attributes['title'] ) { + $img_attributes[ Attribute::ALT ] = $attributes['title']; + } + + $img_node = AMP_DOM_Utils::create_node( + Document::fromNode( $amp_component->ownerDocument ), + Tag::IMG, + $img_attributes + ); + + $placeholder = AMP_DOM_Utils::create_node( + Document::fromNode( $amp_component->ownerDocument ), + Tag::A, + [ + Attribute::PLACEHOLDER => '', + Attribute::HREF => esc_url_raw( sprintf( 'https://www.youtube.com/watch?v=%s', $video_id ) ), + ] + ); + + $placeholder->appendChild( $img_node ); + + return $placeholder; + } + + /** + * To get placeholder for amp component. + * + * @param string $url YouTube URL. + * @param array $attributes YouTube Attributes. + * + * @return string + */ + private function get_placeholder( $url, $attributes ) { if ( empty( $attributes['data-videoid'] ) ) { - return AMP_HTML_Utils::build_tag( - 'a', - [ - 'href' => esc_url_raw( $url ), - 'class' => 'amp-wp-embed-fallback', - ], - esc_html( $url ) - ); + return ''; } - $this->did_convert_elements = true; + $img_attributes = [ + Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $attributes['data-videoid'] ) ), + Attribute::LAYOUT => 'fill', + Attribute::OBJECT_FIT => 'cover', + ]; + + if ( ! empty( $attributes['title'] ) ) { + $img_attributes[ Attribute::ALT ] = $attributes['title']; + } - $args['placeholder'] = ( ! empty( $args['placeholder'] ) ) ? $args['placeholder'] : ''; + $img = AMP_HTML_Utils::build_tag( 'img', $img_attributes ); - return AMP_HTML_Utils::build_tag( 'amp-youtube', $attributes, $args['placeholder'] ); + return AMP_HTML_Utils::build_tag( + Tag::A, + [ + Attribute::PLACEHOLDER => '', + Attribute::HREF => esc_url_raw( $url ), + ], + $img + ); } /** * Determine the video ID from the URL. * * @param string $url URL. + * * @return string|false Video ID, or false if none could be retrieved. */ private function get_video_id_from_url( $url ) { + $parsed_url = wp_parse_url( $url ); if ( ! isset( $parsed_url['host'] ) ) { return false; } - $domain = implode( '.', array_slice( explode( '.', $parsed_url['host'] ), -2 ) ); + $domain = implode( '.', array_slice( explode( '.', $parsed_url['host'] ), - 2 ) ); if ( ! in_array( $domain, $this->applicable_domains, true ) ) { return false; } @@ -434,9 +494,11 @@ private function get_start_time_from_url( $url ) { * * @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; } @@ -445,6 +507,6 @@ public function video_override( $html, $attr ) { return $html; } - return $this->render( compact( 'video_id' ), $attr['src'] ); + return $this->render( $html, $attr['src'] ); } } From 12a722c0173be2b2fe2c4c3c75a4a621e82bc6bf Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 7 Jul 2021 19:02:19 +0530 Subject: [PATCH 15/43] update unit test cases --- tests/php/test-class-amp-youtube-embed-handler.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 0cf08447c47..c6e09a54864 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -110,15 +110,15 @@ public function sanitize_raw_embeds_data_provider() { return [ 'youtube-embed' => [ 'source' => '', - 'expected' => '', + 'expected' => 'YouTube video player', ], 'with_http' => [ 'source' => '', - 'expected' => '', + 'expected' => 'YouTube video player', ], 'short-url' => [ 'source' => '', - 'expected' => '', + 'expected' => 'YouTube video player', ], 'none-youtube' => [ 'source' => '', From 520be01b6bac13291d96b77437bcc46681e035d5 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 7 Jul 2021 19:22:41 +0530 Subject: [PATCH 16/43] Update and fix unit test cases --- includes/embeds/class-amp-youtube-embed-handler.php | 9 ++++++--- tests/php/test-class-amp-youtube-embed-handler.php | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 99cdaa1182f..77b179c3cd0 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -122,11 +122,14 @@ public function filter_embed_oembed_html( $cache, $url ) { */ public function render( $html, $url ) { - $props = $this->match_element_attributes( $html, 'iframe', [ 'title' ] ); $attributes = $this->prepare_attributes( $url ); - if ( ! empty( $props['title'] ) ) { - $attributes['title'] = $props['title']; + $iframe_props = [ 'title', 'height', 'width' ]; + $props = $this->match_element_attributes( $html, 'iframe', $iframe_props ); + foreach ( $iframe_props as $iframe_prop ) { + if ( ! empty( $props[ $iframe_prop ] ) ) { + $attributes[ $iframe_prop ] = $props[ $iframe_prop ]; + } } if ( empty( $attributes['data-videoid'] ) ) { diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index c6e09a54864..65d2b91c6c9 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -207,7 +207,7 @@ public function get_conversion_data() { 'url_with_querystring' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0&hl=en&fs=1&w=425&h=349' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, '

' . PHP_EOL, ], @@ -225,7 +225,7 @@ public function get_conversion_data() { 'with_start_time' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m10s' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, '

' . PHP_EOL, ], ]; From 8e484f02d2b0baa5b9621d79f08895792c89339d Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Wed, 7 Jul 2021 21:07:52 +0530 Subject: [PATCH 17/43] Remove redundant code --- .../class-amp-youtube-embed-handler.php | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 77b179c3cd0..8910f6962c9 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -202,19 +202,14 @@ private function get_amp_component( Document $dom, Element $node ) { return false; } - $url = $node->getAttribute( 'src' ); $attributes = $this->prepare_attributes( $url ); - if ( ! empty( $node->getAttribute( 'title' ) ) ) { - $attributes['title'] = $node->getAttribute( 'title' ); - } - - if ( ! empty( $node->getAttribute( 'width' ) ) ) { - $attributes['width'] = $node->getAttribute( 'width' ); - } + $iframe_props = [ 'title', 'height', 'width' ]; - if ( ! empty( $node->getAttribute( 'height' ) ) ) { - $attributes['height'] = $node->getAttribute( 'height' ); + foreach ( $iframe_props as $iframe_prop ) { + if ( ! empty( $node->getAttribute( $iframe_prop ) ) ) { + $attributes[ $iframe_prop ] = $node->getAttribute( $iframe_prop ); + } } if ( empty( $attributes['data-videoid'] ) ) { @@ -231,8 +226,6 @@ private function get_amp_component( Document $dom, Element $node ) { return $link_node; } - $video_title = ( ! empty( $attributes['title'] ) ) ? $attributes['title'] : null; - $amp_node = AMP_DOM_Utils::create_node( $dom, 'amp-youtube', @@ -296,6 +289,11 @@ private function prepare_attributes( $url ) { continue; } + if ( 'channel' === $key ) { + $attributes['data-live-channelid'] = $value; + continue; + } + $attributes[ "data-param-$key" ] = esc_attr( $value ); } From 6808668227e5cec074ede4147d7fe4485e958daa Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 8 Jul 2021 11:33:33 +0530 Subject: [PATCH 18/43] Add support of live channel when added as raw iframe --- .../class-amp-youtube-embed-handler.php | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 8910f6962c9..8aea21d085c 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -195,13 +195,7 @@ static function ( $domain ) { */ private function get_amp_component( Document $dom, Element $node ) { - $url = $node->getAttribute( 'src' ); - $video_id = $this->get_video_id_from_url( $url ); - - if ( ! $video_id ) { - return false; - } - + $url = $node->getAttribute( 'src' ); $attributes = $this->prepare_attributes( $url ); $iframe_props = [ 'title', 'height', 'width' ]; @@ -212,7 +206,7 @@ private function get_amp_component( Document $dom, Element $node ) { } } - if ( empty( $attributes['data-videoid'] ) ) { + if ( empty( $attributes['data-videoid'] ) && empty( $attributes['data-live-channelid'] ) ) { $link_node = AMP_DOM_Utils::create_node( $dom, Tag::A, @@ -252,19 +246,22 @@ private function get_amp_component( Document $dom, Element $node ) { */ private function prepare_attributes( $url ) { - $video_id = $this->get_video_id_from_url( $url ); - - if ( ! $video_id ) { + if ( empty( $url ) ) { return []; } $attributes = [ - 'data-videoid' => $video_id, - 'layout' => 'responsive', - 'width' => $this->args['width'], - 'height' => $this->args['height'], + 'layout' => 'responsive', + 'width' => $this->args['width'], + 'height' => $this->args['height'], ]; + $video_id = $this->get_video_id_from_url( $url ); + + if ( ! empty( $video_id ) ) { + $attributes['data-videoid'] = $video_id; + } + // Find start time of video. $start_time = $this->get_start_time_from_url( $url ); if ( ! empty( $start_time ) && 0 < (int) $start_time ) { @@ -436,6 +433,15 @@ private function get_video_id_from_url( $url ) { // Other top-level segments indicate non-video URLs. There are examples of URLs having segments including // 'v', 'vi', and 'e' but these do not work anymore. In any case, they are added here for completeness. if ( ! empty( $segments[1] ) && in_array( $segments[0], [ 'embed', 'watch', 'v', 'vi', 'e' ], true ) ) { + + /** + * Ignore live streaming channel URLs. For example: + * * https://www.youtube.com/embed/live_stream?channel=UCkaNo2FUEWips2z4BkOHl6Q + */ + if ( 'embed' === $segments[0] && 'live_stream' === $segments[1] && isset( $query_vars['channel'] ) ) { + return false; + } + return $segments[1]; } From f60fd46dc464b07a40d35b2040e855a4e7e0c1d1 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 8 Jul 2021 11:34:13 +0530 Subject: [PATCH 19/43] Fix unit test cases --- .../test-class-amp-youtube-embed-handler.php | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 65d2b91c6c9..913a5fa895d 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -110,15 +110,15 @@ public function sanitize_raw_embeds_data_provider() { return [ 'youtube-embed' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'with_http' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'short-url' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'none-youtube' => [ 'source' => '', @@ -195,20 +195,20 @@ public function get_conversion_data() { 'url_simple' => [ 'https://www.youtube.com/watch?v=kfVsfOSbJY0' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'url_short' => [ 'https://youtu.be/kfVsfOSbJY0' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'url_with_querystring' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0&hl=en&fs=1&w=425&h=349' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], // Several reports of invalid URLs that have multiple `?` in the URL. @@ -219,14 +219,14 @@ public function get_conversion_data() { 'embed_url' => [ 'https://www.youtube.com/embed/kfVsfOSbJY0' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'with_start_time' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m10s' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], ]; } From c35ecd976df4fc3efb59fd5f8429e41dd9e15548 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 8 Jul 2021 11:42:21 +0530 Subject: [PATCH 20/43] Add unit test cases for live streaming channel --- .../php/test-class-amp-youtube-embed-handler.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 913a5fa895d..50a8d3047fd 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -108,19 +108,23 @@ public function mock_http_request( $preempt, $r, $url ) { public function sanitize_raw_embeds_data_provider() { return [ - 'youtube-embed' => [ + 'youtube-embed' => [ 'source' => '', 'expected' => 'YouTube video player', ], - 'with_http' => [ + 'with_http' => [ 'source' => '', 'expected' => 'YouTube video player', ], - 'short-url' => [ + 'short-url' => [ 'source' => '', 'expected' => 'YouTube video player', ], - 'none-youtube' => [ + 'live_streaming_channel' => [ + 'source' => '', + 'expected' => '', + ], + 'none-youtube' => [ 'source' => '', 'expected' => '', ], @@ -335,6 +339,10 @@ public function get_video_id_data() { 'http://youtube.com/?wrong=XOY3ZUO6P0k', false, ], + 'live_streaming_channel' => [ + 'https://www.youtube.com/embed/live_stream?channel=UCkaNo2FUEWips2z4BkOHl6Q', + false, + ], ]; } From 547f237d277560435ba7e5cf90e78d23fd2aec94 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Fri, 9 Jul 2021 13:32:24 +0530 Subject: [PATCH 21/43] Update prepare_attribute() --- .../class-amp-youtube-embed-handler.php | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 8aea21d085c..d7db8711476 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -100,7 +100,7 @@ public function unregister_embed() { public function filter_embed_oembed_html( $cache, $url ) { if ( empty( $cache ) || empty( $url ) ) { - return ''; + return $cache; } $video_id = $this->get_video_id_from_url( $url ); @@ -109,20 +109,21 @@ public function filter_embed_oembed_html( $cache, $url ) { return $cache; } - return $this->render( $cache, $url ); + return $this->render( $cache, $url, $video_id ); } /** * Convert YouTube iframe into AMP YouTube component. * - * @param string $html HTML markup of YouTube iframe. - * @param string $url YouTube URL. + * @param string $html HTML markup of YouTube iframe. + * @param string $url YouTube URL. + * @param string $video_id YouTube video ID. * * @return string HTML markup of AMP YouTube component. */ - public function render( $html, $url ) { + public function render( $html, $url, $video_id ) { - $attributes = $this->prepare_attributes( $url ); + $attributes = $this->prepare_attributes( $url, $video_id ); $iframe_props = [ 'title', 'height', 'width' ]; $props = $this->match_element_attributes( $html, 'iframe', $iframe_props ); @@ -191,12 +192,13 @@ static function ( $domain ) { * @param Document $dom Document DOM. * @param Element $node DOMElement of YouTube iframe. * - * @return DOMElement|false DOMElement on success, Otherwise false. + * @return DOMElement|false AMP component, otherwise `false`. */ private function get_amp_component( Document $dom, Element $node ) { $url = $node->getAttribute( 'src' ); - $attributes = $this->prepare_attributes( $url ); + $video_id = $this->get_video_id_from_url( $url ); + $attributes = $this->prepare_attributes( $url, $video_id ); $iframe_props = [ 'title', 'height', 'width' ]; @@ -226,7 +228,7 @@ private function get_amp_component( Document $dom, Element $node ) { $attributes ); - if ( ! empty( $amp_node ) && is_a( $amp_node, 'DOMElement' ) ) { + if ( ! empty( $amp_node ) && is_a( $amp_node, '\DOMElement' ) ) { $placeholder = $this->get_placeholder_component( $amp_node, $attributes ); if ( $placeholder ) { @@ -240,11 +242,12 @@ private function get_amp_component( Document $dom, Element $node ) { /** * Prepare attributes for amp-youtube component. * - * @param string $url YouTube video URL. + * @param string $url YouTube video URL. + * @param string $video_id YouTube video ID. * * @return array prepared arguments for amp-youtube component. */ - private function prepare_attributes( $url ) { + private function prepare_attributes( $url, $video_id = '' ) { if ( empty( $url ) ) { return []; @@ -256,8 +259,6 @@ private function prepare_attributes( $url ) { 'height' => $this->args['height'], ]; - $video_id = $this->get_video_id_from_url( $url ); - if ( ! empty( $video_id ) ) { $attributes['data-videoid'] = $video_id; } @@ -298,7 +299,7 @@ private function prepare_attributes( $url ) { } /** - * Placeholder component for amp YouTube component.. + * Placeholder component for AMP YouTube component. * * @param DOMElement $amp_component DOM Element of AMP component. * @param array $attributes YouTube Attributes. @@ -509,11 +510,13 @@ public function video_override( $html, $attr ) { if ( ! isset( $attr['src'] ) ) { return $html; } + $video_id = $this->get_video_id_from_url( $attr['src'] ); + if ( ! $video_id ) { return $html; } - return $this->render( $html, $attr['src'] ); + return $this->render( $html, $attr['src'], $video_id ); } } From 32e9b2d6c2f4b1b026c77280fba12be9e0951a0b Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Fri, 9 Jul 2021 16:24:15 +0530 Subject: [PATCH 22/43] Update regex for to find seek time from URL --- includes/embeds/class-amp-youtube-embed-handler.php | 2 +- tests/php/test-class-amp-youtube-embed-handler.php | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index d7db8711476..89966abc950 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -475,7 +475,7 @@ private function get_start_time_from_url( $url ) { } if ( ! empty( $parsed_url['fragment'] ) ) { - $regex = '/^t=(?\d+)m(?\d+)s$/'; + $regex = '/^t=(?:(?\d+)m)?(?:(?\d+)s?)?$/'; preg_match( $regex, $parsed_url['fragment'], $matches ); diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 50a8d3047fd..a3cebfb6f1f 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -232,6 +232,12 @@ public function get_conversion_data() { '

Rebecca Black - Friday

' . PHP_EOL, '

' . PHP_EOL, ], + + 'with_start_time_2' => [ + 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=62' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, + ], ]; } From 2d1785a7a7e8a39236460f689d7feeb913c5a031 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Fri, 9 Jul 2021 16:31:23 +0530 Subject: [PATCH 23/43] Add additional unit test cases for video seek time --- .../embeds/class-amp-youtube-embed-handler.php | 2 +- .../test-class-amp-youtube-embed-handler.php | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 89966abc950..7ed6df7a3bf 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -479,7 +479,7 @@ private function get_start_time_from_url( $url ) { preg_match( $regex, $parsed_url['fragment'], $matches ); - if ( isset( $matches['minutes'], $matches['seconds'] ) ) { + if ( is_array( $matches ) ) { $matches = wp_parse_args( $matches, [ diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index a3cebfb6f1f..41201c03291 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -238,6 +238,24 @@ public function get_conversion_data() { '

Rebecca Black - Friday

' . PHP_EOL, '

' . PHP_EOL, ], + + 'with_start_time_3' => [ + 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=62s' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, + ], + + 'with_start_time_4' => [ + 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, + ], + + 'with_start_time_5' => [ + 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m2' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, + ], ]; } From 3ecad1e3fa9b44e2c7a0b66918099c96f60126d6 Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 9 Sep 2021 14:53:19 +0530 Subject: [PATCH 24/43] Sanitize attribute name Use original YouTube URL as placeholder when sanitizing raw embed Fix phpcs alignment warning Use constant instead of member variable Let Document::fromNode() obtain the ownerDocument Deduplicate iframe_props into constant --- .../class-amp-twitter-embed-handler.php | 1 + .../class-amp-youtube-embed-handler.php | 117 ++++++++++-------- includes/utils/class-amp-dom-utils.php | 2 +- .../class-amp-validation-error-taxonomy.php | 2 +- .../MonitorCssTransientCaching.php | 1 - 5 files changed, 67 insertions(+), 56 deletions(-) diff --git a/includes/embeds/class-amp-twitter-embed-handler.php b/includes/embeds/class-amp-twitter-embed-handler.php index 9dd030b5164..c33c71e7915 100644 --- a/includes/embeds/class-amp-twitter-embed-handler.php +++ b/includes/embeds/class-amp-twitter-embed-handler.php @@ -19,6 +19,7 @@ class AMP_Twitter_Embed_Handler extends AMP_Base_Embed_Handler { /** * Default width. * + * @phpstan-ignore-next-line * @var int|string */ protected $DEFAULT_WIDTH = 'auto'; diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 7ed6df7a3bf..dccdb4fd3c4 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -5,10 +5,12 @@ * @package AMP */ +use AmpProject\Attribute; use AmpProject\Dom\Document; use AmpProject\Dom\Element; +use AmpProject\Extension; +use AmpProject\Layout; use AmpProject\Tag; -use AmpProject\Attribute; /** * Class AMP_YouTube_Embed_Handler @@ -33,7 +35,7 @@ class AMP_YouTube_Embed_Handler extends AMP_Base_Embed_Handler { /** * Ratio for calculating the default height from the content width. * - * @param float + * @var float */ const RATIO = 0.5625; @@ -56,7 +58,18 @@ class AMP_YouTube_Embed_Handler extends AMP_Base_Embed_Handler { * * @var string[] */ - protected $applicable_domains = [ 'youtu.be', 'youtube.com', 'youtube-nocookie.com' ]; + const APPLICABLE_DOMAINS = [ 'youtu.be', 'youtube.com', 'youtube-nocookie.com' ]; + + /** + * Attributes from iframe which are copied to amp-youtube. + * + * @var string[] + */ + const IFRAME_ATTRIBUTES = [ + Attribute::TITLE, + Attribute::HEIGHT, + Attribute::WIDTH, + ]; /** * AMP_YouTube_Embed_Handler constructor. @@ -125,15 +138,14 @@ public function render( $html, $url, $video_id ) { $attributes = $this->prepare_attributes( $url, $video_id ); - $iframe_props = [ 'title', 'height', 'width' ]; - $props = $this->match_element_attributes( $html, 'iframe', $iframe_props ); - foreach ( $iframe_props as $iframe_prop ) { + $props = $this->match_element_attributes( $html, Tag::IFRAME, self::IFRAME_ATTRIBUTES ); + foreach ( self::IFRAME_ATTRIBUTES as $iframe_prop ) { if ( ! empty( $props[ $iframe_prop ] ) ) { $attributes[ $iframe_prop ] = $props[ $iframe_prop ]; } } - if ( empty( $attributes['data-videoid'] ) ) { + if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { return AMP_HTML_Utils::build_tag( Tag::A, @@ -147,7 +159,7 @@ public function render( $html, $url, $video_id ) { $placeholder = $this->get_placeholder( $url, $attributes ); - return AMP_HTML_Utils::build_tag( 'amp-youtube', $attributes, $placeholder ); + return AMP_HTML_Utils::build_tag( Extension::YOUTUBE, $attributes, $placeholder ); } /** @@ -161,13 +173,12 @@ public function sanitize_raw_embeds( Document $dom ) { $query_segments = array_map( static function ( $domain ) { - return sprintf( 'starts-with( @src, "https://www.%1$s/" ) or starts-with( @src, "https://%1$s/" ) or starts-with( @src, "http://www.%1$s/" ) or starts-with( @src, "http://%1$s/" )', $domain ); }, - $this->applicable_domains + self::APPLICABLE_DOMAINS ); $query = implode( ' or ', $query_segments ); @@ -190,25 +201,23 @@ static function ( $domain ) { * Parse YouTube iframe element and return an AMP YouTube component. * * @param Document $dom Document DOM. - * @param Element $node DOMElement of YouTube iframe. + * @param Element $node YouTube iframe element. * - * @return DOMElement|false AMP component, otherwise `false`. + * @return Element|false AMP component, otherwise `false`. */ private function get_amp_component( Document $dom, Element $node ) { - $url = $node->getAttribute( 'src' ); + $url = $node->getAttribute( Attribute::SRC ); $video_id = $this->get_video_id_from_url( $url ); $attributes = $this->prepare_attributes( $url, $video_id ); - $iframe_props = [ 'title', 'height', 'width' ]; - - foreach ( $iframe_props as $iframe_prop ) { + foreach ( self::IFRAME_ATTRIBUTES as $iframe_prop ) { if ( ! empty( $node->getAttribute( $iframe_prop ) ) ) { $attributes[ $iframe_prop ] = $node->getAttribute( $iframe_prop ); } } - if ( empty( $attributes['data-videoid'] ) && empty( $attributes['data-live-channelid'] ) ) { + if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) && empty( $attributes[ Attribute::DATA_LIVE_CHANNELID ] ) ) { $link_node = AMP_DOM_Utils::create_node( $dom, Tag::A, @@ -224,12 +233,12 @@ private function get_amp_component( Document $dom, Element $node ) { $amp_node = AMP_DOM_Utils::create_node( $dom, - 'amp-youtube', + Extension::YOUTUBE, $attributes ); - if ( ! empty( $amp_node ) && is_a( $amp_node, '\DOMElement' ) ) { - $placeholder = $this->get_placeholder_component( $amp_node, $attributes ); + if ( ! empty( $amp_node ) && $amp_node instanceof Element ) { + $placeholder = $this->get_placeholder_component( $amp_node, $attributes, $url ); if ( $placeholder ) { $amp_node->appendChild( $placeholder ); @@ -254,13 +263,13 @@ private function prepare_attributes( $url, $video_id = '' ) { } $attributes = [ - 'layout' => 'responsive', - 'width' => $this->args['width'], - 'height' => $this->args['height'], + Attribute::LAYOUT => Layout::RESPONSIVE, + Attribute::WIDTH => $this->args['width'], + Attribute::HEIGHT => $this->args['height'], ]; if ( ! empty( $video_id ) ) { - $attributes['data-videoid'] = $video_id; + $attributes[ Attribute::DATA_VIDEOID ] = $video_id; } // Find start time of video. @@ -282,60 +291,62 @@ private function prepare_attributes( $url, $video_id = '' ) { continue; } - if ( in_array( $key, [ 'autoplay', 'loop' ], true ) ) { + if ( in_array( $key, [ Attribute::AUTOPLAY, Attribute::LOOP ], true ) ) { $attributes[ $key ] = $value; continue; } if ( 'channel' === $key ) { - $attributes['data-live-channelid'] = $value; + $attributes[ Attribute::DATA_LIVE_CHANNELID ] = $value; continue; } - $attributes[ "data-param-$key" ] = esc_attr( $value ); + $attributes[ sanitize_key( "data-param-$key" ) ] = $value; } return $attributes; } /** - * Placeholder component for AMP YouTube component. + * Placeholder component for AMP YouTube component in the DOM. * - * @param DOMElement $amp_component DOM Element of AMP component. - * @param array $attributes YouTube Attributes. + * @param Element $amp_component AMP component element. + * @param array $attributes YouTube attributes. + * @param string $url YouTube URL. * - * @return DOMElement|false DOMElement on success otherwise false. + * @return Element|false DOMElement on success otherwise false. */ - private function get_placeholder_component( DOMElement $amp_component, $attributes ) { + private function get_placeholder_component( Element $amp_component, $attributes, $url ) { - if ( empty( $attributes['data-videoid'] ) ) { + if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { return false; } - $video_id = $attributes['data-videoid']; + $video_id = $attributes[ Attribute::DATA_VIDEOID ]; + $dom = Document::fromNode( $amp_component ); $img_attributes = [ Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $video_id ) ), - Attribute::LAYOUT => 'fill', + Attribute::LAYOUT => Layout::FILL, Attribute::OBJECT_FIT => 'cover', ]; - if ( $attributes['title'] ) { - $img_attributes[ Attribute::ALT ] = $attributes['title']; + if ( $attributes[ Attribute::TITLE ] ) { + $img_attributes[ Attribute::ALT ] = $attributes[ Attribute::TITLE ]; } $img_node = AMP_DOM_Utils::create_node( - Document::fromNode( $amp_component->ownerDocument ), + $dom, Tag::IMG, $img_attributes ); $placeholder = AMP_DOM_Utils::create_node( - Document::fromNode( $amp_component->ownerDocument ), + $dom, Tag::A, [ Attribute::PLACEHOLDER => '', - Attribute::HREF => esc_url_raw( sprintf( 'https://www.youtube.com/watch?v=%s', $video_id ) ), + Attribute::HREF => esc_url_raw( $url ), ] ); @@ -345,30 +356,30 @@ private function get_placeholder_component( DOMElement $amp_component, $attribut } /** - * To get placeholder for amp component. + * To get placeholder for AMP component as constructed HTML string. * * @param string $url YouTube URL. - * @param array $attributes YouTube Attributes. + * @param array $attributes YouTube attributes. * - * @return string + * @return string HTML string. */ private function get_placeholder( $url, $attributes ) { - if ( empty( $attributes['data-videoid'] ) ) { + if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { return ''; } $img_attributes = [ - Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $attributes['data-videoid'] ) ), - Attribute::LAYOUT => 'fill', + Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $attributes[ Attribute::DATA_VIDEOID ] ) ), + Attribute::LAYOUT => Layout::FILL, Attribute::OBJECT_FIT => 'cover', ]; - if ( ! empty( $attributes['title'] ) ) { - $img_attributes[ Attribute::ALT ] = $attributes['title']; + if ( ! empty( $attributes[ Attribute::TITLE ] ) ) { + $img_attributes[ Attribute::ALT ] = $attributes[ Attribute::TITLE ]; } - $img = AMP_HTML_Utils::build_tag( 'img', $img_attributes ); + $img = AMP_HTML_Utils::build_tag( Tag::IMG, $img_attributes ); return AMP_HTML_Utils::build_tag( Tag::A, @@ -396,7 +407,7 @@ private function get_video_id_from_url( $url ) { } $domain = implode( '.', array_slice( explode( '.', $parsed_url['host'] ), - 2 ) ); - if ( ! in_array( $domain, $this->applicable_domains, true ) ) { + if ( ! in_array( $domain, self::APPLICABLE_DOMAINS, true ) ) { return false; } @@ -507,16 +518,16 @@ private function get_start_time_from_url( $url ) { */ public function video_override( $html, $attr ) { - if ( ! isset( $attr['src'] ) ) { + if ( ! isset( $attr[ Attribute::SRC ] ) ) { return $html; } - $video_id = $this->get_video_id_from_url( $attr['src'] ); + $video_id = $this->get_video_id_from_url( $attr[ Attribute::SRC ] ); if ( ! $video_id ) { return $html; } - return $this->render( $html, $attr['src'], $video_id ); + return $this->render( $html, $attr[ Attribute::SRC ], $video_id ); } } diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php index 164abfc2c3f..f877d464dab 100644 --- a/includes/utils/class-amp-dom-utils.php +++ b/includes/utils/class-amp-dom-utils.php @@ -158,7 +158,7 @@ public static function get_content_from_dom_node( Document $dom, $node ) { * @param string $tag A valid HTML element tag for the element to be added. * @param string[] $attributes One of more valid attributes for the new node. * - * @return DOMElement|false The DOMElement for the given $tag, or false on failure + * @return Element|false The DOMElement for the given $tag, or false on failure */ public static function create_node( Document $dom, $tag, $attributes ) { $node = $dom->createElement( $tag ); diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index d8c5e96a69a..6511f262791 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -128,7 +128,7 @@ class AMP_Validation_Error_Taxonomy { * This is also used in WP_List_Table, like for the 'Bulk Actions' option. * When this is present, this ensures that this isn't filtered. * - * @var int + * @var int|string */ const NO_FILTER_VALUE = ''; diff --git a/src/BackgroundTask/MonitorCssTransientCaching.php b/src/BackgroundTask/MonitorCssTransientCaching.php index 309b4f76311..a2965aefe84 100644 --- a/src/BackgroundTask/MonitorCssTransientCaching.php +++ b/src/BackgroundTask/MonitorCssTransientCaching.php @@ -108,7 +108,6 @@ public function process( ...$args ) { $date = isset( $args[0] ) && $args[0] instanceof DateTimeInterface ? $args[0] : new DateTimeImmutable(); - /** @phpstan-ignore-next-line */ $transient_count = isset( $args[1] ) ? (int) $args[1] : $this->query_css_transient_count(); $date_string = $date->format( 'Ymd' ); From 74dfcb83f3a745399a2a03074e23797cb5d03cff Mon Sep 17 00:00:00 2001 From: Dhaval Parekh Date: Thu, 9 Sep 2021 15:20:51 +0530 Subject: [PATCH 25/43] update unit test cases --- tests/php/test-class-amp-youtube-embed-handler.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 41201c03291..00e7f030474 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -110,15 +110,15 @@ public function sanitize_raw_embeds_data_provider() { return [ 'youtube-embed' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'with_http' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'short-url' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'live_streaming_channel' => [ 'source' => '', From 7f21703d9563066e6fbc4fda7d070ef514fd6ca3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 09:24:34 -0700 Subject: [PATCH 26/43] Revert phpstan ignores --- includes/embeds/class-amp-twitter-embed-handler.php | 1 - src/BackgroundTask/MonitorCssTransientCaching.php | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/embeds/class-amp-twitter-embed-handler.php b/includes/embeds/class-amp-twitter-embed-handler.php index c33c71e7915..9dd030b5164 100644 --- a/includes/embeds/class-amp-twitter-embed-handler.php +++ b/includes/embeds/class-amp-twitter-embed-handler.php @@ -19,7 +19,6 @@ class AMP_Twitter_Embed_Handler extends AMP_Base_Embed_Handler { /** * Default width. * - * @phpstan-ignore-next-line * @var int|string */ protected $DEFAULT_WIDTH = 'auto'; diff --git a/src/BackgroundTask/MonitorCssTransientCaching.php b/src/BackgroundTask/MonitorCssTransientCaching.php index a2965aefe84..309b4f76311 100644 --- a/src/BackgroundTask/MonitorCssTransientCaching.php +++ b/src/BackgroundTask/MonitorCssTransientCaching.php @@ -108,6 +108,7 @@ public function process( ...$args ) { $date = isset( $args[0] ) && $args[0] instanceof DateTimeInterface ? $args[0] : new DateTimeImmutable(); + /** @phpstan-ignore-next-line */ $transient_count = isset( $args[1] ) ? (int) $args[1] : $this->query_css_transient_count(); $date_string = $date->format( 'Ymd' ); From 2c3ed4ddee61e326212bb2b27977553de2a9c6cd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 10:16:47 -0700 Subject: [PATCH 27/43] Try adding phpstan version to composer cache key --- .github/workflows/build-test-measure.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index 4349a3673f2..929614f1fdf 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -223,7 +223,7 @@ jobs: uses: actions/cache@v2 with: path: ${{ steps.composer-cache.outputs.dir }} - key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} + key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }}-phpstan0.12.98 restore-keys: | ${{ runner.os }}-composer- From d51922080faca21fbd42af5c8b969b7c961f9d23 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 10:31:44 -0700 Subject: [PATCH 28/43] Revert "Revert phpstan ignores" This reverts commit 7f21703d9563066e6fbc4fda7d070ef514fd6ca3. --- includes/embeds/class-amp-twitter-embed-handler.php | 1 + src/BackgroundTask/MonitorCssTransientCaching.php | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/embeds/class-amp-twitter-embed-handler.php b/includes/embeds/class-amp-twitter-embed-handler.php index 9dd030b5164..c33c71e7915 100644 --- a/includes/embeds/class-amp-twitter-embed-handler.php +++ b/includes/embeds/class-amp-twitter-embed-handler.php @@ -19,6 +19,7 @@ class AMP_Twitter_Embed_Handler extends AMP_Base_Embed_Handler { /** * Default width. * + * @phpstan-ignore-next-line * @var int|string */ protected $DEFAULT_WIDTH = 'auto'; diff --git a/src/BackgroundTask/MonitorCssTransientCaching.php b/src/BackgroundTask/MonitorCssTransientCaching.php index 309b4f76311..a2965aefe84 100644 --- a/src/BackgroundTask/MonitorCssTransientCaching.php +++ b/src/BackgroundTask/MonitorCssTransientCaching.php @@ -108,7 +108,6 @@ public function process( ...$args ) { $date = isset( $args[0] ) && $args[0] instanceof DateTimeInterface ? $args[0] : new DateTimeImmutable(); - /** @phpstan-ignore-next-line */ $transient_count = isset( $args[1] ) ? (int) $args[1] : $this->query_css_transient_count(); $date_string = $date->format( 'Ymd' ); From 0524ae5729eeff2a59f4aa25772eb91907d9d404 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 10:32:57 -0700 Subject: [PATCH 29/43] Print phpstan version when running on GHA --- .github/workflows/build-test-measure.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index 929614f1fdf..4d7f86701e1 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -231,7 +231,9 @@ jobs: run: composer install - name: Static Analysis (PHPStan) - run: vendor/bin/phpstan analyse + run: | + vendor/bin/phpstan --version + vendor/bin/phpstan analyse #----------------------------------------------------------------------------------------------------------------------- From b9827d71fcfe03c1f4a5a78656e76dfd715a72b5 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 10:43:17 -0700 Subject: [PATCH 30/43] Try removing phpstan before composer install --- .github/workflows/build-test-measure.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index 4d7f86701e1..cc1286536c6 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -223,12 +223,14 @@ jobs: uses: actions/cache@v2 with: path: ${{ steps.composer-cache.outputs.dir }} - key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }}-phpstan0.12.98 + key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} restore-keys: | ${{ runner.os }}-composer- - name: Install Composer dependencies - run: composer install + run: + if [ -e vendor/bin/phpstan ]; then rm vendor/bin/phpstan; fi + composer install - name: Static Analysis (PHPStan) run: | From fa6ee15d5a1b5ec4b52ea15dbbe5ca2f095ab3be Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 10:46:57 -0700 Subject: [PATCH 31/43] Fix yml syntax error --- .github/workflows/build-test-measure.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index cc1286536c6..d0fd38911fa 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -228,7 +228,7 @@ jobs: ${{ runner.os }}-composer- - name: Install Composer dependencies - run: + run: | if [ -e vendor/bin/phpstan ]; then rm vendor/bin/phpstan; fi composer install From f93b9a1014cfa15e5b29c80e1483b8a20b9ca6a2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 10:53:11 -0700 Subject: [PATCH 32/43] Try downloading a specific version of phpstan instead of latest --- .github/workflows/build-test-measure.yml | 4 +--- composer.json | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index d0fd38911fa..d285372a75f 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -228,9 +228,7 @@ jobs: ${{ runner.os }}-composer- - name: Install Composer dependencies - run: | - if [ -e vendor/bin/phpstan ]; then rm vendor/bin/phpstan; fi - composer install + run: composer install - name: Static Analysis (PHPStan) run: | diff --git a/composer.json b/composer.json index 2fa70b1d6e2..6009be9d0a9 100644 --- a/composer.json +++ b/composer.json @@ -54,7 +54,7 @@ "phpstan": { "path": "vendor/bin/phpstan", "type": "phar", - "url": "https://github.com/phpstan/phpstan/releases/latest/download/phpstan.phar" + "url": "https://github.com/phpstan/phpstan/releases/download/0.12.98/phpstan.phar" } }, "patches": { From 9d51ef490f3424adbd729c831513116ebaf7ecc2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:00:42 -0700 Subject: [PATCH 33/43] Try installing phpstan via shivammathur/setup-php Co-authored-by: Pierre Gordon --- .github/workflows/build-test-measure.yml | 5 +++-- composer.json | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml index d285372a75f..0b34c20be8f 100644 --- a/.github/workflows/build-test-measure.yml +++ b/.github/workflows/build-test-measure.yml @@ -214,6 +214,7 @@ jobs: # phpstan requires PHP 7.1+. php-version: 7.4 extensions: dom, iconv, json, libxml, zip + tools: phpstan - name: Get Composer Cache Directory id: composer-cache @@ -232,8 +233,8 @@ jobs: - name: Static Analysis (PHPStan) run: | - vendor/bin/phpstan --version - vendor/bin/phpstan analyse + phpstan --version + phpstan analyse #----------------------------------------------------------------------------------------------------------------------- diff --git a/composer.json b/composer.json index 6009be9d0a9..2fa70b1d6e2 100644 --- a/composer.json +++ b/composer.json @@ -54,7 +54,7 @@ "phpstan": { "path": "vendor/bin/phpstan", "type": "phar", - "url": "https://github.com/phpstan/phpstan/releases/download/0.12.98/phpstan.phar" + "url": "https://github.com/phpstan/phpstan/releases/latest/download/phpstan.phar" } }, "patches": { From 9d8e977775f437b4ffff63a444855af4184522ce Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:19:34 -0700 Subject: [PATCH 34/43] Fix issues identified by phpstan --- includes/embeds/class-amp-base-embed-handler.php | 4 +++- includes/embeds/class-amp-twitter-embed-handler.php | 1 - includes/utils/class-amp-dom-utils.php | 2 +- includes/validation/class-amp-validation-error-taxonomy.php | 4 ++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/includes/embeds/class-amp-base-embed-handler.php b/includes/embeds/class-amp-base-embed-handler.php index 70b869ff1d2..ace710a8000 100644 --- a/includes/embeds/class-amp-base-embed-handler.php +++ b/includes/embeds/class-amp-base-embed-handler.php @@ -21,7 +21,9 @@ abstract class AMP_Base_Embed_Handler { /** * Default width. * - * @var int + * In some cases, this may be the string 'auto' when a fixed-height layout is used. + * + * @var int|string */ protected $DEFAULT_WIDTH = 600; diff --git a/includes/embeds/class-amp-twitter-embed-handler.php b/includes/embeds/class-amp-twitter-embed-handler.php index c33c71e7915..9dd030b5164 100644 --- a/includes/embeds/class-amp-twitter-embed-handler.php +++ b/includes/embeds/class-amp-twitter-embed-handler.php @@ -19,7 +19,6 @@ class AMP_Twitter_Embed_Handler extends AMP_Base_Embed_Handler { /** * Default width. * - * @phpstan-ignore-next-line * @var int|string */ protected $DEFAULT_WIDTH = 'auto'; diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php index f877d464dab..a3dca61073f 100644 --- a/includes/utils/class-amp-dom-utils.php +++ b/includes/utils/class-amp-dom-utils.php @@ -158,7 +158,7 @@ public static function get_content_from_dom_node( Document $dom, $node ) { * @param string $tag A valid HTML element tag for the element to be added. * @param string[] $attributes One of more valid attributes for the new node. * - * @return Element|false The DOMElement for the given $tag, or false on failure + * @return Element|false The element for the given $tag, or false on failure */ public static function create_node( Document $dom, $tag, $attributes ) { $node = $dom->createElement( $tag ); diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 6511f262791..206a3ed8d17 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -128,7 +128,7 @@ class AMP_Validation_Error_Taxonomy { * This is also used in WP_List_Table, like for the 'Bulk Actions' option. * When this is present, this ensures that this isn't filtered. * - * @var int|string + * @var string */ const NO_FILTER_VALUE = ''; @@ -1095,7 +1095,7 @@ public static function add_term_filter_query_var( $url, $tax ) { && in_array( $_POST[ self::VALIDATION_ERROR_TYPE_QUERY_VAR ], // phpcs:ignore WordPress.Security.NonceVerification.Missing - array_merge( self::get_error_types(), [ (string) self::NO_FILTER_VALUE ] ), + array_merge( self::get_error_types(), [ self::NO_FILTER_VALUE ] ), true ) ) { From 8200f3f7c7161b455b73f6515bd5438903d10b44 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:26:13 -0700 Subject: [PATCH 35/43] Give more explicit array phpdoc type --- includes/validation/class-amp-validation-error-taxonomy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 206a3ed8d17..4eac9a96dc2 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -1457,7 +1457,7 @@ private static function output_error_status_filter_option_markup( $option_text, /** * Gets all of the possible error types. * - * @return array Error types. + * @return string[] Error types. */ public static function get_error_types() { return [ self::HTML_ELEMENT_ERROR_TYPE, self::HTML_ATTRIBUTE_ERROR_TYPE, self::JS_ERROR_TYPE, self::CSS_ERROR_TYPE ]; From edbcc6f6859f80fe31d5b05a92a7de693aed4284 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:32:20 -0700 Subject: [PATCH 36/43] Remove unreachable code --- includes/embeds/class-amp-youtube-embed-handler.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index dccdb4fd3c4..b47ea857de4 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -258,10 +258,6 @@ private function get_amp_component( Document $dom, Element $node ) { */ private function prepare_attributes( $url, $video_id = '' ) { - if ( empty( $url ) ) { - return []; - } - $attributes = [ Attribute::LAYOUT => Layout::RESPONSIVE, Attribute::WIDTH => $this->args['width'], @@ -469,10 +465,6 @@ private function get_video_id_from_url( $url ) { */ private function get_start_time_from_url( $url ) { - if ( empty( $url ) ) { - return 0; - } - $start_time = 0; $parsed_url = wp_parse_url( $url ); From 32f358a4ca3201d005c12b965ef7c368cb8fc8c4 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:40:35 -0700 Subject: [PATCH 37/43] Opt to pass-through iframe for unrecognized YouTube URL rather than convert to link --- includes/embeds/class-amp-youtube-embed-handler.php | 12 +----------- tests/php/test-class-amp-youtube-embed-handler.php | 4 ++++ 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index b47ea857de4..87b4cb562f3 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -218,17 +218,7 @@ private function get_amp_component( Document $dom, Element $node ) { } if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) && empty( $attributes[ Attribute::DATA_LIVE_CHANNELID ] ) ) { - $link_node = AMP_DOM_Utils::create_node( - $dom, - Tag::A, - [ - Attribute::HREF => esc_url_raw( $url ), - Attribute::CLASS_ => 'amp-wp-embed-fallback', - ] - ); - $link_node->appendChild( $dom->createTextNode( $url ) ); - - return $link_node; + return false; } $amp_node = AMP_DOM_Utils::create_node( diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 00e7f030474..a8d876498e8 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -128,6 +128,10 @@ public function sanitize_raw_embeds_data_provider() { 'source' => '', 'expected' => '', ], + 'youtube-unknown' => [ + 'source' => '', + 'expected' => '', + ], ]; } From 1907df33e15a220dbf2e8771801de2006b33ceec Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:49:12 -0700 Subject: [PATCH 38/43] Remove condition which cannot be reached since video ID is always provided --- .../class-amp-youtube-embed-handler.php | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 87b4cb562f3..d06c5ba3657 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -145,19 +145,7 @@ public function render( $html, $url, $video_id ) { } } - if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { - - return AMP_HTML_Utils::build_tag( - Tag::A, - [ - Attribute::HREF => esc_url_raw( $url ), - Attribute::CLASS_ => 'amp-wp-embed-fallback', - ], - esc_url( $url ) - ); - } - - $placeholder = $this->get_placeholder( $url, $attributes ); + $placeholder = $this->get_placeholder_markup( $url, $attributes ); return AMP_HTML_Utils::build_tag( Extension::YOUTUBE, $attributes, $placeholder ); } @@ -349,7 +337,7 @@ private function get_placeholder_component( Element $amp_component, $attributes, * * @return string HTML string. */ - private function get_placeholder( $url, $attributes ) { + private function get_placeholder_markup( $url, $attributes ) { if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { return ''; @@ -504,12 +492,13 @@ public function video_override( $html, $attr ) { return $html; } - $video_id = $this->get_video_id_from_url( $attr[ Attribute::SRC ] ); + $src = $attr[ Attribute::SRC ]; + $video_id = $this->get_video_id_from_url( $src ); if ( ! $video_id ) { return $html; } - return $this->render( $html, $attr[ Attribute::SRC ], $video_id ); + return $this->render( $html, $src, $video_id ); } } From aac0e402a7a7a7a02ecc5a790579f9d6ec3b6f12 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 11:52:08 -0700 Subject: [PATCH 39/43] Use coversDefaultClass --- tests/php/test-class-amp-youtube-embed-handler.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index a8d876498e8..a320b273f14 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -13,7 +13,7 @@ /** * Tests for AMP_YouTube_Embed_Handler. * - * @covers AMP_YouTube_Embed_Handler + * @coversDefaultClass \AMP_YouTube_Embed_Handler */ class Test_AMP_YouTube_Embed_Handler extends TestCase { @@ -35,7 +35,7 @@ class Test_AMP_YouTube_Embed_Handler extends TestCase { /** * An instance of this embed handler. * - * @var AMP_YouTube_Embed_Handler. + * @var AMP_YouTube_Embed_Handler */ public $handler; @@ -138,7 +138,7 @@ public function sanitize_raw_embeds_data_provider() { /** * @dataProvider sanitize_raw_embeds_data_provider * - * @covers AMP_YouTube_Embed_Handler::sanitize_raw_embeds + * @covers ::sanitize_raw_embeds */ public function test_sanitize_raw_embeds( $source, $expected ) { @@ -159,7 +159,7 @@ public function test_sanitize_raw_embeds( $source, $expected ) { /** * Test video_override(). * - * @covers AMP_YouTube_Embed_Handler::video_override() + * @covers ::video_override() */ public function test_video_override() { remove_all_filters( 'wp_video_shortcode_override' ); @@ -378,7 +378,7 @@ public function get_video_id_data() { * Tests get_video_id_from_url. * * @dataProvider get_video_id_data - * @covers AMP_YouTube_Embed_Handler::get_video_id_from_url() + * @covers ::get_video_id_from_url() * * @param string $url The URL to test. * @param string|false $expected The expected result. @@ -422,7 +422,7 @@ public function get_start_time_data() { * Tests get_start_time_from_url. * * @dataProvider get_start_time_data - * @covers AMP_YouTube_Embed_Handler::get_start_time_from_url + * @covers ::get_start_time_from_url * * @param string $url The URL to test. * @param string|false $expected The expected result. From d983fa45272288655caaeff03e492d7e9af96a8e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 12:03:48 -0700 Subject: [PATCH 40/43] Prevent generating closing IMG tags --- .../class-amp-youtube-embed-handler.php | 2 +- .../test-class-amp-youtube-embed-handler.php | 36 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index d06c5ba3657..21a7817dc78 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -353,7 +353,7 @@ private function get_placeholder_markup( $url, $attributes ) { $img_attributes[ Attribute::ALT ] = $attributes[ Attribute::TITLE ]; } - $img = AMP_HTML_Utils::build_tag( Tag::IMG, $img_attributes ); + $img = ''; return AMP_HTML_Utils::build_tag( Tag::A, diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index a320b273f14..3fa2f81f84a 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -203,20 +203,20 @@ public function get_conversion_data() { 'url_simple' => [ 'https://www.youtube.com/watch?v=kfVsfOSbJY0' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'url_short' => [ 'https://youtu.be/kfVsfOSbJY0' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'url_with_querystring' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0&hl=en&fs=1&w=425&h=349' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], // Several reports of invalid URLs that have multiple `?` in the URL. @@ -227,38 +227,38 @@ public function get_conversion_data() { 'embed_url' => [ 'https://www.youtube.com/embed/kfVsfOSbJY0' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'with_start_time' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m10s' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'with_start_time_2' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=62' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'with_start_time_3' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=62s' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'with_start_time_4' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], 'with_start_time_5' => [ 'http://www.youtube.com/watch?v=kfVsfOSbJY0#t=1m2' . PHP_EOL, - '

Rebecca Black - Friday

' . PHP_EOL, - '

' . PHP_EOL, + '

Rebecca Black - Friday

' . PHP_EOL, + '

' . PHP_EOL, ], ]; } From 9eefef9e4c9e5a7c4b436a7fbde0ddab67fdb26f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 14:36:49 -0700 Subject: [PATCH 41/43] Let placeholder link point to YouTube permalink and not iframe src --- .../class-amp-youtube-embed-handler.php | 12 ++++++---- .../test-class-amp-youtube-embed-handler.php | 23 +++++++++++++++---- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 21a7817dc78..6ecacce7262 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -216,7 +216,7 @@ private function get_amp_component( Document $dom, Element $node ) { ); if ( ! empty( $amp_node ) && $amp_node instanceof Element ) { - $placeholder = $this->get_placeholder_component( $amp_node, $attributes, $url ); + $placeholder = $this->get_placeholder_component( $amp_node, $attributes ); if ( $placeholder ) { $amp_node->appendChild( $placeholder ); @@ -286,11 +286,10 @@ private function prepare_attributes( $url, $video_id = '' ) { * * @param Element $amp_component AMP component element. * @param array $attributes YouTube attributes. - * @param string $url YouTube URL. * * @return Element|false DOMElement on success otherwise false. */ - private function get_placeholder_component( Element $amp_component, $attributes, $url ) { + private function get_placeholder_component( Element $amp_component, $attributes ) { if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { return false; @@ -315,12 +314,17 @@ private function get_placeholder_component( Element $amp_component, $attributes, $img_attributes ); + $video_url = esc_url_raw( sprintf( 'https://www.youtube.com/watch?v=%s', $video_id ) ); + if ( array_key_exists( 'start', $attributes ) ) { + $video_url .= '#t=' . $attributes['start']; + } + $placeholder = AMP_DOM_Utils::create_node( $dom, Tag::A, [ Attribute::PLACEHOLDER => '', - Attribute::HREF => esc_url_raw( $url ), + Attribute::HREF => $video_url, ] ); diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index 3fa2f81f84a..a4815a20756 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -110,15 +110,19 @@ public function sanitize_raw_embeds_data_provider() { return [ 'youtube-embed' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', + ], + 'youtube-start' => [ + 'source' => '', + 'expected' => 'YouTube video player', ], 'with_http' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'short-url' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'live_streaming_channel' => [ 'source' => '', @@ -138,7 +142,9 @@ public function sanitize_raw_embeds_data_provider() { /** * @dataProvider sanitize_raw_embeds_data_provider * - * @covers ::sanitize_raw_embeds + * @covers ::sanitize_raw_embeds() + * @covers ::get_amp_component() + * @covers ::get_placeholder_component() */ public function test_sanitize_raw_embeds( $source, $expected ) { @@ -159,7 +165,10 @@ public function test_sanitize_raw_embeds( $source, $expected ) { /** * Test video_override(). * + * @covers ::register_embed() * @covers ::video_override() + * @covers ::render() + * @covers ::get_placeholder_markup() */ public function test_video_override() { remove_all_filters( 'wp_video_shortcode_override' ); @@ -194,6 +203,7 @@ public function test_video_override() { remove_all_filters( 'wp_video_shortcode_override' ); } + /** @return array */ public function get_conversion_data() { return [ 'no_embed' => [ @@ -265,6 +275,8 @@ public function get_conversion_data() { /** * @dataProvider get_conversion_data + * @covers ::filter_embed_oembed_html() + * @covers ::render() */ public function test__conversion( $source, $expected, $fallback_for_expected = null ) { $this->handler->register_embed(); @@ -422,7 +434,7 @@ public function get_start_time_data() { * Tests get_start_time_from_url. * * @dataProvider get_start_time_data - * @covers ::get_start_time_from_url + * @covers ::get_start_time_from_url() * * @param string $url The URL to test. * @param string|false $expected The expected result. @@ -437,6 +449,7 @@ public function test_get_start_time_from_url( $url, $expected ) { ); } + /** @return array */ public function get_scripts_data() { return [ 'not_converted' => [ From 46bc356f00325f8e626bce1171b9dfbdd77f8c26 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 14:55:02 -0700 Subject: [PATCH 42/43] Fix adding start to placeholder link; improve coverage --- .../class-amp-youtube-embed-handler.php | 11 ++++---- .../test-class-amp-youtube-embed-handler.php | 25 ++++++++++++++++--- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 6ecacce7262..701105d66b1 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -100,6 +100,7 @@ public function register_embed() { */ public function unregister_embed() { remove_filter( 'embed_oembed_html', [ $this, 'filter_embed_oembed_html' ], 10 ); + remove_filter( 'wp_video_shortcode_override', [ $this, 'video_override' ], 10 ); } /** @@ -216,7 +217,7 @@ private function get_amp_component( Document $dom, Element $node ) { ); if ( ! empty( $amp_node ) && $amp_node instanceof Element ) { - $placeholder = $this->get_placeholder_component( $amp_node, $attributes ); + $placeholder = $this->get_placeholder_element( $amp_node, $attributes ); if ( $placeholder ) { $amp_node->appendChild( $placeholder ); @@ -282,14 +283,14 @@ private function prepare_attributes( $url, $video_id = '' ) { } /** - * Placeholder component for AMP YouTube component in the DOM. + * Placeholder element for AMP YouTube component in the DOM. * * @param Element $amp_component AMP component element. * @param array $attributes YouTube attributes. * * @return Element|false DOMElement on success otherwise false. */ - private function get_placeholder_component( Element $amp_component, $attributes ) { + private function get_placeholder_element( Element $amp_component, $attributes ) { if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { return false; @@ -315,8 +316,8 @@ private function get_placeholder_component( Element $amp_component, $attributes ); $video_url = esc_url_raw( sprintf( 'https://www.youtube.com/watch?v=%s', $video_id ) ); - if ( array_key_exists( 'start', $attributes ) ) { - $video_url .= '#t=' . $attributes['start']; + if ( array_key_exists( 'data-param-start', $attributes ) ) { + $video_url .= '#t=' . $attributes['data-param-start']; } $placeholder = AMP_DOM_Utils::create_node( diff --git a/tests/php/test-class-amp-youtube-embed-handler.php b/tests/php/test-class-amp-youtube-embed-handler.php index a4815a20756..ff0348864cc 100644 --- a/tests/php/test-class-amp-youtube-embed-handler.php +++ b/tests/php/test-class-amp-youtube-embed-handler.php @@ -101,7 +101,21 @@ public function mock_http_request( $preempt, $r, $url ) { } /** - * data provider for $this->test_sanitize_raw_embeds() + * @covers ::register_embed() + * @covers ::unregister_embed() + */ + public function test_register_and_unregister_embed() { + $embed = new AMP_YouTube_Embed_Handler(); + $embed->register_embed(); + $this->assertEquals( 10, has_filter( 'embed_oembed_html', [ $embed, 'filter_embed_oembed_html' ] ) ); + $this->assertEquals( 10, has_filter( 'wp_video_shortcode_override', [ $embed, 'video_override' ] ) ); + $embed->unregister_embed(); + $this->assertFalse( has_filter( 'embed_oembed_html', [ $embed, 'filter_embed_oembed_html' ] ) ); + $this->assertFalse( has_filter( 'wp_video_shortcode_override', [ $embed, 'video_override' ] ) ); + } + + /** + * Data provider for $this->test_sanitize_raw_embeds() * * @return string[][] */ @@ -114,7 +128,7 @@ public function sanitize_raw_embeds_data_provider() { ], 'youtube-start' => [ 'source' => '', - 'expected' => 'YouTube video player', + 'expected' => 'YouTube video player', ], 'with_http' => [ 'source' => '', @@ -144,7 +158,8 @@ public function sanitize_raw_embeds_data_provider() { * * @covers ::sanitize_raw_embeds() * @covers ::get_amp_component() - * @covers ::get_placeholder_component() + * @covers ::get_placeholder_element() + * @covers ::prepare_attributes() */ public function test_sanitize_raw_embeds( $source, $expected ) { @@ -275,8 +290,10 @@ public function get_conversion_data() { /** * @dataProvider get_conversion_data + * @covers ::register_embed() * @covers ::filter_embed_oembed_html() * @covers ::render() + * @covers ::get_placeholder_markup() */ public function test__conversion( $source, $expected, $fallback_for_expected = null ) { $this->handler->register_embed(); @@ -465,6 +482,8 @@ public function get_scripts_data() { /** * @dataProvider get_scripts_data + * @covers ::register_embed() + * @covers ::get_scripts() */ public function test__get_scripts( $source, $expected ) { $this->handler->register_embed(); From da36d2d6e94e07aa1e69fae0d737fff6494c777f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 15:14:54 -0700 Subject: [PATCH 43/43] Make placeholder dependency on video ID more explicit --- .../class-amp-youtube-embed-handler.php | 35 +++++++------------ 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/includes/embeds/class-amp-youtube-embed-handler.php b/includes/embeds/class-amp-youtube-embed-handler.php index 701105d66b1..3319e589cc6 100644 --- a/includes/embeds/class-amp-youtube-embed-handler.php +++ b/includes/embeds/class-amp-youtube-embed-handler.php @@ -146,7 +146,7 @@ public function render( $html, $url, $video_id ) { } } - $placeholder = $this->get_placeholder_markup( $url, $attributes ); + $placeholder = $this->get_placeholder_markup( $url, $video_id, $attributes ); return AMP_HTML_Utils::build_tag( Extension::YOUTUBE, $attributes, $placeholder ); } @@ -183,7 +183,6 @@ static function ( $domain ) { $node->parentNode->replaceChild( $amp_youtube_component, $node ); } } - } /** @@ -216,12 +215,10 @@ private function get_amp_component( Document $dom, Element $node ) { $attributes ); - if ( ! empty( $amp_node ) && $amp_node instanceof Element ) { - $placeholder = $this->get_placeholder_element( $amp_node, $attributes ); - - if ( $placeholder ) { - $amp_node->appendChild( $placeholder ); - } + if ( $video_id && $amp_node instanceof Element ) { + $amp_node->appendChild( + $this->get_placeholder_element( $amp_node, $video_id, $attributes ) + ); } return $amp_node; @@ -286,18 +283,13 @@ private function prepare_attributes( $url, $video_id = '' ) { * Placeholder element for AMP YouTube component in the DOM. * * @param Element $amp_component AMP component element. + * @param string $video_id Video ID. * @param array $attributes YouTube attributes. * - * @return Element|false DOMElement on success otherwise false. + * @return Element Placeholder. */ - private function get_placeholder_element( Element $amp_component, $attributes ) { - - if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { - return false; - } - - $video_id = $attributes[ Attribute::DATA_VIDEOID ]; - $dom = Document::fromNode( $amp_component ); + private function get_placeholder_element( Element $amp_component, $video_id, $attributes ) { + $dom = Document::fromNode( $amp_component ); $img_attributes = [ Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $video_id ) ), @@ -338,18 +330,15 @@ private function get_placeholder_element( Element $amp_component, $attributes ) * To get placeholder for AMP component as constructed HTML string. * * @param string $url YouTube URL. + * @param string $video_id Video ID. * @param array $attributes YouTube attributes. * * @return string HTML string. */ - private function get_placeholder_markup( $url, $attributes ) { - - if ( empty( $attributes[ Attribute::DATA_VIDEOID ] ) ) { - return ''; - } + private function get_placeholder_markup( $url, $video_id, $attributes ) { $img_attributes = [ - Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $attributes[ Attribute::DATA_VIDEOID ] ) ), + Attribute::SRC => esc_url_raw( sprintf( 'https://i.ytimg.com/vi/%s/hqdefault.jpg', $video_id ) ), Attribute::LAYOUT => Layout::FILL, Attribute::OBJECT_FIT => 'cover', ];