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

Update embeds handlers to unwrap embeds from <p> tag #7564

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
1 change: 1 addition & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -1413,6 +1413,7 @@ function amp_get_content_embed_handlers( $post = null ) {
AMP_Scribd_Embed_Handler::class => [],
AMP_WordPress_Embed_Handler::class => [],
AMP_WordPress_TV_Embed_Handler::class => [],
AMP_Iframe_Embed_Handler::class => [],
],
$post
);
Expand Down
20 changes: 19 additions & 1 deletion includes/embeds/class-amp-base-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@
if (
$parent_node instanceof DOMElement
&&
'p' === $parent_node->tagName
Tag::P === $parent_node->tagName
&&
false === $parent_node->hasAttributes()
&&
Expand All @@ -160,6 +160,24 @@
}
}

/**
* Unwrap `<p>` element based on passed child tag name.
*
* @since 2.4.2
*
* @param Document $dom Document.
* @param string $child_tag_name Child tag name.
*/
protected function unwrap_p_element_by_child_tag_name( $dom, $child_tag_name ) {
$nodes = $dom->xpath->query( "//p/{$child_tag_name}" );

Check warning on line 172 in includes/embeds/class-amp-base-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-base-embed-handler.php#L171-L172

Added lines #L171 - L172 were not covered by tests

if ( $nodes->length && $nodes instanceof DOMNodeList ) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the order is backwards, and I don't think $nodes->length needs to be checked since the foreach will do that:

Suggested change
if ( $nodes->length && $nodes instanceof DOMNodeList ) {
if ( $nodes instanceof DOMNodeList ) {

foreach ( $nodes as $node ) {
$this->unwrap_p_element( $node );

Check warning on line 176 in includes/embeds/class-amp-base-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-base-embed-handler.php#L174-L176

Added lines #L174 - L176 were not covered by tests
}
}
}

/**
* Removes the node's nearest `<script>` sibling with a `src` attribute containing the base `src` URL provided.
*
Expand Down
2 changes: 1 addition & 1 deletion includes/embeds/class-amp-crowdsignal-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function unregister_embed() {
*/
public function filter_embed_oembed_html( $cache, $url, $attr ) {
$parsed_url = wp_parse_url( $url );
if ( empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || ! preg_match( '#(^|\.)(?P<host>polldaddy\.com|crowdsignal\.com|survey\.fm|poll\.fm)#', $parsed_url['host'], $matches ) ) {
if ( empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || ! preg_match( '#(^|\.)(?P<host>polldaddy\.com|poll\.fm|crowdsignal\.com|survey\.fm|poll\.fm)#', $parsed_url['host'], $matches ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like poll.fm is now duplicated in the regex.

return $cache;
}

Expand Down
15 changes: 15 additions & 0 deletions includes/embeds/class-amp-dailymotion-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* @package AMP
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_DailyMotion_Embed_Handler
*
Expand Down Expand Up @@ -117,6 +120,18 @@
);
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 130 in includes/embeds/class-amp-dailymotion-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-dailymotion-embed-handler.php#L130

Added line #L130 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::DAILYMOTION );

Check warning on line 132 in includes/embeds/class-amp-dailymotion-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-dailymotion-embed-handler.php#L132

Added line #L132 was not covered by tests
}

/**
* Determine the video ID from the URL.
*
Expand Down
8 changes: 2 additions & 6 deletions includes/embeds/class-amp-facebook-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* @package AMP
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
Expand Down Expand Up @@ -114,12 +115,7 @@ public function render( $args ) {
*/
public function sanitize_raw_embeds( Document $dom ) {
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$embed_nodes = $dom->xpath->query( "//p/{$this->amp_tag}" );
if ( $embed_nodes->length ) {
foreach ( $embed_nodes as $embed_node ) {
$this->unwrap_p_element( $embed_node );
}
}
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::FACEBOOK );
Copy link
Member

Choose a reason for hiding this comment

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

Might as well reuse this constant in where $this->amp_tag is set in the class above, i.e.:

private $amp_tag = Extension::FACEBOOK;


$nodes = $dom->getElementsByTagName( $this->sanitize_tag );
$num_nodes = $nodes->length;
Expand Down
15 changes: 15 additions & 0 deletions includes/embeds/class-amp-gfycat-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
* @since 1.0
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_Gfycat_Embed_Handler
*
Expand Down Expand Up @@ -79,5 +82,17 @@
}
return $return;
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 93 in includes/embeds/class-amp-gfycat-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-gfycat-embed-handler.php#L93

Added line #L93 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::GFYCAT );

Check warning on line 95 in includes/embeds/class-amp-gfycat-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-gfycat-embed-handler.php#L95

Added line #L95 was not covered by tests
}
}

47 changes: 47 additions & 0 deletions includes/embeds/class-amp-iframe-embed-handler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php
/**
* Class for handling `amp-iframe` embeds.
*
* @package AMP
* @since 2.4.1
*/

