Crop API is not appending a hash when the image styles are converted to WEBP

That is expensive because it duplicates the amount of queries for every image that does not have a crop, it also doesn't work for other file extensions that could possibly be converted to webp as well.

Instead, a better approach would be to check if webp exists at the end and if so, only then do a lookup. Or even better, do a single lookup with an IN condition and both variants.

This patch with working for me. Thanks @kaszarobert!

Does this impact those using the contrib WebP module and not using core's "Convert" image style effect?

Implementation looks OK now. Uses camelCase for the variable name, doesn't really fit in with the rest in the same function (coding standards say you can use camelCase, but the whole file should be consistent). But that's minor.

A test for this would be good. based on our tests, this is also required for the core convert option, so it should be possible adjust/duplicate the existing tests when they also have a convert operation.

Confirming RTBC, in my case crop_file_url_alter was adding a h query parameter and then FileUrlGeneratorInterface::generateAbsoluteString was not adding the itok param because there was something already in there.

Hello there.

I'm using these two lines in settings.php:

$config['image.settings']['suppress_itok_output'] = TRUE;
$config['image.settings']['allow_insecure_derivatives'] = TRUE;

Does it matter for this patch?

Now, despite your patch, converting to WEBP still makes crop disappear (using a cropped image in a view).

D 10.2.2, Crop API 8.x-2.3, ImageWidgetCrop 8.x-2.4, Image Effects 8.x-3.6.

Thanks!

Szy.

Patch is working great, thanks!

I can confirm, patch from #4 works perfect, thanks!

I have this issue with conversion to PNG too.
Therefore I propose a more generic solution to pass the conversion type directly in to the function.
I haven't tested this patch with Webp-Files.

Hi,

I think I've found a solution that I think is ‘simpler’ (less intrusive and based on what the core does in generating thumbnails)., and it adress all converters

The main idea is to retrieve the code from the ImageStyleDownloadController::deliver :

    // Don't try to generate file if source is missing.
    if (!$this->sourceImageExists($image_uri, $token_is_valid)) {
      // If the image style converted the extension, it has been added to the
      // original file, resulting in filenames like image.png.jpeg. So to find
      // the actual source image, we remove the extension and check if that
      // image exists.
      $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']);
      if (!$this->sourceImageExists($converted_image_uri, $token_is_valid)) {

and inject this control in the crop::getCropFromImageStyleId method

In Drupal 10.3+ , we have a new static method to do that

$converted_original_uri = ImageStyleDownloadController::getUriWithoutConvertedExtension($original_uri);

I'm open a merge request to add this

I don't exactly know how it works, but the solution from #15 helped me out.

My problem was Cloudflare which was displaying the old image even after Focal point was saved (using Focal point essentially crops the image). And by adding the fix from #15 the image uri receives the missing ?h parameter.
I'm also converting the image to webp format which seems to be one of the reasons for this problem. Without webp conversion the ?h parameter appears, but with the patch I'm able to get the ?h parameter there even with webp conversion enabled.
I'm using Drupal 10.3.9 and Crop API 8.x-2.4.

Thanks @hartsak, I pass the ticket to RTBC

I create test to demonstrate the issue, created Draft MR #21 to run pipelines.
Also applied changes from MR #21 to MR #20 to test changes.

My suggestion to create followup to refactor CropFunctionalTest::doTestFileUriAlter().

$this->assertTrue(strpos($image_style_url, $shortened_hash) !== FALSE, 'The image style URL contains a shortened hash.');
// Suggestion.
$this->assertStringContainsString($shortened_hash, $image_style_url, 'The image style URL contains a shortened hash.');

Hide patch files.

MR20 works for me. +1

I created a patch file from the Merge request

#25 works for me.

I wasn't aware that this was an issue until I applied the patch in the merge request #21. This is working for us.

I made a few changes -- not really to the implementation, but more to how the code is organized. I'd love another look at this, if it passes muster I'll gladly commit it.

Looked over the MR, seems ok, left one note about the return type.

Having the hash calculation in one place should make it easier to exchange it, I think this would be a good case for using xxhash over md5, but that would invalidate all existing URL's and definitely shouldn't be done in this issue.

Thanks everyone. Auto-merged into 8.x-2.x!

@phenaproxima - I'm not able to apply the the merged in patch in our setup after the changes that were made . . . any recommendations or troubleshooting information I can pass along?

***edit

Update: Patch Application Resolved

After further investigation, I discovered the issue was due to patch ordering and compatibility with other active patches in my setup. Applying the following patches in this specific order resolved the problem:

"drupal/crop": {
  "JSON API integration": "https://git.drupalcode.org/project/crop/-/merge_requests/29.patch",
  "Crop types are not sorted in the image effect form": "https://git.drupalcode.org/project/crop/-/merge_requests/1.patch",
  "Make some coding standards improvements": "https://git.drupalcode.org/project/crop/-/merge_requests/6.patch",
  "Crop API is not appending a hash when the image styles are converted to WEBP": "https://git.drupalcode.org/project/crop/-/merge_requests/20.diff",
  "Improve performance for checking if the crop exists": "https://git.drupalcode.org/project/crop/-/merge_requests/15.patch"
}

This resolved the patch conflict and allowed the WEBP hash functionality to work as expected.
Thanks again for the support