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.

Sign up for GitHub

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

Remove registration of Facebook embed handler #4384

Merged
merged 7 commits into from
Mar 17, 2020
Merged

Remove registration of Facebook embed handler #4384

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dca629e
Remove registration of Facebook embed handler
pierlon Mar 14, 2020
6075007
Use `nodeName` property
pierlon Mar 16, 2020
77981c7
Remove phpstan.neon.dist from build
westonruter Mar 16, 2020
30d174d
Eliminate unnecessary double loop for XPath query since DOMNodeList i…
westonruter Mar 16, 2020
c6869a9
Test removal of <p> tag
pierlon Mar 16, 2020
ca45c7a
Remove multiple instances of div#fb-root
pierlon Mar 16, 2020
fe545a5
Make phpstan happy
pierlon Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 9 additions & 55 deletions includes/embeds/class-amp-facebook-embed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,6 @@
* Class AMP_Facebook_Embed_Handler
*/
class AMP_Facebook_Embed_Handler extends AMP_Base_Embed_Handler {
const URL_PATTERN = '#https?://(www\.)?facebook\.com/.*#i';

/**
* Default width.
*
* @var int
*/
protected $DEFAULT_WIDTH = 600;

/**
* Default height.
*
Expand All @@ -45,57 +36,14 @@ class AMP_Facebook_Embed_Handler extends AMP_Base_Embed_Handler {
* Registers embed.
*/
public function register_embed() {
wp_embed_register_handler( $this->amp_tag, self::URL_PATTERN, [ $this, 'oembed' ], -1 );
// Not implemented.
}

/**
* Unregisters embed.
*/
public function unregister_embed() {
wp_embed_unregister_handler( $this->amp_tag, -1 );
}

/**
* WordPress OEmbed rendering callback.
*
* @param array $matches URL pattern matches.
* @param array $attr Matched attributes.
* @param string $url Matched URL.
* @return string HTML markup for rendered embed.
*/
public function oembed( $matches, $attr, $url ) {
return $this->render( [ 'url' => $url ] );
}

/**
* Gets the rendered embed markup.
*
* @param array $args Embed rendering arguments.
* @return string HTML markup for rendered embed.
*/
public function render( $args ) {
$args = wp_parse_args(
$args,
[
'url' => false,
]
);

if ( empty( $args['url'] ) ) {
return '';
}

$this->did_convert_elements = true;

return AMP_HTML_Utils::build_tag(
$this->amp_tag,
[
'data-href' => $args['url'],
'layout' => 'responsive',
'width' => $this->args['width'],
'height' => $this->args['height'],
]
);
// Not implemented.
}

/**
Expand Down Expand Up @@ -136,7 +84,13 @@ public function sanitize_raw_embeds( Document $dom ) {
$scripts[] = $script;
}
foreach ( $scripts as $script ) {
$script->parentNode->removeChild( $script );
$parent_node = $script->parentNode;
$parent_node->removeChild( $script );

// Remove parent node if it is an empty <p> tag.
if ( 'p' === $parent_node->tagName && null === $parent_node->firstChild ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script tag is wrapped with a p tag (from wpautop()), so I've removed the lingering empty tag if it's there.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( 'p' === $parent_node->tagName && null === $parent_node->firstChild ) {
if ( 'p' === $parent_node->nodeName && null === $parent_node->firstChild ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its also an issue for TikTok, Twitter and Instagram.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking at some of the other embeds, they are for the most part also wrapped with a <p> tag. That isn't an issue per se, but if this PR is merged we should probably standardize the other embeds to not be wrapped with a paragraph element.

$parent_node->parentNode->removeChild( $parent_node );
}
}
$fb_root->parentNode->removeChild( $fb_root );
Copy link
Member

Choose a reason for hiding this comment

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

It turns out that when you have multiple Facebook embeds on a page, each instance will include a div#fb-root. So I suppose this should be changed to:

Suggested change
$fb_root->parentNode->removeChild( $fb_root );
while ( $fb_root ) {
$fb_root->parentNode->removeChild( $fb_root );
$fb_root = $dom->getElementById( 'fb-root' );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory this should work, but when it retrieves the #fb_root div again it's returning the same div as before. I'll adapt this to use an XPath query instead.

Copy link
Member

Choose a reason for hiding this comment

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

Why would it return the same div as before if it was removed from the document?

}
Expand Down
153 changes: 82 additions & 71 deletions tests/php/test-amp-facebook-embed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,60 +12,6 @@
*/
class AMP_Facebook_Embed_Test extends WP_UnitTestCase {

/**
* Data provider for test__conversion.
*
* @return array Data.
*/
public function get_conversion_data() {
return [
'no_embed' => [
'<p>Hello world.</p>',
'<p>Hello world.</p>' . PHP_EOL,
],
'simple_url_https' => [
'https://www.facebook.com/zuck/posts/10102593740125791' . PHP_EOL,
'<p><amp-facebook data-href="https://www.facebook.com/zuck/posts/10102593740125791" layout="responsive" width="600" height="400"></amp-facebook></p>' . PHP_EOL,
],
'simple_url_http' => [
'http://www.facebook.com/zuck/posts/10102593740125791' . PHP_EOL,
'<p><amp-facebook data-href="http://www.facebook.com/zuck/posts/10102593740125791" layout="responsive" width="600" height="400"></amp-facebook></p>' . PHP_EOL,
],
'no_dubdubdub' => [
'https://facebook.com/zuck/posts/10102593740125791' . PHP_EOL,
'<p><amp-facebook data-href="https://facebook.com/zuck/posts/10102593740125791" layout="responsive" width="600" height="400"></amp-facebook></p>' . PHP_EOL,
],
'notes_url' => [
'https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/' . PHP_EOL,
'<p><amp-facebook data-href="https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/" layout="responsive" width="600" height="400"></amp-facebook></p>' . PHP_EOL,
],
'photo_url' => [
'https://www.facebook.com/photo.php?fbid=10102533316889441&set=a.529237706231.2034669.4&type=3&theater' . PHP_EOL,
'<p><amp-facebook data-href="https://www.facebook.com/photo.php?fbid=10102533316889441&amp;set=a.529237706231.2034669.4&amp;type=3&amp;theater" layout="responsive" width="600" height="400"></amp-facebook></p>' . PHP_EOL,
],
'notes_url2' => [
'https://www.facebook.com/zuck/videos/10102509264909801/' . PHP_EOL,
'<p><amp-facebook data-href="https://www.facebook.com/zuck/videos/10102509264909801/" layout="responsive" width="600" height="400"></amp-facebook></p>' . PHP_EOL,
],

];
}

/**
* Test conversion.
*
* @dataProvider get_conversion_data
* @param string $source Source.
* @param string $expected Expected.
*/
public function test__conversion( $source, $expected ) {
$embed = new AMP_Facebook_Embed_Handler();
$embed->register_embed();
$filtered_content = apply_filters( 'the_content', $source );

$this->assertEqualMarkup( $expected, $filtered_content );
}

/**
* Get scripts data.
*
Expand Down Expand Up @@ -94,9 +40,12 @@ public function get_scripts_data() {
public function test__get_scripts( $source, $expected ) {
$embed = new AMP_Facebook_Embed_Handler();
$embed->register_embed();
$source = apply_filters( 'the_content', $source );

$whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( AMP_DOM_Utils::get_dom_from_content( $source ) );
$filtered_content = apply_filters( 'the_content', $source );
$dom = AMP_DOM_Utils::get_dom_from_content( $filtered_content );
$embed->sanitize_raw_embeds( $dom );

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

$scripts = array_merge(
Expand All @@ -114,21 +63,79 @@ public function test__get_scripts( $source, $expected ) {
*/
public function get_raw_embed_dataset() {
return [
'no_embed_blockquote' => [
'no_embed_blockquote' => [
'<p>Hello world.</p>',
'<p>Hello world.</p>',
],
'div_without_instagram' => [

'div_without_facebook_embed' => [
'<div>lorem ipsum</div>',
'<div>lorem ipsum</div>',
],

'post_embed' => [
'simple_url_https' => [
'https://www.facebook.com/zuck/posts/10102593740125791' . PHP_EOL,
'
<amp-facebook width="500" height="400" data-href="https://www.facebook.com/zuck/posts/10102593740125791" data-embed-as="post" layout="responsive">
<blockquote cite="https://www.facebook.com/zuck/posts/10102593740125791" class="fb-xfbml-parse-ignore" fallback="">
<p>February 4 is Facebook’s 12th birthday!</p>
<p>Our anniversary has a lot of meaning to me as an opportunity to reflect on how…</p>
<p>Posted by <a href="https://www.facebook.com/zuck">Mark Zuckerberg</a> on <a href="https://www.facebook.com/zuck/posts/10102593740125791">Tuesday, January 12, 2016</a></p>
</blockquote>
</amp-facebook>
' . PHP_EOL,
],

'notes_url' => [
'https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/' . PHP_EOL,
'
<amp-facebook width="500" height="400" data-href="https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/" data-embed-as="post" layout="responsive">
<blockquote cite="https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/" class="fb-xfbml-parse-ignore" fallback="">
<p>Posted by <a href="https://www.facebook.com/Engineering/">Facebook Engineering</a> on <a href="https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/">Saturday, December 8, 2012</a></p>
</blockquote>
</amp-facebook>
' . PHP_EOL,
],

'photo_url' => [
'https://www.facebook.com/photo.php?fbid=10102533316889441&set=a.529237706231.2034669.4&type=3&theater' . PHP_EOL,
'
<amp-facebook width="500" height="400" data-href="https://www.facebook.com/photo.php?fbid=10102533316889441&amp;set=a.529237706231.2034669.4&amp;type=3&amp;theater" data-embed-as="post" layout="responsive">
<blockquote cite="https://www.facebook.com/photo.php?fbid=10102533316889441&amp;set=a.529237706231&amp;type=3" class="fb-xfbml-parse-ignore" fallback="">
<p>Meanwhile, Beast turned to the dark side.</p>
<p>Posted by <a href="https://www.facebook.com/zuck">Mark Zuckerberg</a> on <a href="https://www.facebook.com/photo.php?fbid=10102533316889441&amp;set=a.529237706231&amp;type=3">Friday, December 18, 2015</a></p>
</blockquote>
</amp-facebook>
' . PHP_EOL,
],

'video_url' => [
'https://www.facebook.com/zuck/videos/10102509264909801/' . PHP_EOL,
'
<amp-facebook width="500" height="400" data-href="https://www.facebook.com/zuck/videos/10102509264909801/" data-embed-as="video" layout="responsive">
<blockquote cite="https://www.facebook.com/zuck/videos/10102509264909801/" class="fb-xfbml-parse-ignore" fallback="">
<p><a href="https://www.facebook.com/zuck/videos/10102509264909801/"></a></p>
<p>I want to share a few more thoughts on the Chan Zuckerberg Initiative before I just start posting photos of me and Max for a while 🙂</p>
<p>I hope one idea comes through: that we as a society should make investments now to ensure this world is much better for the next generation.</p>
<p>I don\'t think we do enough of this right now. </p>
<p>Sure, there are many areas where investment now will solve problems for today and also improve the world for the future. We do muster the will to solve some of those.</p>
<p>But for the problems that will truly take decades of investment before we see any major return, we are dramatically underinvested.</p>
<p>One example is basic science research to cure disease. Another is developing clean energy to protect the world for the future. Another is the slow and steady improvement to modernize schools. Others are systematic issues around poverty and justice. There is a long list of these opportunities.</p>
<p>The role of philanthropy is to invest in important areas that companies and governments aren\'t funding — either because they may not be profitable for companies or because they are too long term for people to want to invest now.</p>
<p>In the case of disease, basic research often needs to be funded before biotech or pharma companies can create drugs to help people. If we invest more in science, we can make faster progress towards curing disease.</p>
<p>Our investment in the Chan Zuckerberg Initiative is small compared to what the world can invest in solving these great challenges. My hope is that our work inspires more people to invest in these longer term issues. If we can do that, then we can all really make a difference together.</p>
<p>Posted by <a href="https://www.facebook.com/zuck">Mark Zuckerberg</a> on Friday, December 4, 2015</p>
</blockquote>
</amp-facebook>
' . PHP_EOL,
],

'post_embed' => [
'<div class="fb-post" data-href="https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/"></div>',
'<amp-facebook width="600" height="400" data-href="https://www.facebook.com/notes/facebook-engineering/under-the-hood-the-javascript-sdk-truly-asynchronous-loading/10151176218703920/" data-embed-as="post" layout="responsive"></amp-facebook>',
],

'post_with_fallbacks' => [
'post_with_fallbacks' => [
'
<div class="fb-post" data-href="https://www.facebook.com/20531316728/posts/10154009990506729/" data-width="500" data-show-text="true">
<blockquote cite="https://developers.facebook.com/20531316728/posts/10154009990506729/" class="fb-xfbml-parse-ignore">
Expand All @@ -139,18 +146,18 @@ public function get_raw_embed_dataset() {
'
<amp-facebook width="500" height="400" data-href="https://www.facebook.com/20531316728/posts/10154009990506729/" data-show-text="true" data-embed-as="post" layout="responsive">
<blockquote cite="https://developers.facebook.com/20531316728/posts/10154009990506729/" class="fb-xfbml-parse-ignore" fallback="">
Posted by <a href="https://www.facebook.com/facebook/">Facebook</a> on <a href="https://developers.facebook.com/20531316728/posts/10154009990506729/">Thursday, August 27, 2015</a>
<p> Posted by <a href="https://www.facebook.com/facebook/">Facebook</a> on <a href="https://developers.facebook.com/20531316728/posts/10154009990506729/">Thursday, August 27, 2015</a></p>
</blockquote>
</amp-facebook>
',
],

'video_embed' => [
'video_embed' => [
'<div class="fb-video" data-href="https://www.facebook.com/amanda.orr.56/videos/10212156330049017/" data-show-text="false"></div>',
'<amp-facebook width="600" height="400" data-href="https://www.facebook.com/amanda.orr.56/videos/10212156330049017/" data-show-text="false" data-embed-as="video" layout="responsive"></amp-facebook>',
],

'page_embed' => [
'page_embed' => [
'
<div class="fb-page" data-href="https://www.facebook.com/xwp.co/" data-width="340" data-height="432" data-hide-cover="true" data-show-facepile="true" data-show-posts="false">
<div class="fb-xfbml-parse-ignore">
Expand All @@ -161,13 +168,15 @@ public function get_raw_embed_dataset() {
'
<amp-facebook-page width="340" height="432" data-href="https://www.facebook.com/xwp.co/" data-hide-cover="true" data-show-facepile="true" data-show-posts="false" layout="responsive">
<div class="fb-xfbml-parse-ignore" fallback="">
<blockquote cite="https://www.facebook.com/xwp.co/"><a href="https://www.facebook.com/xwp.co/">Like Us</a></blockquote>
<blockquote cite="https://www.facebook.com/xwp.co/">
<p><a href="https://www.facebook.com/xwp.co/">Like Us</a></p>
</blockquote>
</div>
</amp-facebook-page>
',
],

'like' => [
'like' => [
'
<div class="fb-like" data-href="https://developers.facebook.com/docs/plugins/" data-width="400" data-layout="standard" data-action="like" data-size="small" data-show-faces="true" data-share="true"></div>
',
Expand All @@ -177,35 +186,35 @@ public function get_raw_embed_dataset() {
',
],

'comments' => [
'comments' => [
'
<div class="fb-comments" data-href="https://developers.facebook.com/docs/plugins/comments#configurator" data-numposts="5"></div>
',
'<amp-facebook-comments width="600" height="400" data-href="https://developers.facebook.com/docs/plugins/comments#configurator" data-numposts="5" layout="responsive"></amp-facebook-comments>',
],

'comments_full_width' => [
'comments_full_width' => [
'
<div class="fb-comments" data-href="https://developers.facebook.com/docs/plugins/comments#configurator" data-width="100%" data-numposts="5"></div>
',
'<amp-facebook-comments width="auto" height="400" data-href="https://developers.facebook.com/docs/plugins/comments#configurator" data-numposts="5" layout="fixed-height"></amp-facebook-comments>',
],

'comments_full_width_2' => [
'comments_full_width_2' => [
'
<div class="fb-comments" data-href="https://developers.facebook.com/docs/plugins/comments#configurator" data-height="123" data-width="100%" data-numposts="5"></div>
',
'<amp-facebook-comments width="auto" height="123" data-href="https://developers.facebook.com/docs/plugins/comments#configurator" data-numposts="5" layout="fixed-height"></amp-facebook-comments>',
],

'comment_embed' => [
'comment_embed' => [
'
<div class="fb-comment-embed" data-href="https://www.facebook.com/zuck/posts/10102735452532991?comment_id=1070233703036185" data-width="500"></div>
',
'<amp-facebook width="500" height="400" data-href="https://www.facebook.com/zuck/posts/10102735452532991?comment_id=1070233703036185" data-embed-as="comment" layout="responsive"></amp-facebook>',
],

'remove_fb_root' => [
'remove_fb_root' => [
'<div id="fb-root"></div><script async defer crossorigin="anonymous" src="https://connect.facebook.net/en_US/sdk.js#xfbml=1&version=v3.2"></script>', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
'',
],
Expand All @@ -221,9 +230,11 @@ public function get_raw_embed_dataset() {
* @covers AMP_Facebook_Embed_Handler::sanitize_raw_embeds()
*/
public function test__raw_embed_sanitizer( $source, $expected ) {
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$embed = new AMP_Facebook_Embed_Handler();
$embed->register_embed();

$filtered_content = apply_filters( 'the_content', $source );
$dom = AMP_DOM_Utils::get_dom_from_content( $filtered_content );
$embed->sanitize_raw_embeds( $dom );

$layout_sanitizer = new AMP_Layout_Sanitizer( $dom );
Expand Down