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
Comment | File | Size | Author |
---|---|---|---|
#87 | 3080666-87.patch | 17.19 KB | Chewie |
#86 | 3080666-86.patch | 12.55 KB | SpadXIII |
#73 | interdiff-3080666-72-73.txt | 654 bytes | phenaproxima |
#73 | 3080666-73.patch | 17.19 KB | phenaproxima |
#72 | interdiff-3080666-868-72.txt | 1.15 KB | phenaproxima |
Issue fork drupal-3080666
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
zipymonkey CreditAttribution: zipymonkey commentedI created a patch that uses
\Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesser
to guess the file extension.Comment #3
joewhitsitt@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 dataComment #5
zipymonkey CreditAttribution: zipymonkey commentedThanks 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.
Comment #6
ieguskiza CreditAttribution: ieguskiza at European Commission and European Union Institutions, Agencies and Bodies commentedHi, 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.
Comment #7
ieguskiza CreditAttribution: ieguskiza at European Commission and European Union Institutions, Agencies and Bodies commentedI just realized that I completely misread what the ExtensionMimeTypeGuesser service did :(
Instead here is a patch using the original solution plus the test.
Comment #8
Upchuk CreditAttribution: Upchuk at European Commission and European Union Institutions, Agencies and Bodies commentedLooked 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.
Comment #9
ieguskiza CreditAttribution: ieguskiza at European Commission and European Union Institutions, Agencies and Bodies commentedWe 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
Comment #10
joevagyok CreditAttribution: joevagyok commentedIt 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.
Comment #11
phenaproximaLet 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.
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.
Nit: I think
/media_test_oembed/thumbnail_no_extension
is a bit verbose. How about just/media_test_oembed/thumbnail
?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?
Comment #12
Upchuk CreditAttribution: Upchuk at European Commission and European Union Institutions, Agencies and Bodies commentedSorry 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.
Comment #13
phenaproximaI 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!
Comment #14
Upchuk CreditAttribution: Upchuk at European Commission and European Union Institutions, Agencies and Bodies commentedA, 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.
Comment #15
Upchuk CreditAttribution: Upchuk at European Commission and European Union Institutions, Agencies and Bodies commentedAdding also a patch for 8.7 and 8.8 to be used in the interim.
Comment #17
seanBNot 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?
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?
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".
Comment #18
george.karaivanov CreditAttribution: george.karaivanov at Peytz & Co commentedPatch 3080666-15-8.8.patch solved my problem with no thumbnail extension from Kaltura embed service
Comment #20
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commentedPer @phenaproxima on Slack,
Comment #21
phenaproximaComment #22
ieguskiza CreditAttribution: ieguskiza at European Commission and European Union Institutions, Agencies and Bodies commentedSimply rerolling the patch for 8.9 for now, I'll try to get the comments from @seanB sorted asap
Comment #24
BladeduLast patch #22 does not apply correctly on 8.9.6
Comment #25
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #22 for Drupal-9.2.x and added a test only patch also as per comment #20.
Comment #26
BladeduRerolled patch #22 against 8.9.9
Comment #27
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedfixed the test case failure for #25.
Comment #28
phenaproximaSelf-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:
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 therequest()
method, which is concrete. Sinceget()
is an alias ofrequest('GET')
, it works the same way.Comment #30
phenaproximaLinking #3214875: Remote Video (oEmbed) - unable to generate thumbnail for Vimeo video as a possible duplicate.
Comment #31
phenaproximaCleaning 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.
Comment #34
phenaproximaTransferring credit from #3214875: Remote Video (oEmbed) - unable to generate thumbnail for Vimeo video, which I've closed as a duplicate.
Comment #35
phenaproximaRerolled 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.
Comment #36
phenaproximaFixed the coding standards violation and added a couple of type hints.
Comment #38
SpokjeNitpick:
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.
Comment #39
phenaproximaNice catch. That shouldn't have been changed in the first place. Fixed!
Comment #40
larowlancorrect 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?
Comment #41
phenaproximaEDIT: Never mind, need to investigate further here.
Comment #42
dcimorraWithout 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.
Comment #43
steinmb CreditAttribution: steinmb as a volunteer commentedI 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?
Comment #44
phenaproximaNot in this patch; this is strictly a bug fix. :)
Comment #45
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented@steinmb Check out: #2983456: Expose triggering update of media metadata + thumbnail to end users
Comment #46
steinmb CreditAttribution: steinmb as a volunteer commentedThank you both for you swift replies.
Comment #47
steinmb CreditAttribution: steinmb as a volunteer commentedComment #48
fgmConfirming #39 works for me (custom Vimeo integration).
Comment #49
phenaproximaResponding to @larowlan's comment in #40:
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:
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.
Comment #50
phenaproximaOK, here's a refactored version which I hope will be clearer. Let's see if tests pass...
Comment #53
phenaproximaThis should fix the broken test. I still would like to change the coverage to use a data provider and test more cases...
Comment #55
phenaproximaRemoving deprecated assertion.
Comment #56
phenaproximaA 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.
Comment #57
phenaproximaFinished 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.
Comment #58
phenaproximaI'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
Comment #60
b_sharpe CreditAttribution: b_sharpe at ImageX commentedPatch looks good, has tests, and most importantly works as intended, RTBC!
Comment #61
larowlanLeft a review in the MR, some of it is just style preferences
NW for the missing try/catch on the HEAD request
Comment #62
phenaproximaThanks 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.Comment #63
pameeela CreditAttribution: pameeela commentedI've manually tested the latest patch and can confirm it fixes the issue.
Video added before patch has no thumbnail, video added after does:
Comment #64
b_sharpe CreditAttribution: b_sharpe at ImageX commentedTry/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
Comment #65
larowlanadding review credits
Comment #69
larowlanI 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.
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.
Comment #71
phenaproximaOh, 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 usedFileSystem::scanDirectory
instead. Tests passed locally. Maybe someday we'll have Finder in core (it sure blowsscanDirectory()
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. 😎Comment #72
phenaproximaAnd, patch. Switching back to patches since GitLab doesn't seem to consider the MR branch part of an active merge request anymore.
Comment #73
phenaproximaMinor change to make the file mask regex more correct.
Comment #74
b_sharpe CreditAttribution: b_sharpe at ImageX commentedTested 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?
Comment #75
phenaproximaYeah, 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.
Comment #76
phenaproximaFiled #3223012: oEmbed system should validate that downloaded thumbnails are valid images to deal with the validation issue.
Comment #78
phenaproximaUnrelated test failure.
Comment #80
larowlanCommitted 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
Comment #81
asigrist CreditAttribution: asigrist commentedI 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.
Comment #82
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedSame 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
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
Comment #83
phenaproximaHi -- 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.
Comment #84
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedHi @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.
Comment #86
SpadXIII CreditAttribution: SpadXIII at SIM commentedFor 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 :)
Comment #87
Chewie CreditAttribution: Chewie for European Commission and European Union Institutions, Agencies and Bodies commentedReroll of patch for 9.1.x
Comment #88
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commentedHm, I'm seeing this issue on 10.1.2 for Vimeo urls.
Comment #89
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commentedSee: https://www.drupal.org/project/drupal/issues/3225184#comment-15181567