use AmpProject\Html\Tag;
use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_Iframe_Embed_Handler
*
* @since 2.4.1
*/
class AMP_Iframe_Embed_Handler extends AMP_Base_Embed_Handler {

/**
* Register embed.
*/
public function register_embed() {
// Not implemented.
}

/**
* Unregister embed.
*/
public function unregister_embed() {

Check warning on line 30 in includes/embeds/class-amp-iframe-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-iframe-embed-handler.php#L30

Added line #L30 was not covered by tests
// Not implemented.
}

Check warning on line 32 in includes/embeds/class-amp-iframe-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-iframe-embed-handler.php#L32

Added line #L32 was not covered by tests

/**
* Sanitize `amp-iframe` raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 41 in includes/embeds/class-amp-iframe-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-iframe-embed-handler.php#L41

Added line #L41 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
foreach ( [ Extension::IFRAME, Tag::IFRAME ] as $tag_name ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wary of doing this for all iframes. I think it should be targeted specifically to embeds we know about. There could legitimately be a non-embed iframe which is inside of a p, and this could break styling.

Do we need AMP_Iframe_Embed_Handler?

$this->unwrap_p_element_by_child_tag_name( $dom, $tag_name );

Check warning on line 44 in includes/embeds/class-amp-iframe-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-iframe-embed-handler.php#L43-L44

Added lines #L43 - L44 were not covered by tests
}
}
}
15 changes: 15 additions & 0 deletions includes/embeds/class-amp-imgur-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
* @since 1.0
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_Imgur_Embed_Handler
*
Expand Down Expand Up @@ -73,6 +76,18 @@
return $return;
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 86 in includes/embeds/class-amp-imgur-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-imgur-embed-handler.php#L86

Added line #L86 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::IMGUR );

Check warning on line 88 in includes/embeds/class-amp-imgur-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-imgur-embed-handler.php#L88

Added line #L88 was not covered by tests
}

/**
* Get Imgur ID from URL.
*
Expand Down
4 changes: 4 additions & 0 deletions includes/embeds/class-amp-instagram-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use AmpProject\Dom\Document;
use AmpProject\Html\Attribute;
use AmpProject\Html\Tag;
use AmpProject\Extension;

/**
* Class AMP_Instagram_Embed_Handler
Expand Down Expand Up @@ -142,6 +143,9 @@ private function get_instagram_id_from_url( $url ) {
* @param Document $dom DOM.
*/
public function sanitize_raw_embeds( Document $dom ) {
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::INSTAGRAM );

/**
* Node list.
*
Expand Down
15 changes: 15 additions & 0 deletions includes/embeds/class-amp-pinterest-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* @package AMP
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_Pinterest_Embed_Handler
*
Expand Down Expand Up @@ -89,4 +92,16 @@
]
);
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 103 in includes/embeds/class-amp-pinterest-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-pinterest-embed-handler.php#L103

Added line #L103 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::PINTEREST );

Check warning on line 105 in includes/embeds/class-amp-pinterest-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-pinterest-embed-handler.php#L105

Added line #L105 was not covered by tests
}
}
16 changes: 15 additions & 1 deletion includes/embeds/class-amp-reddit-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
* @since 0.7
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_Reddit_Embed_Handler
*
Expand Down Expand Up @@ -73,5 +76,16 @@
]
);
}
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 87 in includes/embeds/class-amp-reddit-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-reddit-embed-handler.php#L87

Added line #L87 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::EMBEDLY_CARD );

Check warning on line 89 in includes/embeds/class-amp-reddit-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-reddit-embed-handler.php#L89

Added line #L89 was not covered by tests
}
}
15 changes: 15 additions & 0 deletions includes/embeds/class-amp-soundcloud-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* @package AMP
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_SoundCloud_Embed_Handler
*
Expand Down Expand Up @@ -174,6 +177,18 @@
);
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 187 in includes/embeds/class-amp-soundcloud-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-soundcloud-embed-handler.php#L187

Added line #L187 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::SOUNDCLOUD );

Check warning on line 189 in includes/embeds/class-amp-soundcloud-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-soundcloud-embed-handler.php#L189

Added line #L189 was not covered by tests
}

/**
* Get params from Soundcloud iframe src.
*
Expand Down
1 change: 1 addition & 0 deletions includes/embeds/class-amp-tiktok-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public function sanitize_raw_embeds( Document $dom ) {

foreach ( $nodes as $node ) {
$this->make_embed_amp_compatible( $node );
$this->unwrap_p_element( $node );
}
}

Expand Down
15 changes: 15 additions & 0 deletions includes/embeds/class-amp-vimeo-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* @package AMP
*/

use AmpProject\Extension;
use AmpProject\Dom\Document;

/**
* Class AMP_Vimeo_Embed_Handler
*
Expand Down Expand Up @@ -131,6 +134,18 @@
);
}

/**
* Sanitize raw embeds.
*
* @param Document $dom Document.
*
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {

Check warning on line 144 in includes/embeds/class-amp-vimeo-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-vimeo-embed-handler.php#L144

Added line #L144 was not covered by tests
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::VIMEO );

Check warning on line 146 in includes/embeds/class-amp-vimeo-embed-handler.php

View check run for this annotation

Codecov / codecov/patch

includes/embeds/class-amp-vimeo-embed-handler.php#L146

Added line #L146 was not covered by tests
}

/**
* Determine the video ID from the URL.
*
Expand Down
2 changes: 2 additions & 0 deletions includes/embeds/class-amp-youtube-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ public function render( $html, $url, $video_id ) {
* @return void
*/
public function sanitize_raw_embeds( Document $dom ) {
// If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them.
$this->unwrap_p_element_by_child_tag_name( $dom, Extension::YOUTUBE );

$query_segments = array_map(
static function ( $domain ) {
Expand Down