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
Comment | File | Size | Author |
---|---|---|---|
#38 | 3009003-38.patch | 4.26 KB | phenaproxima |
#36 | 3009003-36.patch | 4.27 KB | phenaproxima |
#29 | interdiff-2-29.txt | 15.65 KB | ilya.no |
#29 | ability-to-embed-youtube-params-into-remote-video-3009003-29.patch | 11.77 KB | ilya.no |
#13 | 3009003-add-resource.patch | 1.14 KB | b_sharpe |
Issue fork drupal-3009003
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:
Comments
Comment #2
kirkkalaHere 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.
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...
Comment #3
BrankoC CreditAttribution: BrankoC as a volunteer commentedIf 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:
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).
Comment #4
MrMason CreditAttribution: MrMason at Sealed Air commentedI'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?
Comment #5
alexpottThis 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.
Comment #6
seanBCore 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.
Comment #7
phenaproximaComment #8
pameeela CreditAttribution: pameeela commented+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.
Comment #9
lunk rat CreditAttribution: lunk rat commentedI 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.
Comment #10
b_sharpe CreditAttribution: b_sharpe at ImageX commentedWhat 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:Comment #11
b_sharpe CreditAttribution: b_sharpe at ImageX commentedPatch to allow altering the HTML provided by the resource.
USE:
Comment #12
phenaproximaSo, 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.
Comment #13
b_sharpe CreditAttribution: b_sharpe at ImageX commentedFully 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.
Comment #14
phenaproximaI 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?
Comment #16
kekkisNot 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.
Comment #17
dshumaker CreditAttribution: dshumaker at Phase2 commented@Kekkis your attachment patch is 0 bytes.
Comment #18
kekkisYup, 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.
Comment #19
kekkisComment #20
pameeela CreditAttribution: pameeela commentedUpdating IS to specify that rel=0 no longer removes related videos but will show related videos only from the same channel.
Comment #22
gargsuchi CreditAttribution: gargsuchi as a volunteer commentedI have tested the patch in #11 and it works perfectly for the use case.
Comment #23
alexpottAs #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).
Comment #24
joshua.boltz CreditAttribution: joshua.boltz at Mediacurrent commentedI 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
Comment #25
joshua.boltz CreditAttribution: joshua.boltz at Mediacurrent commentedI'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.
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
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.
Comment #26
vacho CreditAttribution: vacho at Skilld commentedI 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
Comment #27
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedAttaching 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.
Comment #28
phenaproximaWait, 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.
Comment #29
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedIf 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.
Comment #30
phenaproximaOver 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. :)
Comment #31
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedI 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.
Comment #32
marcoscano+1 for starting off with the simpler approach from #13 .
Comment #33
phenaproximaUpdated the issue summary. Once we have tests, we can get this over the finish line!
Comment #34
phenaproximaRe-titling for clarity on the proposed resolution.
Comment #35
phenaproximaI suspect that the change proposed in this issue would also address #3043821: Better privacy for youtube videos.
Comment #36
phenaproximaAnd, 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.
Comment #37
phenaproximaD'oh, forgot to change the status.
Comment #38
phenaproximaMinor stylistic change.
Comment #39
phenaproximaComment #40
seanBLooks like a simple and flexible change. Like it.
This is the relevant test coverage, looks good to me.
I think this is ready!
Comment #41
marcoscanoI 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 !
Comment #42
alexpottAs per #41. We can set this back to rtbc once the CR exists...
Comment #43
phenaproximaDone. https://www.drupal.org/node/3169717
Comment #44
alexpottCommitted cf2039b and pushed to 9.1.x. Thanks!
Fixed spelling on commit.