Problem/Motivation

By default, embedded YouTube videos display related videos based on the user's viewing history. Adding rel=0 to the iframe would adjust this to show only related videos from the same channel as the embed. However, there is no way to do this easily with the core oEmbed formatter; it requires weird hacks like modifying media-oembed-iframe.html.twig to force ?rel=0 into the URL, like so:

{#
/**
 * @file
 * Default theme implementation to display an oEmbed resource in an iframe.
 *
 * @ingroup themeable
 */
#}
<!DOCTYPE html>
<html>
<head>
  <style>
    iframe {
      position: absolute;
      left: 0;
      top: 0;
      right: 0;
      bottom: 0;
      margin: 0;
      width: 100%;
      height: 100%;
    }
  </style>
</head>
<body style="margin: 0">
{{ media|replace({ "?feature=oembed": "?rel=0&feature=oembed"  })|raw }}
</body>
</html>

Proposed resolution

Adding provider-specific configuration options to the oEmbed formatter would be ideal, but that is overly complicated and probably more in the contrib realm.

For the time being, it would be best to at least pass the oEmbed resource value object to the theme hook for the oEmbed iframe, so that extensions can implement preprocess functions and take appropriate action if needed. That would at least make it easier to implement useful provider-specific behavior in the iframe.

Remaining tasks

  • ✅ Agree on an approach
  • ✅ Create a patch (implemented in #13)
  • ✅ Write tests
  • Commit it!

User interface changes

None.

API changes

An existing theme hook will receive an additional new variable.

Data model changes

None.

Release notes snippet

TBD

Issue fork drupal-3009003

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vood002 created an issue. See original summary.

kirkkala’s picture

Status: Active » Needs review
FileSize
28.41 KB
7.34 KB

Here is a solution that adds "Remote video embed options" and the 'rel' setting of youtube videos configurable at /admin/config/media/media-settings. This way we would apply the setting site-wide, not as per field instance as suggested display option would have done.

Media setting for related youtube videos

PS. Youtube has changed a bit recently not allowing to remove the related videos completely with rel=0. See more about the change at https://developers.google.com/youtube/player_parameters#release_notes_08...

BrankoC’s picture

If you want responsive videos, you will need to wrap your iframe in another element. The CSS spec defines an iframe to have a native width of 300px and a ratio of 2:1 (i.e. a native height of 150 pixels).

The usual work-around for this is a wrapper element with a padding that creates a space for the iframe to be 100% in, for example:

.videowrapper {
  display: block;
  position: relative;
  padding-bottom: 56.25%; /* Default to 16:9 aspect ratio. */
}

In a Wordpress filter I wrote, I use the oEmbed values for width and height to determine the ratio of a video, then add this ratio in a classname to the video wrapper element. This allows me to change the padding-bottom from the example above for a number of common ratios (2:1, 16:9, 4:3 and so on).

MrMason’s picture

Status: Needs review » Reviewed & tested by the community

I've tested the existing patch and while it works do we really want the change for showing related videos to be site wide or do we want them to be set in individual videos?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This seems like a useful feature. I'm not sure about the implementation though. I think this should be configured via \Drupal\media_entity_embeddable_video\Plugin\MediaEntity\VideoProvider\YouTube

Also some feedback from the Media maintainers would be great.

seanB’s picture

Core currently supports oEmbed, and has a provider for Youtube and Vimeo enabled by default. The oEmbed spec unfortunately doesn't allow extra parameters to be set. While I do see a need for additional settings, there are multiple ways to solve this. And since the spec doesn't support this, it all feels a little hacky.

The list of things that might need to be altered in the output could be endless, since there are a lot of oEmbed providers out there. Adding all these settings to the media type is probably not the most flexible way to handle this.

We could add settings pages per provider, but I was thinking we might also be able to add the settings to a formatter. This would also be a little more flexible since you can change the settings per view mode. We then need to figure out a way to let modules define which settings are available for each provider, and let the providers change the output based on these settings. oEmbed provides a HTML string in most cases, so changing that HTML is probably going to be the most hacky part of all.

The hacky nature of this worries me a bit though.

phenaproxima’s picture

Issue tags: +oembed
pameeela’s picture

+1 for this! Agree it's not the best approach, but as someone with a lot of government clients (who often freak out when they see what comes up!) this is a must so we have to do it one way or another. Support for configuration would be ideal.

lunk rat’s picture

I would go so far as to recommend that related videos be disabled by default. I argue that it is likely that a majority of the use-cases for remote video on Drupal sites want only a single video available in each iFrame. So implement a patch to hardcode-disable related videos by default (all providers), and keep this issue open for deciding how to allow per-provider configurations.

And I agree with @seanB that the display formatters are a desirable place to implement related video configuration toggle.

b_sharpe’s picture

What about just an alter? There is already $this->moduleHandler->alter('oembed_resource_url', $parsed_url, $provider); which allows altering BEFORE the oEmbed, could we not just allow an alter after as well and then something like:

/**
 * Implements hook_oembed_url_alter().
 */
function my_module_oembed_url_alter(array &$url, Provider $provider) {
  if ($provider->getName() === 'YouTube') {
    $url['query']['rel'] = 0;
  }
}
b_sharpe’s picture

Status: Needs work » Needs review
FileSize
3.56 KB

Patch to allow altering the HTML provided by the resource.

USE:

/**
 * Implements hook_oembed_resource_html_alter().
 */
function my_module_oembed_resource_html_alter(&$html, Provider $provider) {
  if ($provider->getName() === 'YouTube') {
    $oembed_query = '?feature=oembed';
    $html = str_replace($oembed_query, $oembed_query . '&rel=0', $html);
  }
}
phenaproxima’s picture

So, here's the thing...everything I'm seeing in the patch from #2 is stuff that could be done by contrib or custom code, in a preprocess function. The only reason it's not, is because the resource object itself is not being provided to the theme hook. If core did that, we could have a documented way to set these flags or otherwise modify the HTML coming from the third party service.

So, beyond adding that, I'm not sure that this is a problem core should be solving.

b_sharpe’s picture

Fully agree with the assessment above. So let's get the resource into the theme variables so custom or contrib modules have meaningful preprocess and template suggestion capabilities.

phenaproxima’s picture

I like it. This is a nice, unobtrusive, tightly scoped change that will enable a lot of flexibility for contrib and custom code.

I don't think we need explicit tests of this, but it might be nice to have a small test case in core that proves you can indeed use the resource variable to do something useful in a preprocess function or template. I don't know if that would be handy, or if we'd just be testing the theme system...what do committers think?

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kekkis’s picture

Not adding tests but combining @b_sharpe's work into one patch (from #11 and #13).

Edit: or I may have misunderstood the intentions in #14 actually and this patch might be doing too much already! Maybe #13 alone is enough.

dshumaker’s picture

@Kekkis your attachment patch is 0 bytes.

kekkis’s picture

Yup, so it would seem. Thanks for the heads up @dshumaker! Anyway, there was no added value in it, my attempt was to combine the two different approaches. But it turned out I had misunderstood, and the only thing relevant is @b_sharpe's patch in #13. At least that's how I've understood this issue now.

kekkis’s picture

pameeela’s picture

Title: Need the ability to embed YouTube videos without the related video thumbnails » Ability to embed YouTube videos with related videos from the same channel only
Issue summary: View changes

Updating IS to specify that rel=0 no longer removes related videos but will show related videos only from the same channel.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

gargsuchi’s picture

Status: Needs review » Reviewed & tested by the community

I have tested the patch in #11 and it works perfectly for the use case.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs issue summary update

As #14 suggests we should add some testing around the expanded capabilities.

Also the solution and the issue title don't align. The solution appears to be to provide the resource object to the theme layer as a template variable. The issue summary needs to be updated to reflect this (and probably the issue title).

joshua.boltz’s picture

I could see this becoming it's own small utility module. This patch adds support to allow a configurable ?rel param, but I could also see use cases where users may want to include things like start time and end time, where the video should start playing and when it should end, as well as other params that could be set when embedding Youtube videos
https://developers.google.com/youtube/youtube_player_demo

joshua.boltz’s picture

I've been spending some time trying with this issue and providing users an easy way to add custom parameters to their Youtube URLs they want embedded into the page.

I see there's 3 different solutions offered in this d.o issue alone.
I just applied the patch from #13 (https://www.drupal.org/project/drupal/issues/3009003#comment-13481093), which works well to provide me a way in code to preprocess it, so if i wanted, I could easily str_replace() the existing iframe text to update it to include the params I want.

The problem is I want the user to be in control of those params. But this solution puts me in control, where I _can_ do it, but the params are not known what they will be. With a param like ?rel, it's either a 0 or 1, simple. But, there are other params like ?start and ?end, which allows users to decide when a video should start playing and when it should end. And using the preprocess route in the theme, if i had that information, I could rewrite the iframe to include the params, but i see no way to even get to that information that was provided by the user when they added the URL.

We run into the same issues with the #11 (https://www.drupal.org/project/drupal/issues/3009003#comment-13479553), where it allows us to override it, but we have no way to get it from the user's URL input.

The patch in #2 (https://www.drupal.org/project/drupal/issues/3009003#comment-12860476) seems interesting too, but is geared more towards a simple boolean option for ?rel, so it's either 0 or 1, and I believe it's handled in the media entity settings, whereas the values I would need to track for ?start and ?end would need to be on the media item themselves, coming from the URL field, or other fields on the media bundle.

The main issue I think we're working around here is that Youtube's Oembed API doesn't carry the params along when it builds and provides the iframe.

For example, in OEmbedIframeController.php around line 140, the $resource_url is generated correctly and contains the params, such as:
https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=dspsFCTTu8o&start=10&end=20

Given I entered this URL into the field for the media item
https://www.youtube.com/watch?v=dspsFCTTu8o&start=10&end=20

But, during this line is where it goes bad.

$resource = $this->resourceFetcher->fetchResource($resource_url);

And you can see why when loading it in the browser
https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=dspsF...

So, when ResourceFetcher.php does it's thing around line 73

elseif (strstr($format, 'text/javascript') || strstr($format, 'application/json')) {
      $data = Json::decode($content);
    }

To get the data from the Youtube Ombed URL, noticed the data for "html". The SRC value does not have the params in the URL.
"html": "<iframe width=\"480\" height=\"270\" src=\"https://www.youtube.com/embed/dspsFCTTu8o?feature=oembed\" frameborder=\"0\" allow=\"accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture\" allowfullscreen></iframe>",

So, when Drupal embeds this into the page, obviously, the ?start and ?end params aren't there and the user wonders why.

vacho’s picture

I think that the definitive solution here is to implement a Form configuration at time to embed the video to set parameters that we expected. The patch that works in this direction is #2 https://www.drupal.org/project/drupal/issues/3009003#comment-12860476

We need implement in this direction some new patch that covers the supported parameters:

https://developers.google.com/youtube/player_parameters?hl=en

ilya.no’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
11.75 KB
15.65 KB

Attaching patch with new field formatter "oEmbed YouTube content". I've added not all parameters, only those with "boolean" values and also 'color' and 'start', 'end'. I've added these settings into new formatter, because, for example, 'start' and 'end' parameters don't seem to be properly applied for all videos.

phenaproxima’s picture

Status: Needs review » Needs work

Wait, wait, hold up! :)

What was wrong with the proposed solution #12, and the subsequent patch? This still feels very much like contrib's province to me, and the patch in #13 would have beautifully enabled any number of contrib solutions.

I would definitely feel better if we focused on getting #13 in -- and all we need to accomplish that is a test and an issue summary update. We can get it to RTBC in less than a week if we step on the gas and keep our eyes on the prize. Therefore, I'm tagging this as "needs work" for those two things specifically.

ilya.no’s picture

If we provide ability to embed videos from core and not from contrib, we can add ability to properly display videos with necessary parameters. We can, of course, go contrib or go with custom, I'm just suggestion for those, who finds this helpful. Attaching new patch, previous one has faulty code.

phenaproxima’s picture

If we provide ability to embed videos from core and not from contrib, we can add ability to properly display videos with necessary parameters.

Over time, yes, as the features are needed.

It's best to start with small, tightly-scoped changes; that keeps things simple and maintainable. Wearing my subsystem maintainer hat, I'm not comfortable pushing a complex patch that introduces a slew of YouTube-specific parameters; this would take a LOT longer to get into core anyway, because it would not only need very extensive test coverage, but also a ton of discussion and validation by multiple stakeholders.

So the choice looks, to me, like this: we could either get this done in a few days and accelerate contrib, or we could spend months (maybe years) trying to land this big feature directly in core. I like the first choice, so that's what I'm going to advocate for. :)

