Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
when thumbnail URL with query string comes from oEmbed resource, the local file gets the query string on the filename as pathinfo($url, PATHINFO_EXTENSION)
doesn't ignore query string. Here is the sample response from Wistia:
{
"version": "1.0",
"type": "video",
"html": "<iframe src=\"https://fast.wistia.net/embed/iframe/xfepf8u5c4\" title=\"Lenny Delivers Video\" allowtransparency=\"true\" frameborder=\"0\" scrolling=\"no\" class=\"wistia_embed\" name=\"wistia_embed\" allowfullscreen mozallowfullscreen webkitallowfullscreen oallowfullscreen msallowfullscreen width=\"960\" height=\"540\"></iframe>\n<script src=\"https://fast.wistia.net/assets/external/E-v1.js\" async></script>",
"width": 960,
"height": 540,
"provider_name": "Wistia, Inc.",
"provider_url": "https://wistia.com",
"title": "Lenny Delivers Video",
"thumbnail_url": "https://embed-ssl.wistia.com/deliveries/acc2a3080d9de9d8ddb36e2d246f45e7791d93d3.jpg?image_crop_resized=960x540",
"thumbnail_width": 960,
"thumbnail_height": 540,
"player_color": "54bbff",
"duration": 40.207
}
Proposed resolution
Remove query string before get file extension.
Remaining tasks
Review and commit the patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None needed, this is a bug fix.
Comment | File | Size | Author |
---|---|---|---|
#42 | 3071760-42.patch | 4.33 KB | phenaproxima |
#35 | interdiff-3071760-29-35.txt | 1.03 KB | phenaproxima |
#35 | 3071760-35.patch | 4.25 KB | phenaproxima |
#30 | 3071760-29.patch | 4.28 KB | nils.destoop |
#26 | 3071760-diff-24-26.txt | 593 bytes | vijaycs85 |
Comments
Comment #2
vijaycs85Here is the initial patch...
Comment #3
vijaycs85Comment #4
phenaproximaOooh! Great catch, @vijaycs85!
Nit: We can extract just the path, rather than an array of parts, by passing PHP_URL_PATH as the second argument to parse_url().
Otherwise, I think this is a nice bug fix. Just needs a test :) I think a kernel test of the oEmbed source would be sufficient, honestly.
Comment #5
vijaycs85👍🏽
👍🏽I have updated
core/modules/media/tests/src/Functional/FieldFormatter/OEmbedFormatterTest.php
as well to check Wistia. This test class has the potential to convert to Kernel. Using some of the stubbing parts in the new kernel test.Comment #7
phenaproximaThanks for that, @vijaycs85! Removing the "Needs tests" tag.
In reviewing it, though, I thought we might be able to make a simpler kernel test that's more tightly scoped, rather than needing to create yet another oEmbed fixture and do all the scaffolding for a functional test. What do you think of this?
Comment #8
phenaproximaNominating this patch for backport to 8.7.x since it is very clearly a valid and tightly scoped bug fix.
Comment #9
vijaycs85Thanks @phenaproxima. The test here is much cleaner than the one in #5. +1 to RTBC.
Comment #10
phenaproximaFixing a minor typo. :)
Comment #12
seanBI first thought maybe we should check for anchors/fragments in URLs as well, but I guess since the parse_url() documention is very clear about PHP_URL_PATH returning the URL without anchors and query strings, I guess we are fine.
Comment #13
phenaproximaComment #14
phenaproximaComment #15
catchInstead of doing toString(), then parse_url(), could we use Url::getOptions() then Url::setOptions() to remove the query string first?
Comment #16
phenaproximaYup, that works!
Comment #17
seanBThe change looks good to me. Back to RTBC assuming it will be green.
Comment #18
catchOK but now we're removing the query string above, do we still need to do this check? Could it be Url::getUri() instead? Or is it really possible for the thumbnail to have only a query string and no URI?
Comment #19
phenaproximaAh, good call. I didn't know about
getUri()
. That's a lot nicer! Leaving RTBC until proven otherwise...Comment #21
catchI think we can do without the extra check altogether and just remove the query string.
Comment #22
seanBThat was a little confusing in the interdiff but it is not in the patch. So I think this is RTBC again.
Comment #23
alexpottI'm not sure this is correct. This means that we'll make the request to the remote media provider without the query string and potentially it might be important. I think we need to only remove the query string when we generate the local uri.
Comment #24
vijaycs85Thanks @alexpott. Added another variable to handle the local file name. I cloned the object to a) avoid updating original b) avoid another object call (see https://3v4l.org/9CP1N).
Comment #26
vijaycs85Comment #30
nils.destoop CreditAttribution: nils.destoop as a volunteer and commentedPatch was broken. Included a patch for 9.1.x, but I think it also works for 8.9
Comment #31
phenaproximaComment #32
phenaproximaComment #33
longwaveNice little fix, looks good to me - the actual change is simple and the test case is comprehensive.
Comment #34
larowlanBecause the thumbnail URL is known, is there any reason we can't just assertEquals here on the local thumbnail url, i.e. be more explicit in what we're expecting instead of three asserts about the start and contents?
Comment #35
phenaproximaI was going to push back on that, but the more I thought about it, the more sense that idea made. So, request granted :)
Comment #36
longwaveReview comment addressed, back to RTBC.
Comment #37
larowlanComment #40
larowlanCommitted 9800208 and pushed to 9.1.x. Thanks!
As there is little risk of disruption here, and this is a bug, backported to 9.0.x
Queued an 8.9.x run.
Comment #41
larowlanThis needs re-working for 8.9.x
Comment #42
phenaproximaLucky #42, let's see if this passes on 8.9.x.
Comment #43
longwaveReroll looks good to me.
Comment #44
phenaproximaComment #45
alexpottCommitted cfd66d1 and pushed to 8.9.x. Thanks!
Backported to 8.9.x