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

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

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: 🐛 Incorrect headers for image derivative if file stored in private filesystem Needs work , 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 🐛 Incorrect headers for image derivative if file stored in private filesystem Needs work .

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

The last submitted patch, 51: 2786735-MR4772-5d72a9763e--20230926.diff, failed testing. View results →

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 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed 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 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed contains the fix from https://www.drupal.org/project/drupal/issues/2786763 🐛 Incorrect headers for image derivative if file stored in private filesystem Needs work

But according to #49 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed , #50 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed , and #51 🐛 Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem Fixed , 2786763's fix should not be included.

GaëlG → made their first commit to this issue’s fork.

hiding patches that are not from the MR

MR appears to have test failures.

A rebase may solve some of them.

recrit → closed merge request !4772

recrit → changed the visibility of the branch 11.x to hidden.

@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.
I am seeing the same errors on other MRs too - example https://git.drupalcode.org/issue/drupal-2786763/-/pipelines/138806/failures.

Debugging now, 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} 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 🐛 Image styles - thumbnails are broken in config page when private file system is used RTBC 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!

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