ilya.no’s picture

I fully agree with you. I didn't mean to do a harm to the system. Let's assume my patch as an option to choose without any intense.

marcoscano’s picture

+1 for starting off with the simpler approach from #13 .

phenaproxima’s picture

Updated the issue summary. Once we have tests, we can get this over the finish line!

phenaproxima’s picture

Title: Ability to embed YouTube videos with related videos from the same channel only » Expose oEmbed resource object to iframe template

Re-titling for clarity on the proposed resolution.

phenaproxima’s picture

I suspect that the change proposed in this issue would also address #3043821: Better privacy for youtube videos.

phenaproxima’s picture

Issue tags: -Needs tests
FileSize
4.27 KB

And, here's a test. I can't RTBC this issue now since I've written too much of it, so if someone else cares to review it, please do.

phenaproxima’s picture

Status: Needs work » Needs review

D'oh, forgot to change the status.

phenaproxima’s picture

Minor stylistic change.

phenaproxima’s picture

Issue summary: View changes
seanB’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/media/media.module
    @@ -77,6 +77,7 @@ function media_theme() {
    +        'resource' => NULL,
    
    +++ b/core/modules/media/src/Controller/OEmbedIframeController.php
    @@ -146,6 +146,7 @@ public function render(Request $request) {
    +        '#resource' => $resource,
    

    Looks like a simple and flexible change. Like it.

  2. +++ b/core/modules/media/tests/modules/media_test_oembed/media_test_oembed.module
    @@ -7,6 +7,15 @@
    +function media_test_oembed_preprocess_media_oembed_iframe(array &$variables) {
    +  if ($variables['resource']->getProvider()->getName() === 'YouTube') {
    +    $variables['media'] = str_replace('?feature=oembed', '?feature=oembed&pasta=fazoul', (string) $variables['media']);
    
    +++ b/core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php
    @@ -54,4 +63,43 @@ public function testBadHashParameter($hash) {
    +    // This query parameter is added by
    +    // media_test_oembed_preprocess_media_oembed_iframe() for YouTube videos.
    +    $this->assertStringContainsString('&pasta=fazoul', $content);
    

    This is the relevant test coverage, looks good to me.

I think this is ready!

marcoscano’s picture

I also read the code and I'm +1 for this change.

@phenaproxima asked in slack if we needed a CR for this, and in my opinion it would be very useful, if anything, to document more clearly from which point in time on this is something possible to achieve in code.

Thanks @phenaproxima !

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

As per #41. We can set this back to rtbc once the CR exists...

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cf2039b and pushed to 9.1.x. Thanks!

diff --git a/core/modules/media/tests/modules/media_test_oembed/media_test_oembed.module b/core/modules/media/tests/modules/media_test_oembed/media_test_oembed.module
index ad06d590d7..2e19d9face 100644
--- a/core/modules/media/tests/modules/media_test_oembed/media_test_oembed.module
+++ b/core/modules/media/tests/modules/media_test_oembed/media_test_oembed.module
@@ -12,7 +12,7 @@
  */
 function media_test_oembed_preprocess_media_oembed_iframe(array &$variables) {
   if ($variables['resource']->getProvider()->getName() === 'YouTube') {
-    $variables['media'] = str_replace('?feature=oembed', '?feature=oembed&pasta=fazool', (string) $variables['media']);
+    $variables['media'] = str_replace('?feature=oembed', '?feature=oembed&pasta=rigatoni', (string) $variables['media']);
   }
 }
 
diff --git a/core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php b/core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php
index ff51cad571..3b702a6ef7 100644
--- a/core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php
+++ b/core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php
@@ -99,7 +99,7 @@ public function testResourcePassedToPreprocess() {
 
     // This query parameter is added by
     // media_test_oembed_preprocess_media_oembed_iframe() for YouTube videos.
-    $this->assertStringContainsString('&pasta=fazool', $content);
+    $this->assertStringContainsString('&pasta=rigatoni', $content);
   }
 
 }

Fixed spelling on commit.

  • alexpott committed cf2039b on 9.1.x
    Issue #3009003 by ilya.no, phenaproxima, b_sharpe, kirkkala, kekkis,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

AlMunnings made their first commit to this issue’s fork.