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

Preserve seek time and other parameters to amp-youtube from YouTube embeds #6423

Merged
merged 43 commits into from
Sep 9, 2021

Conversation

dhaval-parekh
Copy link
Collaborator

Summary

Fixes #4518

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2021

Plugin builds for da36d2d are ready 🛎️!

@westonruter
Copy link
Member

There are additional parameters which the embed API supports: https://developers.google.com/youtube/player_parameters

For example, if you add a “raw embed” via the Custom HTML block with:

<iframe width="560" height="315" src="https://www.youtube.com/embed/0zM3nApSvMg?controls=0&amp;autoplay=1" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>

This should result in an amp-youtube element being added with data-param-controls="0" and data-param-autoplay="1".

I think any query parameters on the YouTube embed URL should be copied to data-pram-* attributes: https://amp.dev/documentation/components/amp-youtube/#data-param-*

For the YouTube Embed block specifically, it appears that only the start parameter is passed along in oEmbed requests. So this is only relevant to YouTube raw embeds.

Humm, but actually in looking at AMP_YouTube_Embed_Handler it doesn't actually implement the sanitize_raw_embeds method so these iframe video embeds are being passed through as amp-iframe elements instead of amp-youtube. So they work right now, but I think it would be better if we implemented sanitize_raw_embeds to convert these into amp-youtube.

@westonruter
Copy link
Member

cc @pierlon for his thoughts on this since he's done a lot of work with embeds in the past

@dhaval-parekh dhaval-parekh force-pushed the bug/4518-amp-youtube-component branch from 463ceb7 to b477337 Compare June 29, 2021 13:57
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @dhaval-parekh.

Left a few comments here that should help move this PR forward.

includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
@dhaval-parekh dhaval-parekh force-pushed the bug/4518-amp-youtube-component branch from 7a31e69 to cf81afa Compare July 1, 2021 07:11
@dhaval-parekh dhaval-parekh requested a review from pierlon July 1, 2021 15:25
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Looking good so far! Just a few comments and suggestions to be made.

includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved

preg_match( $regex, $parsed_url['fragment'], $matches );

