Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix conversion of video to amp-video #1477

Merged
merged 3 commits into from Sep 28, 2018
Merged

Fix conversion of video to amp-video #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd21353
Fix undefined index notice when video lacks src attribute
westonruter Sep 27, 2018
cf6d2a2
Preserve extra video attributes and non-source children in amp-video …
westonruter Sep 27, 2018
c1f3cff
Set fallback attribute on first non-source/track child element of amp…
westonruter Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Expand Up @@ -766,6 +766,7 @@ public static function add_hooks() {
remove_action( 'wp_print_styles', 'print_emoji_styles' );
remove_action( 'wp_head', 'wp_oembed_add_host_js' );

// @todo The wp_mediaelement_fallback() should still run to be injected inside of the audio/video generated by wp_audio_shortcode()/wp_video_shortcode() respectively.
// Prevent MediaElement.js scripts/styles from being enqueued.
add_filter( 'wp_video_shortcode_library', function() {
return 'amp';
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-base-sanitizer.php
View file
Open in desktop
Expand Up @@ -317,7 +317,7 @@ public function add_or_append_attribute( &$attributes, $key, $value, $separator
* @return string URL which may have been updated with HTTPS, or may have been made empty.
*/
public function maybe_enforce_https_src( $src, $force_https = false ) {
$protocol = strtok( $src, ':' );
$protocol = strtok( $src, ':' ); // @todo What about relative URLs? This should use wp_parse_url( $src, PHP_URL_SCHEME )
if ( 'https' !== $protocol ) {
// Check if https is required.
if ( isset( $this->args['require_https_src'] ) && true === $this->args['require_https_src'] ) {
Expand Down
99 changes: 45 additions & 54 deletions includes/sanitizers/class-amp-video-sanitizer.php
View file
Open in desktop
Expand Up @@ -62,46 +62,50 @@ public function sanitize() {
$old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node );
$old_attributes = $this->filter_data_amp_attributes( $old_attributes, $amp_data );

$sources_count = 0;
$new_attributes = $this->filter_attributes( $old_attributes );
$layout = isset( $amp_data['layout'] ) ? $amp_data['layout'] : false;
$new_attributes = $this->filter_video_dimensions( $new_attributes );
$new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout );
$new_attributes = $this->set_layout( $new_attributes );
if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['width'] ) && ! empty( $new_attributes['height'] ) ) {
$new_attributes['layout'] = 'responsive';
if ( isset( $new_attributes['src'] ) ) {
$new_attributes = $this->filter_video_dimensions( $new_attributes, $new_attributes['src'] );
if ( $new_attributes['src'] ) {
$sources_count++;
}
}

$new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes );

foreach ( $node->childNodes as $child_node ) {
/**
* Child node.
*
* @todo: Fix when `source` has no closing tag as DOMDocument does not handle well.
*
* @var DOMNode $child_node
*/
$new_child_node = $child_node->cloneNode( true );
if ( ! $new_child_node instanceof DOMElement ) {
continue;
// Gather all child nodes and supply empty video dimensions from sources.
$fallback = null;
$child_nodes = array();
while ( $node->firstChild ) {
$child_node = $node->removeChild( $node->firstChild );
if ( $child_node instanceof DOMElement && 'source' === $child_node->nodeName && $child_node->hasAttribute( 'src' ) ) {
$src = $this->maybe_enforce_https_src( $child_node->getAttribute( 'src' ), true );
if ( ! $src ) {
// @todo $this->remove_invalid_child( $child_node ), but this will require refactoring the while loop since it uses firstChild.
continue; // Skip adding source.
}
$sources_count++;
$child_node->setAttribute( 'src', $src );
$new_attributes = $this->filter_video_dimensions( $new_attributes, $src );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to pass $src to $this->filter_video_dimensions().

}

$old_child_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $new_child_node );
$new_child_attributes = $this->filter_attributes( $old_child_attributes );

if ( empty( $new_child_attributes['src'] ) ) {
continue;
}
if ( 'source' !== $new_child_node->tagName ) {
continue;
if ( ! $fallback && $child_node instanceof DOMElement && ! ( 'source' === $child_node->nodeName || 'track' === $child_node->nodeName ) ) {
$fallback = $child_node;
$fallback->setAttribute( 'fallback', '' );
}

$this->update_src( $new_child_node, $new_child_attributes['src'], $old_child_attributes['src'] );
$child_nodes[] = $child_node;
}

$new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout );
if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['width'] ) && ! empty( $new_attributes['height'] ) ) {
$new_attributes['layout'] = 'responsive';
}
$new_attributes = $this->set_layout( $new_attributes );

/**
* Only append source tags with a valid src attribute
*/
$new_node->appendChild( $new_child_node );
// @todo Make sure poster and artwork attributes are HTTPS.
$new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes );
foreach ( $child_nodes as $child_node ) {
$new_node->appendChild( $child_node );
}

/*
Expand All @@ -111,7 +115,7 @@ public function sanitize() {
* TODO: Add a fallback handler.
* See: https://github.com/ampproject/amphtml/issues/2261
*/
if ( 0 === $new_node->childNodes->length && empty( $new_attributes['src'] ) ) {
if ( 0 === $sources_count ) {
$this->remove_invalid_child( $node );
} else {
$node->parentNode->replaceChild( $new_node, $node );
Expand All @@ -125,16 +129,17 @@ public function sanitize() {
/**
* Filter video dimensions, try to get width and height from original file if missing.
*
* @param array $new_attributes Attributes.
*
* @param array $new_attributes Attributes.
* @param string $src Video URL.
* @return array Modified attributes.
*/
protected function filter_video_dimensions( $new_attributes ) {
protected function filter_video_dimensions( $new_attributes, $src ) {
if ( empty( $new_attributes['width'] ) || empty( $new_attributes['height'] ) ) {

// Get the width and height from the file.
$ext = pathinfo( $new_attributes['src'], PATHINFO_EXTENSION );
$name = wp_basename( $new_attributes['src'], ".$ext" );
$path = wp_parse_url( $src, PHP_URL_PATH );
$ext = pathinfo( $path, PATHINFO_EXTENSION );
$name = sanitize_title( wp_basename( $path, ".$ext" ) );
$args = array(
'name' => $name,
'post_type' => 'attachment',
Expand Down Expand Up @@ -193,9 +198,9 @@ private function filter_attributes( $attributes ) {
$out[ $name ] = $this->sanitize_dimension( $value, $name );
break;

// @todo Convert to HTTPS when is_ssl().
case 'poster':
case 'class':
case 'sizes':
case 'artwork':
$out[ $name ] = $value;
break;

Expand All @@ -217,24 +222,10 @@ private function filter_attributes( $attributes ) {
break;

default:
break;
$out[ $name ] = $value;
}
}

return $out;
}

/**
* Update the node's src attribute if it is different from the old src attribute.
*
* @param DOMNode $node The given DOMNode.
* @param string $new_src The new src attribute.
* @param string $old_src The old src attribute.
*/
protected function update_src( &$node, $new_src, $old_src ) {
if ( $old_src === $new_src ) {
return;
}
$node->setAttribute( 'src', $new_src );
}
}
20 changes: 12 additions & 8 deletions tests/test-amp-video-sanitizer.php
View file
Open in desktop
Expand Up @@ -40,9 +40,9 @@ public function get_data() {
'<amp-video width="300" height="300" src="https://example.com/video.mp4" layout="responsive"></amp-video>',
),

'video_with_blacklisted_attribute' => array(
'<video width="300" height="300" src="https://example.com/video.mp4" style="border-color: red;"></video>',
'<amp-video width="300" height="300" src="https://example.com/video.mp4" layout="responsive"></amp-video>',
'video_with_custom_attribute' => array(
'<video width="300" height="300" src="https://example.com/video.mp4" data-foo="bar"></video>',
'<amp-video width="300" height="300" src="https://example.com/video.mp4" data-foo="bar" layout="responsive"></amp-video>',
),

'video_with_sizes_attribute_is_overridden' => array(
Expand Down Expand Up @@ -102,11 +102,8 @@ public function get_data() {
),

'http_video_with_children' => array(
'<video width="480" height="300" poster="http://example.com/video-image.gif">
<source src="http://example.com/video.mp4" type="video/mp4">
<source src="http://example.com/video.ogv" type="video/ogg">
</video>',
'<amp-video width="480" height="300" poster="http://example.com/video-image.gif" layout="responsive"><source src="https://example.com/video.mp4" type="video/mp4"><source src="https://example.com/video.ogv" type="video/ogg"></amp-video>',
'<video width="480" height="300" poster="https://example.com/poster.jpeg"><source src="http://example.com/video.mp4" type="video/mp4"><source src="http://example.com/video.ogv" type="video/ogg"><track srclang="en" label="English" kind="subtitles" src="https://example.com/test-en.vtt" /><a href="http://example.com/video.mp4">http://example.com/video.mp4</a></video>',
'<amp-video width="480" height="300" poster="https://example.com/poster.jpeg" layout="responsive"><source src="https://example.com/video.mp4" type="video/mp4"><source src="https://example.com/video.ogv" type="video/ogg"><track srclang="en" label="English" kind="subtitles" src="https://example.com/test-en.vtt"><a href="http://example.com/video.mp4" fallback="">http://example.com/video.mp4</a></amp-video>',
),
);
}
Expand All @@ -118,6 +115,10 @@ public function test_converter( $source, $expected ) {
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Video_Sanitizer( $dom );
$sanitizer->sanitize();

$whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom );
$whitelist_sanitizer->sanitize();

$content = AMP_DOM_Utils::get_content_from_dom( $dom );
$this->assertEquals( $expected, $content );
}
Expand All @@ -132,6 +133,9 @@ public function test__https_required() {
) );
$sanitizer->sanitize();

$whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom );
$whitelist_sanitizer->sanitize();

$content = AMP_DOM_Utils::get_content_from_dom( $dom );
$this->assertEquals( $expected, $content );
}
Expand Down