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

Add embed handler for gfycat. #1136

Merged
merged 6 commits into from
May 19, 2018
Merged
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
@@ -447,6 +447,7 @@ function amp_get_content_embed_handlers( $post = null ) {
'AMP_Reddit_Embed_Handler' => array(),
'AMP_Tumblr_Embed_Handler' => array(),
'AMP_Gallery_Embed_Handler' => array(),
'AMP_Gfycat_Embed_Handler' => array(),
'WPCOM_AMP_Polldaddy_Embed' => array(),
),
$post
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
@@ -39,6 +39,7 @@ class AMP_Autoloader {
'AMP_DailyMotion_Embed_Handler' => 'includes/embeds/class-amp-dailymotion-embed',
'AMP_Facebook_Embed_Handler' => 'includes/embeds/class-amp-facebook-embed',
'AMP_Gallery_Embed_Handler' => 'includes/embeds/class-amp-gallery-embed',
'AMP_Gfycat_Embed_Handler' => 'includes/embeds/class-amp-gfycat-embed-handler',
'AMP_Core_Block_Handler' => 'includes/embeds/class-amp-core-block-handler',
'AMP_Instagram_Embed_Handler' => 'includes/embeds/class-amp-instagram-embed',
'AMP_Issuu_Embed_Handler' => 'includes/embeds/class-amp-issuu-embed-handler',
72 changes: 72 additions & 0 deletions includes/embeds/class-amp-gfycat-embed-handler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php
/**
* Class AMP_Gfycat_Embed_Handler
*
* @package AMP
* @since 1.0
*/

/**
* Class AMP_Gfycat_Embed_Handler
*/
class AMP_Gfycat_Embed_Handler extends AMP_Base_Embed_Handler {
/**
* Regex matched to produce output amp-gfycat.
*
* @const string
Copy link
Contributor

Choose a reason for hiding this comment

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

Strangely, even constants should have a @var tag. I just recently learned this. But it's not a blocker.

*/
const URL_PATTERN = '#https?://(www\.)?gfycat\.com/gifs/detail/.*#i';

/**
* Register embed.
*/
public function register_embed() {
add_filter( 'embed_oembed_html', array( $this, 'filter_embed_oembed_html' ), 10, 3 );
}

/**
* Unregister embed.
*/
public function unregister_embed() {
remove_filter( 'embed_oembed_html', array( $this, 'filter_embed_oembed_html' ), 10 );
}

/**
* Filter oEmbed HTML for Meetup to prepare it for AMP.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be for Gfycat?

*
* @param mixed $return The shortcode callback function to call.
* @param string $url The attempted embed URL.
* @param array $attr An array of shortcode attributes.
* @return string Embed.
*/
public function filter_embed_oembed_html( $return, $url, $attr ) {
$parsed_url = wp_parse_url( $url );
if ( false !== strpos( $parsed_url['host'], 'gfycat.com' ) ) {
if ( preg_match( '/width=(?:"|\')(\d+)(?:"|\')/', $return, $matches ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Might want to adjust this to be like '/width=["\']?(\d+)/' since the quote marks would be optional in HTML.

$attr['width'] = $matches[1];
}
if ( preg_match( '/height=(?:"|\')(\d+)(?:"|\')/', $return, $matches ) ) {
$attr['height'] = $matches[1];
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this bail if the width and height are empty? In that case we wouldn't be able to build a valid amp-gfycat since the dimensions wouldn't be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added checking for height within e1d29a8 and using fixed-height for layout if only width is missing.


$pieces = explode( '/detail/', $parsed_url['path'] );
Copy link
Member

Choose a reason for hiding this comment

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

This isn't taking into account the oEmbed URLs like https://gfycat.com/webbedbossycollardlizard

if ( ! isset( $pieces[1] ) ) {
return $return;
}

$data_gfyid = $pieces[1];

$return = AMP_HTML_Utils::build_tag(
'amp-gfycat',
array(
'width' => $attr['width'],
'height' => $attr['height'],
'data-gfyid' => $data_gfyid,
'layout' => 'responsive',
Copy link
Contributor

@kienstra kienstra May 9, 2018

Choose a reason for hiding this comment

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

How about a layout of intrinsic here? That layout is like responsive, in that it expands to the size of its container.

But it always has at least the dimensions in the width and height.

When left or right-aligning the Embed block, this <amp-gfycat> gets wrapped in a <figure>.

right-aligned-gutenberg

But that <figure> doesn't look to have width or height of its own, so the layout="responsive" value causes the <amp-gfycat> to not appear.

With layout="intrinsic", the element appears:

right-aligned-front-end

We're already using layout="intrinsic" as defaults for <amp-img> and <amp-iframe>

)
);
}
return $return;
}
}

94 changes: 94 additions & 0 deletions tests/test-class-amp-gfycat-embed-handler.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
<?php
/**
* Test Gfycat embed.
*
* @package AMP.
*/

/**
* Class AMP_Gfycat_Embed_Test
*/
class AMP_Gfycat_Embed_Test extends WP_UnitTestCase {

/**
* Get conversion data.
*
* @return array
*/
public function get_conversion_data() {
return array(
'no_embed' => array(
'<p>Hello world.</p>',
'<p>Hello world.</p>' . PHP_EOL,
),

'url_simple' => array(
'https://gfycat.com/gifs/detail/tautwhoppingcougar' . PHP_EOL,
'<p><amp-gfycat width="500" height="750" data-gfyid="tautwhoppingcougar" layout="responsive"></amp-gfycat></p>' . PHP_EOL,
),

);
}

/**
* Test conversion.
*
* @param string $source Source.
* @param string $expected Expected.
* @dataProvider get_conversion_data
*/
public function test__conversion( $source, $expected ) {
if ( ( version_compare( get_bloginfo( 'version' ), '4.9', '<' ) ) ) {
$this->markTestSkipped( 'Gfycat is not supported in this WP version.' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added skipping tests since it looked like amp-iframe was not created in WP 4.8 and WP 4.7 and some tests failed, however, after testing locally, not sure it's related to WP version. Thought that perhaps the tests failed due to PHP version. Any thoughts on what might fail, could wp_parse_url used here differ in previous versions?

Copy link
Contributor

@kienstra kienstra May 10, 2018

Choose a reason for hiding this comment

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

Hi @miina, good question.

Could you try adding this part of the setUp method from AMP_SoundCloud_Embed_Test to AMP_Gfycat_Embed_Test?

That worked in the testing PR #1138 that I opened, even without $this->markTestSkipped().

The failed build that you linked to used WordPress version 4.7. When I set my local to that version, the test here failed (without any modification). Strangely, the code works fine on the front-end. Just not in the tests in that case.

}
$embed = new AMP_Gfycat_Embed_Handler();
$embed->register_embed();
$filtered_content = apply_filters( 'the_content', $source );

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

/**
* Get scripts data.
*
* @return array
*/
public function get_scripts_data() {
return array(
'not_converted' => array(
'<p>Hello World.</p>',
array(),
),
'converted' => array(
'https://www.gfycat.com/gifs/detail/tautwhoppingcougar' . PHP_EOL,
array( 'amp-gfycat' => true ),
),
);
}

/**
* Test scripts.
*
* @param string $source Source.
* @param string $expected Expected.
* @dataProvider get_scripts_data
*/
public function test__get_scripts( $source, $expected ) {
if ( ( version_compare( get_bloginfo( 'version' ), '4.9', '<' ) ) ) {
$this->markTestSkipped( 'Gfycat is not supported in this WP version.' );
}
$embed = new AMP_Gfycat_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 ) );
$whitelist_sanitizer->sanitize();

$scripts = array_merge(
$embed->get_scripts(),
$whitelist_sanitizer->get_scripts()
);

$this->assertEquals( $expected, $scripts );
}
}