if ( ! empty( $matches ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternatively (to make sure the match groups do indeed exist):

Suggested change
if ( ! empty( $matches ) ) {
if ( isset( $matches['minutes'], $matches['seconds'] ) ) {

includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
*/
public function process_embed( Document $dom, $html, $url ) {

$id = $this->get_video_id_from_url( $url );
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized the AMP component can also [accept a YouTube channel id](Youtube channel id), so we need to provide support for that as well. So if there is no video ID, then check for a live channel ID.

From the docs:

For example, in this URL: https://www.youtube.com/embed/live_stream?channel=UCB8Kb4pxYzsDsHxzBfnid4Q, UCB8Kb4pxYzsDsHxzBfnid4Q is the channel id. You can provide a data-live-channelid instead of a data-videoid attribute to embed a stable url for a live stream instead of a video.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pierlon I have added support for live streaming channels for iframe added as custom HTML markup.
However, WordPress by default does not provide support for embedding YouTube URLs (It will just render as text)

Copy link
Contributor

Choose a reason for hiding this comment

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

If WordPress never supported it I think it's safe to say we don't have to either.

@pierlon I have added support for live streaming channels for iframe added as custom HTML markup.

I tried this but it parsed an incorrect video ID. Eg:

<iframe src="https://www.youtube.com/embed/live_stream?channel=12345" allowfullscreen="" width="560" height="315"></iframe>

Gives:

<amp-youtube data-videoid="live_stream" layout="responsive" width="560" height="315" data-param-channel="12345" ...</amp-youtube>

Instead, the data-videoid attribute should not have been set, nor the data-param-channel attribute. Perhaps we could add a condition to say if the parsed video ID is live_stream then it's invalid and bail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have addressed this issue.

includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
Comment on lines 127 to 134
$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 ];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved to the prepare_attributes() method as it seems more relevant there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These three attributes parse differently in render() (from HTML markup) and in get_amp_component() (from DOMElement). That is a reason it's parse saperatlly.

includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
@dhaval-parekh dhaval-parekh force-pushed the bug/4518-amp-youtube-component branch from d9faed3 to 9728368 Compare July 9, 2021 11:58
@dhaval-parekh dhaval-parekh self-assigned this Jul 21, 2021
@dhaval-parekh dhaval-parekh force-pushed the bug/4518-amp-youtube-component branch from 9728368 to 8c97e14 Compare July 21, 2021 10:25
@dhaval-parekh dhaval-parekh requested a review from pierlon July 21, 2021 10:25
includes/embeds/class-amp-youtube-embed-handler.php Outdated Show resolved Hide resolved
$attributes
);

if ( ! empty( $amp_node ) && is_a( $amp_node, '\DOMElement' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( ! empty( $amp_node ) && is_a( $amp_node, '\DOMElement' ) ) {
if ( ! empty( $amp_node ) && $amp_node instanceof Element ) {

Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work on this @dhaval-parekh, passing this on to @westonruter for final review.

@pierlon pierlon added this to the v2.2 milestone Sep 2, 2021
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@dhaval-parekh A few tests are failing. It seems just the expected link in the text data needs to be updated.

continue;
}

$attributes[ sanitize_key( "data-param-$key" ) ] = esc_attr( $value );
Copy link
Member

Choose a reason for hiding this comment

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

Escaping shouldn't be done here because the returned attributes array may be sent into the DOM directly, in which case the escaping will be duplicated.

Suggested change
$attributes[ sanitize_key( "data-param-$key" ) ] = esc_attr( $value );
$attributes[ sanitize_key( "data-param-$key" ) ] = $value;

dhaval-parekh and others added 3 commits September 9, 2021 14:53
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
@westonruter
Copy link
Member

@dhaval-parekh The phpstan ignores that you added caused errors for me locally in the same way as they caused build failures on GHA. So I reverted them in 3ecad1e. It may be that you have a stale copy of phpstan. I have v0.12.76 (humm, which is also stale). I thought that composer clear-cache && composer install would get the latest version, but it didn't work for me. So you can do this instead: rm vendor/bin/phpstan && composer install.

@westonruter
Copy link
Member

Er, when I upgrade to 0.12.98 then I get errors:

 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   includes/embeds/class-amp-twitter-embed-handler.php
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  24     PHPDoc type int|string of property AMP_Twitter_Embed_Handler::$DEFAULT_WIDTH is not covariant with PHPDoc type int of overridden property AMP_Base_Embed_Handler::$DEFAULT_WIDTH.
         💡 You can fix 3rd party PHPDoc types with stub files:
            https://phpstan.org/user-guide/stub-files
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

 ------ ---------------------------------------------------
  Line   src/BackgroundTask/MonitorCssTransientCaching.php
 ------ ---------------------------------------------------
  112    No error to ignore is reported on line 112.
 ------ ---------------------------------------------------

So apparently GitHub's copy is stale and you were on the latest version.

@pierlon
Copy link
Contributor

pierlon commented Sep 9, 2021

@westonruter As an alternative, might I suggest installing PHPStan from the shivammathur/setup-php GHA? Not providing a version for phpstan via that GHA should download the latest version. It should look something like:

diff --git a/.github/workflows/build-test-measure.yml b/.github/workflows/build-test-measure.yml
--- a/.github/workflows/build-test-measure.yml	(revision 08017ae8e1da05ae8e445e51807610e4e5365599)
+++ b/.github/workflows/build-test-measure.yml	(date 1631210205414)
@@ -214,6 +214,7 @@
           # 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
@@ -231,7 +232,7 @@
         run: composer install
 
       - name: Static Analysis (PHPStan)
-        run: vendor/bin/phpstan analyse
+        run: phpstan analyse
 
 #-----------------------------------------------------------------------------------------------------------------------
 

@westonruter westonruter changed the title Transfer seek time to AMP youtube component from youtube URL. Preserve seek time and other parameters to amp-youtube from YouTube embeds Sep 9, 2021
@westonruter
Copy link
Member

💥 image

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Whew! This was a deceptively complicated one!

@westonruter westonruter merged commit deba08a into develop Sep 9, 2021
@westonruter westonruter deleted the bug/4518-amp-youtube-component branch September 9, 2021 22:25
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Embeds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameters in a YouTube URL are not transferred to the amp-youtube component
3 participants