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.

CommentFileSizeAuthor
#42 3071760-42.patch4.33 KBphenaproxima
#35 interdiff-3071760-29-35.txt1.03 KBphenaproxima
#35 3071760-35.patch4.25 KBphenaproxima
#30 3071760-29.patch4.28 KBnils.destoop
#26 3071760-diff-24-26.txt593 bytesvijaycs85
#26 3071760-26.patch4.23 KBvijaycs85
#24 3071760-diff-21-24.txt1.5 KBvijaycs85
#24 3071760-24.patch4.23 KBvijaycs85
#21 3071760-21.patch3.54 KBcatch
#21 3071760-interdiff-19-21.txt1.53 KBcatch
#19 interdiff-3071760-16-19.txt789 bytesphenaproxima
#19 3071760-19.patch3.59 KBphenaproxima
#16 interdiff-3071760-10-16.txt1.68 KBphenaproxima
#16 3071760-16.patch3.66 KBphenaproxima
#10 interdiff-3071760-7-10.txt694 bytesphenaproxima
#10 3071760-10.patch3.62 KBphenaproxima
#7 3071760-7-PASS.patch3.61 KBphenaproxima
#7 3071760-7-FAIL.patch2.69 KBphenaproxima
#5 3071760-5-test-only.patch9.45 KBvijaycs85
#5 3071760-5.patch10.74 KBvijaycs85
#2 3071760-2.patch1.28 KBvijaycs85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85 created an issue. See original summary.

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.28 KB

Here is the initial patch...

vijaycs85’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Oooh! Great catch, @vijaycs85!

+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -389,10 +389,17 @@ protected function getLocalThumbnailUri(Resource $resource) {
+    $thumbnail_url_parts = parse_url($remote_thumbnail_url);

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
10.74 KB
9.45 KB

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.

👍🏽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.

Status: Needs review » Needs work

The last submitted patch, 5: 3071760-5-test-only.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.69 KB
3.61 KB

Thanks 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?

phenaproxima’s picture

Issue tags: +backport

Nominating this patch for backport to 8.7.x since it is very clearly a valid and tightly scoped bug fix.

vijaycs85’s picture

Thanks @phenaproxima. The test here is much cleaner than the one in #5. +1 to RTBC.

phenaproxima’s picture

Fixing a minor typo. :)

The last submitted patch, 7: 3071760-7-FAIL.patch, failed testing. View results

seanB’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

phenaproxima’s picture

Title: oEmbed media of thumbnail URL with query string is broken » oEmbed system does not remove query strings from local thumbnail filenames
phenaproxima’s picture

Issue tags: +oembed
catch’s picture

+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -389,6 +389,14 @@ protected function getLocalThumbnailUri(Resource $resource) {
 
+    // Only use the path component of the URL, since PATHINFO_EXTENSION will
+    // include the query string, which we do not want to include in the local
+    // thumbnail URI.
+    $remote_thumbnail_url = parse_url($remote_thumbnail_url, PHP_URL_PATH);
+    if (!$remote_thumbnail_url) {

Instead of doing toString(), then parse_url(), could we use Url::getOptions() then Url::setOptions() to remove the query string first?

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.66 KB
1.68 KB

Yup, that works!

seanB’s picture

Status: Needs review » Reviewed & tested by the community

The change looks good to me. Back to RTBC assuming it will be green.

catch’s picture

+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -387,7 +387,15 @@ protected function getLocalThumbnailUri(Resource $resource) {
+
+    $remote_thumbnail_url = parse_url($remote_thumbnail_url->toString(), PHP_URL_PATH);
+    if (!$remote_thumbnail_url) {
+      return NULL;
+    }

OK 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?

phenaproxima’s picture

Ah, good call. I didn't know about getUri(). That's a lot nicer! Leaving RTBC until proven otherwise...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3071760-19.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
3.54 KB

I think we can do without the extra check altogether and just remove the query string.

seanB’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/phpunit.xml.dist
@@ -18,7 +18,7 @@
-    <env name="SIMPLETEST_DB" value=""/>
+    <env name="SIMPLETEST_DB" value="mysql://catch:@localhost/d8"/>

That was a little confusing in the interdiff but it is not in the patch. So I think this is RTBC again.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -387,7 +387,10 @@ protected function getLocalThumbnailUri(Resource $resource) {
+    $remote_thumbnail_url = $remote_thumbnail_url->setOption('query', [])->toString();

I'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.

$local_thumbnail_uri = "$directory/" . Crypt::hashBase64($remote_thumbnail_url) . '.' . pathinfo($remote_thumbnail_url, PATHINFO_EXTENSION);
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
1.5 KB

Thanks @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).

Status: Needs review » Needs work

The last submitted patch, 24: 3071760-24.patch, failed testing. View results

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
593 bytes

Status: Needs review » Needs work

The last submitted patch, 26: 3071760-26.patch, failed testing. View results

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.

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.

nils.destoop’s picture

Status: Needs work » Needs review
FileSize
4.28 KB

Patch was broken. Included a patch for 9.1.x, but I think it also works for 8.9

phenaproxima’s picture

phenaproxima’s picture

Issue summary: View changes
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Nice little fix, looks good to me - the actual change is simple and the test case is comprehensive.

larowlan’s picture

+++ b/core/modules/media/tests/src/Kernel/OEmbedSourceTest.php
@@ -34,4 +41,50 @@ public function testGetMetadata() {
+    $local_thumbnail_uri = $media_type->getSource()->getMetadata($media, 'thumbnail_uri');
+    $this->assertStringStartsWith('public://oembed_thumbnails/', $local_thumbnail_uri);
+    $this->assertStringNotContainsString('?', $local_thumbnail_uri);
+    $this->assertStringNotContainsString('foo=bar', $local_thumbnail_uri);

Because 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?

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.25 KB
1.03 KB

I was going to push back on that, but the more I thought about it, the more sense that idea made. So, request granted :)

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Review comment addressed, back to RTBC.

larowlan’s picture

  • larowlan committed 9800208 on 9.1.x
    Issue #3071760 by phenaproxima, vijaycs85, catch, nils.destoop, longwave...

  • larowlan committed dd92aad on 9.0.x
    Issue #3071760 by phenaproxima, vijaycs85, catch, nils.destoop, longwave...
larowlan’s picture

Title: oEmbed system does not remove query strings from local thumbnail filenames » [backport] oEmbed system does not remove query strings from local thumbnail filenames
Version: 9.1.x-dev » 9.0.x-dev

Committed 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.

larowlan’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs reroll

This needs re-working for 8.9.x

phenaproxima’s picture

Issue tags: -Needs reroll
FileSize
4.33 KB

Lucky #42, let's see if this passes on 8.9.x.

longwave’s picture

Version: 9.0.x-dev » 8.9.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community

Reroll looks good to me.

phenaproxima’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cfd66d1 and pushed to 8.9.x. Thanks!

Backported to 8.9.x

  • alexpott committed cfd66d1 on 8.9.x
    Issue #3071760 by phenaproxima, vijaycs85, catch, nils.destoop, larowlan...

Status: Fixed » Closed (fixed)

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