Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem

We need tests. Please post also a "test only" patch that proves the bug.

Rerolled it against the 8.5.x branch.

Here is a test only patch and a patch with the test.

Did not add an interdiff, since it's literally the same as the test-only patch :)

Hm, interestingly enough it seems the logic we depend on here, does not work if the file is in the top directory.

$path_info = pathinfo($image_uri);
$converted_image_uri = $path_info['dirname'] . DIRECTORY_SEPARATOR . $path_info['filename'];
if (!file_exists($converted_image_uri)) {

This code ends up checking if private:/test_image.png exists.

While if the image was inside of "images" it would end up checking if private://images/test_image.png

This also fails for public files by the way. And is tracked in the issue #2630230: Image effect convert fails when image file is in the public files root.

So not sure if we should postpone until that one is landed?

+1 to patch #12 - which solves my issue over in #3072051: "Convert JPG" Image Effect doesn't work for private files

Hmm, something's not quite right here - patch #12 works (converting PDF to JPG) but the server seems to serve it up as the wrong file type. It displays fine on Chrome on Linux, shows up as a question mark on Safari. I open the myfile.pdf.jpeg image in a new tab, Chrome says it's having difficulty displaying the PDF.

If I move the generated myfile.pdf.jpeg to a public portion of the server, it serves up just fine.

So even with patch #12, there's something wildly wrong about the returned mimetype.

Just starting to look at this.

I've found the issue. Originally, I was thinking that private file headers were being incorrectly generated by this block of code:

    // If using the private scheme, let other modules provide headers and
    // control access to the file.
    if ($scheme == 'private') {
      $headers = $this->moduleHandler()->invokeAll('file_download', [$image_uri]);
      if (in_array(-1, $headers) || empty($headers)) {
        throw new AccessDeniedHttpException();
      }
    }

This is using the original $image_url to generate the headers. But if the content type had changed in the derivative form (like PDF to JPEG), $headers would remain an application/pdf. I thought this should be $derivative_url, not $image_url. But $derivative_url isn't a file in the file DB table, so this method of figuring out the mimetype would fail.

What I later noticed is that derivative image's $headers are recalculated just a few lines down in the same function:

    if ($success) {
      $image = $this->imageFactory->get($derivative_uri);
      $uri = $image->getSource();
      $headers += [
        'Content-Type' => $image->getMimeType(),
        'Content-Length' => $image->getFileSize(),
      ];
      // \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
      // sets response as not cacheable if the Cache-Control header is not
      // already modified. We pass in FALSE for non-private schemes for the
      // $public parameter to make sure we don't change the headers.
      return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
    }

The problem with this is that $headers['Content-Type'] and $headers['Content-Length'] are already set, and this += operator on the $headers array is not replacing existing values. As defined on php.net:

The + operator returns the right-hand array appended to the left-hand array; for keys that exist in both arrays, the elements from the left-hand array will be used, and the matching elements from the right-hand array will be ignored.

Instead, this additional block works great:

    if ($success) {
      $image = $this->imageFactory->get($derivative_uri);
      $uri = $image->getSource();
      $headers['Content-Type'] = $image->getMimeType();
      $headers['Content-Length'] = $image->getFileSize();
      // \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
      // sets response as not cacheable if the Cache-Control header is not
      // already modified. We pass in FALSE for non-private schemes for the
      // $public parameter to make sure we don't change the headers.
      return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
    }

I'm not sure why the += operator was there. Presumably the headers from the derivative file are really want we really want to serve up here.

Here's a patch that should work much better.

Just to say that I installed the patch mentioned in #20, and it seems to be working on my end! JPEG images in the private file system are converted correctly, and I haven't come across any issues yet.

Hope this gets submitted soon!

Thanks to https://www.drupal.org/project/drupal/issues/2786735#comment-13378964 after doing research for 2 hours this patch worked for me on Drupal 9.3.

I was also using "Convert" on all image styles in Drupal and the Private File system and was not able to generate image styles.

Thanks to all the contributers.

I can confirm patch #20 works as expected!

Rerolled patch against #20 for 9.5.

Rerolled patch against #20 for 9.4.3.

Tested the latest patch on #34, if the private files in in the root, ie private://something.jpg.webp

$path_info = pathinfo(StreamWrapperManager::getTarget($image_uri));
$converted_image_uri = sprintf('%s://%s%s%s', $this->streamWrapperManager->getScheme($derivative_uri), $path_info['dirname'], DIRECTORY_SEPARATOR, $path_info['filename']);

converts the uri to `private://./something.jpg, which will failed on the invoke call below

$headers = $this->moduleHandler()->invokeAll('file_download', [$image_uri]);

Can we make the converted image to remove the period of the path file is in the root something like this?

$converted_image_uri = sprintf('%s://%s%s', $this->streamWrapperManager->getScheme($derivative_uri), $path_info['dirname'] === '.' ? '' : $path_info . DIRECTORY_SEPARATOR, $path_info['filename']);

Here is the updated patch to fixed the root path to show up correctly when the converted images are in the root private path.

#3310963: Wrong file header returned, when converting an image for example to webp seems to have the same root cause, described in #20 so the priority is definitely major here.

Tried to fix the failed test of patch #37.

I tested patch #39 and everything is working as expected.

Patching Drupal 9.4.8 with #39 generates image styles (with WEBP convertion) in private folder.
Thanks

I am getting intermittent file not being found after applying the patch. The image will not appear and I cannot refresh the page to bring it back. Instead I have to flush image style cache, drush if my-image-style, then it will be regenerated.

I am not sure why the image url is what it is.

I am using the private file system. I am converting a jpg to webp
Drupal 9.5.8
php8.1

the image url is:

http://mysite/system/files/styles/my-image-style/private/2023-05/myimage.jpg.webp?itok=R0SbukIK

might be helpful: https://www.drupal8.ovh/en/tutoriels/83/get-a-image-file-path-src-on-dru...
The url seems odd.
1. On the sever the converted images are in
mysite/private/files/styles/my-image-style/private/2023-05

So the actual file is here: private/files/styles/my-image-style/private/2023-05/myimage.jpg.webp.

The url that the site accesses is the public file system mysites/files/styles/ then seems to append a path.

Is this what should be happening with the Convert function?

The test "testImageStylePrivateWithConversion" was failing since the "ImageStyleDownloadController" converted the image URI to "private://./image-test_0.png" instead of "private://image-test_0.png".

Original conversion code in "ImageStyleDownloadController":

$converted_image_uri = sprintf('%s://%s%s%s', $this->streamWrapperManager->getScheme($derivative_uri), $path_info['dirname'], DIRECTORY_SEPARATOR, $path_info['filename']);
      if (!$this->sourceImageExists($converted_image_uri, $token_is_valid)) {

To "reduce" the extensions correctly, a new utility function was created in image.module. This may not be the best place for it but I wanted it globally available. The original URI also needed to be reduced in the "image_file_download" hook so that it pass the correct original URI to the file_download hooks.
image.module:

function image_reduce_file_extensions($uri) {
  $converted_uri = $uri;
  $path_info = pathinfo(StreamWrapperManager::getTarget($uri));
  // Only convert the URI when the filename still has an extension.
  if (!empty($path_info['filename']) && pathinfo($path_info['filename'], PATHINFO_EXTENSION)) {
    $converted_uri = StreamWrapperManager::getScheme($uri) . '://';
    if (!empty($path_info['dirname']) && $path_info['dirname'] !== '.') {
      $converted_uri .= $path_info['dirname'] . DIRECTORY_SEPARATOR;
    }
    $converted_uri .= $path_info['filename'];
  }

  return $converted_uri;
}

@@ -216,10 +216,8 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
-      $headers += [
-        'Content-Type' => $image->getMimeType(),
-        'Content-Length' => $image->getFileSize(),
-      ];
+      $headers['Content-Type'] = $image->getMimeType();
+      $headers['Content-Length'] = $image->getFileSize();

There is a related/duplicate issue: #2786763: Incorrect headers for image derivative if effect "Convert" in use and file stored in private filesystem, which is trying to fix the headers in ImageStyleDownloadController::deliver() as well. I think we should remove the fix from this MR, so that we do not combine multiple fixes in one issue (otherwise we need an explanation, why it needs to be fixed together, because it seems to be unrelated to the original issue summary). If needed, this issue can be postponed on that fix. Thanks!

@poker10 - I reverted the file header changes in this issue and created new patches in #2786763: Incorrect headers for image derivative if effect "Convert" in use and file stored in private filesystem.

Attached is a static patch of the MR 4772 at commit 5d72a9763e.

Rebased the issue to run the test only

There was 1 error:
1) Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStylePrivateWithConversion
Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.
/builds/issue/drupal-2786735/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-2786735/vendor/behat/mink/src/WebAssert.php:130
/builds/issue/drupal-2786735/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:221
/builds/issue/drupal-2786735/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:135
/builds/issue/drupal-2786735/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 10, Assertions: 232, Errors: 1.

Which is good.

Did some manual testing next

Installed Media library
Updated Image bundle image field to use private folder vs public
Setup a private folder
Updated thumbnail image style to convert to JPEG
Created a Media object and the thumbnail failed to generate
Applied MR 4772
Refreshed the page and the thumbnail generated

Hiding patches to make it more clear for committers.

Nice to see some progress on this old bug. I've added some comments to the MR that can be addressed.

1) Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStylePrivateWithConversion
Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.

/var/www/html/web/vendor/behat/mink/src/WebAssert.php:794
/var/www/html/web/vendor/behat/mink/src/WebAssert.php:130
/var/www/html/web/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:221
/var/www/html/web/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:135
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Seems the test-only feature of gitlab doesn't work with javascript tests yet. But ran locally and confirmed the test failure.

Testing this out following the title, confirmed the problem and that the MR does fix it.

Left 1 small nitpicky comment on the MR.

Thanks LGTM!

We also need to add a test when someone uploads a file like with additional dots in the name not related to the convert plugin - ie something like: test.me.jpg or even test.jpg.jpg where the user has unnecessarily doubled the file extension.

I recreate the patch #39 to be compatible with Drupal 10.2.4.

Patch in comment #64 is working on Drupal 10.2.4 under Open Social 12.2.0.

Thank you.

The patch in #64 contains the fix from https://www.drupal.org/project/drupal/issues/2786763

But according to #49, #50, and #51, 2786763's fix should not be included.

hiding patches that are not from the MR

MR appears to have test failures.

A rebase may solve some of them.

@smustgrave I created a clean MR !7349 but there are still failures that are not related to the changes in this issue - https://git.drupalcode.org/issue/drupal-2786735/-/pipelines/138852/failures. Failed test appears to be "Drupal\Tests\image\Functional\ImageAdminStylesTest"

I am seeing the same errors on other MRs too - example https://git.drupalcode.org/issue/drupal-2786763/-/pipelines/138806/failures. - this appears to be different.

This may be related to this issue since the failure is for a private scheme test "Drupal\Tests\image\Functional\ImageAdminStylesTest:testPreviewImageShowInPrivateScheme"

Some more debugging: The changes in #3127116: Image styles - thumbnails are broken in config page when private file system is used to target the sample images is causing this test failure. This change was committed in February 2024 and it removes the scheme when the sample images are requested. See commit fcc1ba6.

Cause of test failure:
Failed test: Drupal\Tests\image\Functional\ImageAdminStylesTest::testPreviewImageShowInPrivateScheme
The following code that was added in #3127116: Image styles - thumbnails are broken in config page when private file system is used must be after the hook_file_download has been called since that hook is expected a URI and not a file path.

    // If it is default sample.png, ignore scheme.
    if ($image_uri === $sample_image_uri) {
      $image_uri = $target;
    }

Fix implemented:
- Moved the code that removes the scheme from $image_uri to after the hook_file_download call.
- Added a check for $image_uri !== $sample_image_uri when checking whether to test the converted image uri.

Test only feature showed the coverage

1) Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStylePrivateWithConversion
Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.
/builds/issue/drupal-2786735/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-2786735/vendor/behat/mink/src/WebAssert.php:145
/builds/issue/drupal-2786735/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:223
/builds/issue/drupal-2786735/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:137
/builds/issue/drupal-2786735/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 10, Assertions: 232, Errors: 1.

Issue summary solution appears slightly different then the fix if that could be updated

But solution seems fine and does solve the issue so probably fine to self RTBC after updating the IS.

the "Proposed resolution" in the issue summary has been updated to reflect the final solution

Committed and pushed 31f9e8a9c6 to 11.x and 36619bd1b4 to 10.3.x and 0299bec90b to 10.2.x. Thanks!