Problem/Motivation

When adding a remote video in media library the standard thumbnail fetched from oEmbed.
This results in low res images when using the thumbnails while theming.

Steps to reproduce

Proposed resolution

We should get the highres thumbnails from the providers and use drupal core image styles to generate low res derivates if needed.

Remaining tasks

Decide on overall architecture. See comment 25

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3447326

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:

Comments

Wouter Waeytens created an issue. See original summary.

wouter waeytens’s picture

StatusFileSize
new1.6 KB
cilefen’s picture

Version: 10.4.x-dev » 11.0.x-dev
Status: Active » Needs work
Issue tags: +Needs issue summary update
wouter waeytens’s picture

Issue summary: View changes
cilefen’s picture

isalmanhaider’s picture

Status: Needs work » Reviewed & tested by the community
StatusFileSize
new423.27 KB

+1

#2 worked! Tested on Drupal 11.0-dev, PHP 8.3.

Good catch and a must-have fix.

By default, it pulls low resolution (480 × 360 pixels) and stores as original style.
After applying the patch, it pulls high resolution (1280 × 720 pixels) and stores as original style.

Only local images are allowed.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

There is a patch here that failed testing. Please fix that and merge requests are preferred.

cilefen’s picture

Also: It isn't clear what the screenshots in comment #6 are representing as supporting this issue being ready to commit.

quietone’s picture

Version: 11.0.x-dev » 11.x-dev

Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

bessone’s picture

The patch also worked on my Drupal 10.3.2 (with Thunder distro).

I haven't done any testing, just added the patch in composer.json on a demo I'm working on.

sahana _n made their first commit to this issue’s fork.

sahana _n’s picture

Status: Needs work » Needs review

Hi,

I created an MR Please review I tested the test cases, and now the test cases are working fine.

I would greatly appreciate your suggestions for improvement. Please let me know.

Thank you!!

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left comments in MR.

sahana _n’s picture

Status: Needs work » Needs review

Hi @smustgrave ,

Thanks for the review.

I updated the MR. The test coverage for the provider's name is already covered.
Path: core/modules/media/tests/src/Functional/ResourceFetcherTest.php

Please let me know for other concerns. I am happy to take suggestions.

Thank you!!

smustgrave’s picture

Status: Needs review » Needs work

Test coverage should be expanded for additional changes.

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

daveiano’s picture

While it works and gets the maxresdefault image for most YouTube Videos, it does not work every time. There is a chance that a video does not provide a maxresdefault image, just the default hqdefault. I guess that's something to do with the video size.

I added a check that the maxresdefault image is only used, when available. Otherwise, it will just return a 404.

Before the change, this would leave media entities without a thumbnail at all.

eliechoufani’s picture

Hello,

I had the same problème on Drupal 10.3.5

Trying the patch in #2 didn't solve the problem because the http 404 error request was overriding the response.

So I did this patch that's working for my use case, maybe it needs more testing

wouter waeytens’s picture

I can confirm that patch in #19 works :)

pcambra’s picture

recrit’s picture

There are a few issues with the patch in #19:

  1. In the new method doVimeoRequest, the TransferException is not defined and needs to be imported.
  2. the import "use GuzzleHttp\Client" is never used.
  3. The fallback for Youtube should be uncommented:
          catch (RequestException $e) {
            // Use default thumbnail.
            //$data['thumbnail_url'] = str_replace('maxresdefault', 'hqdefault', $data['thumbnail_url']);
          }
    
recrit’s picture

Pushed some fixes for better error handling. This is now working for me when Vimeo does not have a high resolution option.
Thinking about this more, I feel like it could be better handled in \Drupal\media\Plugin\media\Source\OEmbed::getLocalThumbnailUri() so that the high resolution image is only requested once and then the local file is created. Currently, the fixes in this issue would causes the high resolution images to be request every time the resource cache is rebuilt. This seems a bit excessive to me.

jesus_md’s picture

Hi I have some errors that break the site when a Vimeo video is removed. So I have created a patch from Drupal 10 (Tested on 10.4.1) with Drupal 11 solution. If anyone have the same problem, try to use this patch and check if it solve the error.

If you want to replicate the error I had, just create a remote video with a fake vimeo url like this one: https://vimeo.com/3453453453453

dalin’s picture

Shouldn't the bulk of this code live within the provider rather than here?

Maybe ResourceFetcherInterface should ask the ProviderRepositoryInterface "Here's the data from the oEmbed call. Is there a URL for a higher-quality version of the thumbnail?" and it responds with a URL. Then ResourceFetcherInterface then tests to see if it can get an image from that URL, otherwise falls back to the original.

With this approach ResourceFetcherInterface doesn't need to know about how each provider's API works, and the providers don't have to replicate the work of testing to see if the higher-quality version exists.

dalin’s picture

dalin’s picture

Issue summary: View changes
relisiuol’s picture

StatusFileSize
new1.6 KB

Fix oembed & get max thumbnail size for vimeo & youtube.

relisiuol’s picture

StatusFileSize
new2.37 KB

Better patch for vimeo because some large thumbnails are too small.

relisiuol’s picture

StatusFileSize
new2.36 KB
robcarr’s picture

I couldn't apply the patches 28-30, but the latest MR applied. I'm assuming I need to play about with CSS on the IFRAME to get the larger thumbnail to render in a suitable size, rather than some magic happening out of the box.

Thumbnail looks great.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

jalpesh’s picture

StatusFileSize
new2.3 KB

Updated patch with latest 11.3.8 release.

swild-ma’s picture

On Vimeo we have most of our videos marked as "Public". The patch from #33 works for these, thanks.
However, I came across one problem. We have at least one video we licensed that is marked as "Hide from vimeo".
Fortunately I do not need a thumbnail for this video, but when doVimeoRequest() requests "https://vimeo.com/api/v2/video/nnnnnnnn.xml" for this video anyway, i get a 404.
Ignoring the 404 in doVimeoRequest() and returning an empty array and as a result leaving the thumbnail_url untouched in fetchResource() solves the problem for us.

Thanks again!

jwilson3’s picture

I've seen and can confirm the same issue as #34.

In our case, a Vimeo Pro account where videos were set to be domain-restricted to our dev/test/prod domains only. We see 404s when fetching the larger image size.

It seems fundamentally broken that the video can play on the site, but the request to cache the poster image for that video when run from the same website wouldn't also work. I know they're two totally separate mechanisms, but there should probably be a way to authenticate for api requests to get the poster image of private videos somehow.

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

johnatas’s picture

Hey,

The current implementation in !9518 introduces a regression (with the sixth commit) for environments where allow_url_fopen is disabled (which is a common security hardening practice in production).

The call to getimagesize($data['thumbnail_url']) with a remote URL relies on PHP's URL stream wrappers, which require allow_url_fopen=1. When disabled, it produces the following warnings:

Warning: getimagesize(): https:// wrapper is disabled in the server configuration by allow_url_fopen=0
Warning: getimagesize(https://...): Failed to open stream: no suitable wrapper could be found

This is particularly problematic since the rest of the class already uses Guzzle (via $this->httpClient) for all HTTP requests, making the direct use of getimagesize() with a URL inconsistent and environment-dependent.

I pushed a new commit replacing getimagesize() with a Guzzle request followed by getimagesizefromstring(), which works on the raw response body and has no dependency on allow_url_fopen