Problem/Motivation

After downloading a thumbnail for a remote oEmbed resource, certain operations (e.g., applying an image style to the thumbnail) can fail if the thumbnail had no file extension, with errors like:

Could not apply Medium (220×220) image style to public://oembed_thumbnails/5qPKeF_Fcn22-QFyRf6_CK8IX1YMBi4xeeLOjBrlWtY. because the style does not support it.

This has recently broken our support for Vimeo, which no longer supplies file extensions in its thumbnail URLs. As an example, see https://vimeo.com/api/oembed.json?url=https://vimeo.com/347119375. Note that the response has a thumbnail_url field, but it has no file extension.

Proposed resolution

If no file extension is present in the thumbnail URL, then use the MIME type of the thumbnail to add a file extension to the local copy of the thumbnail.

Remaining tasks

Review and commit the patch.

Release notes snippet

TBD

CommentFileSizeAuthor
#87 3080666-87.patch17.19 KBChewie
#86 3080666-86.patch12.55 KBSpadXIII
#73 interdiff-3080666-72-73.txt654 bytesphenaproxima
#73 3080666-73.patch17.19 KBphenaproxima
#72 interdiff-3080666-868-72.txt1.15 KBphenaproxima
#72 3080666-72.patch17.19 KBphenaproxima
#63 Screen Shot 2021-07-07 at 7.20.40 am.png25.95 KBpameeela
#57 3080666-57.patch14.63 KBphenaproxima
#56 3080666-56.patch18.03 KBphenaproxima
#55 3080666-55.patch16.7 KBphenaproxima
#53 3080666-53.patch16.68 KBphenaproxima
#50 interdiff-3080666-39-50.txt6.11 KBphenaproxima
#50 3080666-50.patch15.6 KBphenaproxima
#39 3080666-39.patch12.83 KBphenaproxima
#36 3080666-36.patch13.06 KBphenaproxima
#35 3080666-35.patch13.04 KBphenaproxima
#35 3080666-35-FAIL.patch10.01 KBphenaproxima
#27 interdiff_25-28.txt854 bytesnikitagupta
#27 3080666-28.patch13.06 KBnikitagupta
#26 3080666-26.patch36.09 KBBladedu
#25 3080666-25.patch13.08 KBravi.shankar
#25 3080666_test-only.patch10.01 KBravi.shankar
#22 3080666-22.patch13.01 KBieguskiza
#15 3080666-15-8.8.patch12.58 KBUpchuk
#15 3080666-15-8.7.patch12.58 KBUpchuk
#14 interdiff-3080666-14.txt1.88 KBUpchuk
#14 3080666-14.patch12.81 KBUpchuk
#13 3080666-13.patch12.58 KBphenaproxima
#12 interdiff-3080666-12.txt6.21 KBUpchuk
#12 3080666-12.patch9.15 KBUpchuk
#9 interdiff-8-9.txt2.1 KBieguskiza
#9 3080666-9.patch8.52 KBieguskiza
#8 interdiff-3080666-8.txt8.79 KBUpchuk
#8 3080666-8.patch19 KBUpchuk
#7 3080666-7.patch5.38 KBieguskiza
#6 3080666-6.patch8.02 KBieguskiza
#6 3080666-6-test-only.patch3.17 KBieguskiza
#2 3080666-guess-file-extension-of-thumbnail-02.patch2.2 KBzipymonkey

Issue fork drupal-3080666

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

zipymonkey created an issue. See original summary.

zipymonkey’s picture

I created a patch that uses \Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesser to guess the file extension.

joewhitsitt’s picture

@zipmonkey, followed you from #3008119: Provide hook_oembed_providers_alter() in order to stay on topic. Thumbnails don't seem to save/work for our Kaltura/Mediaspace instance due to the reason you described. We are only providing basic level support (thumbnails were not a requirement) for the instance and have just added a min-height to .media-library-item__preview to make the Media Grid Library usable alongside other media with thumbnails and haven't pursued a real solution.

Since thumbnail_url is optional and doesn't not specify extension in the oembed spec, I doubt the provider will change their output based on our previous experience with them. https://oembed.com/#section2

In D7 we altered responses as needed with hook_oembed_reponse_alter(), so I would think a similar approach might be better: #3042423: Add a hook to modify Media data

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

zipymonkey’s picture

Thanks for the feedback @joewhitsitt. I really appreciate it. Adding a min-height is not 100% ideal but it at least makes the UI usable.

We had no luck when asking about getting a file extension added to the thumbnail url.

ieguskiza’s picture

Hi, I also encountered the problem.

I am not 100% certain this is the best place to solve the issue though as the problem comes from the fact that the GDToolkit (and by extension the ImageStyle class) do not support images without extensions. We could try and make it so they do support files without extensions but I think we would be opening a whole other can of worms.

That being said, I think we could use the ExtensionMimeTypeGuesser service instead of staticly calling the symfony methods. Also we could add tests to prove our solution works.
I added to patches, one with just a test and another using the same proposed solution.

ieguskiza’s picture

I just realized that I completely misread what the ExtensionMimeTypeGuesser service did :(
Instead here is a patch using the original solution plus the test.

Upchuk’s picture

Status: Active » Needs review
FileSize
19 KB
8.79 KB

Looked at the solution and I think it's a good start.

A couple of problems though:

1. Patch doesn't apply, the extra fixture file is not included
2. Requesting directly a local file without an extension does not seem to return a Content-Type header.
3. The solution misses a case that the getLocalThumbnailUri() method does, namely that it returns the local file after subsequent requests.

I attach a patch and sort of an interdiff between #7 and #8 addressing all these points:

1. New file (copy of an existing jpg fixture file) without extension
2. Creating a resource Controller endpoint for the file that returns it with the proper Content-Type header
3. If the path extension cannot be determined (due to lack of actual file extension in the resource), a simple HEAD request is made early on to try to deduce the extension. If found, this allows for returning a local thumbnail in all subsequent requests with the cost of a simple HEAD request instead of a full download every time.

The test coverage also includes this aspect by keeping track of all types of requests made to the endpoint.

ieguskiza’s picture

We noticed that since we are using the controller to handle the image we don't need to ship with a new fixture. I attached the same patch but using an already existing image. Interdiff is also provided

joevagyok’s picture

It also worth noticing that the previous patch did not apply on linux machine with composer nor git apply, because of the binary image in the diff. Without the image, the patch applies correctly. The patch looks good for me.
I have tested the patch in our project and it works.
Thank you @ieguskiza and @Upchuk.

phenaproxima’s picture

Let me start by saying that I really like this approach. I think the idea of first trying pathinfo(), then falling back to the more expensive HTTP request, is smart. My only questions concern aspects of how we're testing this.

  1. +++ b/core/modules/media/tests/fixtures/oembed/video_vimeo-no-extension.json
    @@ -0,0 +1,16 @@
    +{
    +  "type": "video",
    +  "version": "1.0",
    +  "provider_name": "Vimeo",
    +  "provider_url": "https:\/\/vimeo.com\/",
    +  "title": "",
    +  "author_name": "Tendenci - The Open Source AMS",
    +  "author_url": "https:\/\/vimeo.com\/schipul",
    +  "html": "<iframe width=\"480\">By the power of Greyskull, Vimeo works!</iframe>",
    +  "width": 480,
    +  "height": 360,
    +  "description": "Special thanks to Tendenci, formerly Schipul for sponsoring this video with training, equipment and time. The open source way. All creative however was self directed by the individuals - A. Hughes (www.schipul.com\/ahughes) featuring QCait (www.schipul.com\/qcait) - Hands On Drupal\n\nDrupal is a free software package that allows an individual or a community of users to easily publish, manage and organize a wide variety of content on a website.\n\nNeed a little Drupal help or just want to geek out with us?  Visit our www.schipul.com\/drupal for more info - we'd love to connect!\n\nGo here for Drupal Common Terms and Suggested Modules : http:\/\/schipul.com\/en\/helpfiles\/v\/229",
    +  "thumbnail_url": "internal:\/media_test_oembed\/thumbnail_no_extension",
    +  "thumbnail_width": 295,
    +  "thumbnail_height": 221
    +}
    

    I'm wondering if creating a whole new fixture for this isn't a bit excessive. All the existing fixtures already use core/misc/druplicon.png, so it seems like it should be fine to just modify an existing one to use this extensionless thumbnail URL.

    If we choose to go that route, I suggest we modify video_collegehumor.xml, since that will allow us to insert a comment explaining why the thumbnail URL is different.

  2. +++ b/core/modules/media/tests/modules/media_test_oembed/media_test_oembed.routing.yml
    @@ -4,3 +4,9 @@ media_test_oembed.resource.get:
    +  path: '/media_test_oembed/thumbnail_no_extension'
    

    Nit: I think /media_test_oembed/thumbnail_no_extension is a bit verbose. How about just /media_test_oembed/thumbnail?

  3. +++ b/core/modules/media/tests/modules/media_test_oembed/src/Controller/ResourceController.php
    @@ -36,6 +42,25 @@ public function get(Request $request) {
    +    // Keep track of the GET and HEAD requests.
    +    $state = \Drupal::state()->get(static::THUMBNAIL_NO_EXTENSION_STATE_ID, []);
    +    $request_method = \Drupal::request()->getMethod();
    +    $state[$request_method] = isset($state[$request_method]) ? $state[$request_method] + 1 : 1;
    +    \Drupal::state()->set(static::THUMBNAIL_NO_EXTENSION_STATE_ID, $state);
    

    I think this is a little bit awkward, to be honest. I do agree that it's worth testing that we're not making unnecessary requests, but I think it would make more sense to do this with a mocked HTTP client, testing the OEmbed source plugin directly. Maybe we could do this in a new, dedicated kernel test?

Upchuk’s picture

Sorry for the delay but got caught up with other stuff.

So I addressed some of your points here:

1. I used the collegehumor one instead, but had to adjust also the resource controller to return XML content. I guess it's a good byproduct that we support that as well.

2. Simplified the endpoint.

3. I don't know about this. I gave it a whirl, trying to mock the http client. The problem there is that there is a lot to do to test something small which is actually mostly tested in the functional test. Moreover, mocking the client endpoint is, as far as I managed, not really easy because methods like get() are magic and cannot be mocked (since they don't actually exist). And if you mock the underlying methods (like request()) those don't get called on the mock.. Maybe there is a way? In any case, an entire new Kernel test for this seems also a bit over-engineered, especially that we'd have to again count anyway the requests made to the GET and HEAD methods...

Let me know what you think.

phenaproxima’s picture

I took a crack at it and was able to get a kernel test working using mocked services. I think this is going to be a little more explicit, and allow us to more easily test low-level edge cases in the future. Did some additional random clean-up as well...overall I think this is looking great!

Upchuk’s picture

A, ok, I see what you mean. I've never worked with prophecy and was trying with the mock builder :)

I corrected some of the comments in the test and made them a bit more explicit.

Upchuk’s picture

Adding also a patch for 8.7 and 8.8 to be used in the interim.

The last submitted patch, 15: 3080666-15-8.7.patch, failed testing. View results

seanB’s picture

Not really sure if this is actually fixing a bug or not. Can we update the title and IS a bit the clarify what we are doing here? And can we have a fail patch to demonstrate the actual issue? It seems it is technically still possible our extra efforts could produce a thumbnail without an extension. So I would also like to see a test to demonstrate what happens when this is the case. Maybe we should fall back to the default thumbnail when all our efforts fail?

  1. +++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
    @@ -392,7 +393,11 @@ protected function getLocalThumbnailUri(Resource $resource) {
    +    if ($path_extension) {
    

    It seems there is still somehow a possibility we can't get an extension. Should we inform users about this so they know they might need to add a custom thumbnail or something? Maybe we should fall back to the default thumbnail in this case?

  2. +++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
    @@ -410,7 +415,7 @@ protected function getLocalThumbnailUri(Resource $resource) {
    +      $response = $this->httpClient->request('GET', $remote_thumbnail_url);
    

    Not sure why this is changed?

Maybe we can also have a followup to fix the issue "no extension found" and for now rescope this issue to something like "Try to get extension from request when pathinfo is unable to get an extension".

george.karaivanov’s picture

Patch 3080666-15-8.8.patch solved my problem with no thumbnail extension from Kaltura embed service

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now 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.

Chris Matthews’s picture

Status: Needs review » Needs work

Per @phenaproxima on Slack,

I think we need to prove a bug exists, which means we need a fail patch (i.e., a patch containing a test which fails with the current state of the code)

phenaproxima’s picture

ieguskiza’s picture

Simply rerolling the patch for 8.9 for now, I'll try to get the comments from @seanB sorted asap

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Bladedu’s picture

Last patch #22 does not apply correctly on 8.9.6

ravi.shankar’s picture

Added reroll of patch #22 for Drupal-9.2.x and added a test only patch also as per comment #20.

Bladedu’s picture

nikitagupta’s picture

Status: Needs work » Needs review
FileSize
13.06 KB
854 bytes

fixed the test case failure for #25.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning to sort out the issue summary and get a fail patch up against 9.2.x, as @seanB requested in #17.

To address his second point, though:

Not sure why this is changed?

I believe that changed because AFAIK it's impossible to mock the get() method for testing, since it's a magic method (i.e., implemented using __call() or something). The only way to mock the client properly here was to switch to the request() method, which is concrete. Since get() is an alias of request('GET'), it works the same way.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

phenaproxima’s picture

phenaproxima’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +Bug Smash Initiative

Cleaning up the issue summary using helpful information from #3214875: Remote Video (oEmbed) - unable to generate thumbnail for Vimeo video, and escalating priority since this breaking our Vimeo support. Also tagging this for inclusion in the Bug Smash Initiative, since it is clearly a bug, and closing the duplicate issue.

phenaproxima’s picture

Transferring credit from #3214875: Remote Video (oEmbed) - unable to generate thumbnail for Vimeo video, which I've closed as a duplicate.

phenaproxima’s picture

Rerolled against 9.3.x and created a fail patch while I was at it. This ought to be everything needed to get this to RTBC.

phenaproxima’s picture

Fixed the coding standards violation and added a couple of type hints.

The last submitted patch, 35: 3080666-35-FAIL.patch, failed testing. View results

Spokje’s picture

Status: Needs review » Needs work

Nitpick:

diff --git a/core/modules/media/tests/fixtures/oembed/video_collegehumor.xml b/core/modules/media/tests/fixtures/oembed/video_collegehumor.xml
index 76815ed138..b799d8daf1 100644
--- a/core/modules/media/tests/fixtures/oembed/video_collegehumor.xml
+++ b/core/modules/media/tests/fixtures/oembed/video_collegehumor.xml
@@ -10,9 +10,12 @@
   <provider_url>http://www.collegehumor.com</provider_url>
   <width>610</width>
   <height>343</height>
-  <html><h1>By the power of Grayskull, CollegeHumor works!</h1>
-  </html>
-  <thumbnail_url>internal:/core/misc/druplicon.png</thumbnail_url>
+  <html><h1>By the power of Grayskull, CollegeHumor works!</h1></html>
+  <html><h1>By the power of Greyskull, CollegeHumor works!</h1></html>

I agree we can never have enough "Power of Greyskull", but the double adding seems superfluous.
When I tested locally with (there can be) only one of them, the tests still pass.

Other than that, looks fine to me.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
12.83 KB

Nice catch. That shouldn't have been changed in the first place. Fixed!

larowlan’s picture

+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -432,6 +437,32 @@ protected function getLocalThumbnailUri(Resource $resource) {
+    $response = $this->httpClient->request('HEAD', $thumbnail_url);

correct me if I'm wrong but this relies on Drupal being able to make requests back to itself because the URL we're passing here is a local thumbnail url?f

What happens if e.g. the shield module is enabled for httpauth in dev/stage environments?

If this is a local url, can we not use finfo_file instead?

phenaproxima’s picture

EDIT: Never mind, need to investigate further here.

dcimorra’s picture

Without applying the patch now it works for me but for example on Friday if I did not apply it, it didn't work for me, is it happening to someone else?

Thx.

steinmb’s picture

I am currently migrating a site remote videos from D7, and also noticed this. Will there be a way for us to bulk update/re-generate these thumbnails later on or does these have to be generated when the media entity gets created?

phenaproxima’s picture

Will there be a way for us to bulk update/re-generate these thumbnails later on

Not in this patch; this is strictly a bug fix. :)

Chris Matthews’s picture

steinmb’s picture

Thank you both for you swift replies.

steinmb’s picture

fgm’s picture

Confirming #39 works for me (custom Vimeo integration).

phenaproxima’s picture

Status: Needs review » Needs work

Responding to @larowlan's comment in #40:

correct me if I'm wrong but this relies on Drupal being able to make requests back to itself because the URL we're passing here is a local thumbnail url?f

What happens if e.g. the shield module is enabled for httpauth in dev/stage environments?

If this is a local url, can we not use finfo_file instead?

I agree with you, something doesn't smell right here. The thumbnail URL should never be a local URL; this should never make a request to Drupal itself except maybe in testing circumstances.

The idea is this:

  • Given a thumbnail URL (which will normally be remote, not served by Drupal), create a deterministic local URI where it will be stored.
  • If that local URI doesn't yet exist, we'll have to download the thumbnail.
  • To do that, we need to know the file extension (so that the downloaded thumbnail can be used with image styles and whatnot).
  • See if we can determine the file extension by parsing the remote thumbnail URL.
  • If we can't, then make a HEAD request to the remote thumbnail URL and see if we can figure out its MIME type.

I think the code we have in the patch is implementing this logic, but it's unclear due to poor variable naming. I'll refactor it to be a bit clearer.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
15.6 KB
6.11 KB

OK, here's a refactored version which I hope will be clearer. Let's see if tests pass...

The last submitted patch, 39: 3080666-39.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 50: 3080666-50.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
16.68 KB

This should fix the broken test. I still would like to change the coverage to use a data provider and test more cases...

Status: Needs review » Needs work

The last submitted patch, 53: 3080666-53.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
16.7 KB

Removing deprecated assertion.

phenaproxima’s picture

A little refactoring on the test to reduce the amount of mocking, and trying to shape it into something that can be used with a data provider.

phenaproxima’s picture

Finished refactoring the test -- it's almost a full rewrite, but it's quite a bit better now and helps us test more scenarios than we previously did.

phenaproxima’s picture

I've been reliably informed that some people are annoyed by my lack of interdiffs. To you, I say HUMBUG! Also, here's a merge request. <3

b_sharpe’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good, has tests, and most importantly works as intended, RTBC!

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Left a review in the MR, some of it is just style preferences

NW for the missing try/catch on the HEAD request

phenaproxima’s picture

Status: Needs work » Needs review

Thanks for the review, @larowlan. Good catch (see what I did there?) on the missing try and test coverage. That's fixed now. I pushed back a little on the stylistic stuff, but hopefully what we have now is reasonable.

pameeela’s picture

I've manually tested the latest patch and can confirm it fixes the issue.

Video added before patch has no thumbnail, video added after does:

b_sharpe’s picture

Status: Needs review » Reviewed & tested by the community

Try/Catch is implemented. I agree with the inline "if" statements, they way it is in current MR makes for easier readability on what you're testing against. Tests still passing and functionality works. RTBC!

One note if this is ever to be backported to 9.1 (or just re-rolled for patch), is it appears a few `use` statements are missing for Guzzle Exceptions, this does not affect 9.2/9.3

larowlan’s picture

adding review credits

  • larowlan committed 7bebde3 on 9.2.x
    Revert "Issue #3080666 by phenaproxima, Upchuk, ieguskiza, ravi.shankar...
larowlan’s picture

Status: Reviewed & tested by the community » Needs work

I merged this to 9.3.x and backported to 9.2.x

But then I reverted it because we don't have a dependency on symfony/finder and this is adding a runtime usage of it.

So back to needs-work.

composer why symfony/finder
drupal/drupal      9.2.x-dev  requires (for development)  symfony/finder (^4.4)
composer/composer  2.0.13     requires                    symfony/finder (^2.8.52 || ^3.4.35 || ^4.4 || ^5.0)

composer why composer/composer
drupal/drupal  9.2.x-dev  requires (for development)  composer/composer (^2.0.2)

So both of those are only dev usages.

So we need to either do this without finder, or we need to go through the dependency addition process in order to make it a non-dev dependency.

  • larowlan committed 4067ac0 on 9.3.x
    Revert "Issue #3080666 by phenaproxima, Upchuk, ieguskiza, ravi.shankar...
phenaproxima’s picture

Status: Needs work » Needs review

Oh, man. Really glad you caught that; would have been sad if this had been falsely fixed in production releases.

But, not to worry; the OEmbed source already has the file_system service injected, so I just used FileSystem::scanDirectory instead. Tests passed locally. Maybe someday we'll have Finder in core (it sure blows scanDirectory() out of the water from a DX standpoint) but I'm not gonna be the one to drag it through the Swamp of Vetted Dependencies. 😎

phenaproxima’s picture

And, patch. Switching back to patches since GitLab doesn't seem to consider the MR branch part of an active merge request anymore.

phenaproxima’s picture

Minor change to make the file mask regex more correct.

b_sharpe’s picture

Status: Needs review » Reviewed & tested by the community

Tested patch locally and still does the job. Code change is very minor and tests still pass. RTBC again

Minor note that I don't believe is in scope of this change is whether or not we should allow the extension to be anything other than valid image types, let's say the HEAD request returns an mp4 for example, or worst-case, "php" (gasp). Possibly worth a follow up issue?

phenaproxima’s picture

Issue tags: +Needs followup

Minor note that I don't believe is in scope of this change is whether or not we should allow the extension to be anything other than valid image types, let's say the HEAD request returns an mp4 for example, or worst-case, "php" (gasp). Possibly worth a follow up issue?

Yeah, I think it is. Even right now in HEAD, there's nothing preventing a provider from sending us something with a dangerous extension -- although AFAIK nothing they send us is ever executed by the PHP interpreter. So even if they sent us executable code, it would just be fed into the image toolkit, which would probably (hopefully) choke on it.

Tagging for a follow-up and crediting @b_sharpe for raising this point.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: 3080666-73.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test failure.

  • larowlan committed a23557a on 9.2.x
    Issue #3080666 by phenaproxima, Upchuk, ieguskiza, larowlan, ravi....
  • larowlan committed bb02eb5 on 9.3.x
    Issue #3080666 by phenaproxima, Upchuk, ieguskiza, larowlan, ravi....
larowlan’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed bb02eb5 and pushed to 9.3.x. Thanks!

Backported to 9.2.x

Second time lucky, thanks folks for the quick turnaround on the redo

asigrist’s picture

I can't believe how timely this patch is, I just set up Vimeo video on a site I'm developing. Thank you to everyone who worked on it.

I installed the latest patch and can indeed see that my remote video thumbnails now have proper file extensions and are in the folder I set them to...but they're still not appearing in my Media Library preview. I've flushed every cache and examined my ML view mode and everything looks as I think it should. Any ideas? (or does this query now belong in a new thread/issue?).

One interesting thing is that I actually do see a tiny thumbnail when I'm first creating the video from my Paragraph, but as soon as I insert it, it is not displaying in the Media library preview, either in my node or in the ML view.

Bhanu951’s picture

Same issue for me as well as mentioned in #81 . I have created a custom media plugin for Tiktok using hook_media_source_info_alter() . I am able to create the media entity . Thumbnail is being rendered during media save.

But If I try to add the media field in the node the thumbnail is not being generated.

I am getting the error

Could not apply Medium (220×220) image style to public://oembed_thumbnails/QqccOC33trPlEK1OKjHsTC1ZGZKksB2ieeHVVkD-OWg.image because the style does not support it.

Reference images :

Media Library :
https://i.postimg.cc/jSq5DrQv/Screenshot-2021-07-22-at-2-53-19-PM.png

Node Attach :
https://i.postimg.cc/R0NZTQYH/Screenshot-2021-07-22-at-2-57-43-PM.png
https://i.postimg.cc/4x83tFWG/Screenshot-2021-07-22-at-2-58-39-PM.png
https://i.postimg.cc/Zq2KsvkD/Screenshot-2021-07-22-at-2-59-14-PM.png

https://postimg.cc/gallery/902VmWB

phenaproxima’s picture

Hi -- thanks for testing this out. Would you be willing to each open separate issues for your respective troubles? This issue is fixed, so it's not really monitored anymore, and chances are your problems are not related to the bug that was dealt with here. :) Alternately, you can find me in Drupal Slack (I hang out in the #media channel all the time) and I can try to give some advice.

Bhanu951’s picture

Hi @phenaproxima, Thank you for fixing the issue . I have created a new issue #3225184 Please ave a look into it. I believe the issue is related because before applying this patch I was not able to create the media entity itself but after applying this patch I was able to create media entity and render thumbnail in stand-lone media creation.

Status: Fixed » Closed (fixed)

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

SpadXIII’s picture

FileSize
12.55 KB

For those looking for a fix for d8.9; I rerolled the patch in #26 to remove the .rej en .orig files. Other than that, it applies and seems to still work fine :)

Chewie’s picture

FileSize
17.19 KB

Reroll of patch for 9.1.x

Chris Matthews’s picture

Hm, I'm seeing this issue on 10.1.2 for Vimeo urls.

Chris Matthews’s picture