Add option to not upscale images that are too small to fix target scale

Changing all of the >= checks to >. This was causing issues for me where if the image is already scaled to the correct width or height, then it was still recalculating a new value. The comments in the patch seem to indicate that it should actually be just > and not >=.

This patch is experiencing the issue that was pointed out in #17. I'll work on a fix for it, but the patch on 21 and 23 don't work because we need to implement the transformDimensions method and correctly handle when we're not scaling the image.

And here is a lightly tested patch. Did some code re-organization to re-use the same logic for calculating the dimensions when the img tag is generated and when the image is actually resized.

@j_ten_man your patch #25 is missing the focal_point_post_update_upscale_scale_crop from patch #21. Please re-add this update and post an interdiff.txt comparing your changes to patch #21.

Here's the corrected patch with the interdiff.

@j_ten_man thanks for the updated patch. Altering the dimensions in transformDimensions is closer to how core does tit in \Drupal\image\Plugin\ImageEffect\ScaleImageEffect::transformDimensions().

The patch is missing the empty width and height check similar to \Drupal\image\Plugin\ImageEffect\ScaleImageEffect::transformDimensions(). This prevents division by zero errors when calculating the aspect ratios. Example: The blazy → module could call with the NULL width and height if it cannot get the dimensions, example and SVG.

The attached patch adds the empty check for the dimensions.

I'm seeing strange behavior with #28 when applied to 8.x-1.x.

My image style only has one effect: Focal Point Scale and Crop, width of 1200, height of 600, and upscale is set to "Don't upscale but still crop".

However, the image style preview page shows 800x400, and the label says "Focal Point Scale and Crop 800×400 - Upscaling: Don't upscale but still crop".

The second thing is that I'm not sure it's working correctly.

I have an image that is 1600x300. When it's passed through this image style it comes out as 600x300. Shouldn't it be 1200x300?

The attached patch fixes a major issue with the previous patches. The previous patches were altering the "width" and "height" in the $this->configuration in order to pass it to the parent ResizeImageEffect::transformDimensions(). However, altering the instance configuration will corrupt the entity cache for the image style since it caches the effects plugin instances in ImageStyle::effectsCollection. This can lead to unexpected results when multiple images are on the page using the same image style. The image style effect instance's configuration width and height will always be set to the last image's transformed dimensions.

@DamienMcKenna please re-test as this change may fix the odd behaviour that you were seeing.

Added the missing downsize case to the transformDimensions method.

Patch #31 doesn't give the desired result.

The image style preview page has an image of 800x600. When applying "Focal Point Scale and Crop 1400×1000 - Upscaling: Don't upscale but still crop", the image is still upscaled to 1400x1000, while I would expect it to return the original image (800x600).

@matthiasm11 I have updated the patch. Please re-test. There were a few other areas where the adjusted crop dimensions needed to be used (see the interdiff for a comparison).

This one works, thanks!

Really nice to see that this has been fixed, thank you so much to everyone who was working on this 💪

I did some minor tweaks like

• Using × instead of × unicode directly in Twig template
• Change upsale config select list label from If image is too small: to Upscaling behavior
• Fix indentation

#35 also works, thank you!

@yannickoo can we get MR for the same.

Thanks everyone!!

Having the "Upscale and crop" and "Don't upscale but still crop" options is very useful. However, I am wondering what is the expected behavior of "Don't upscale and don't crop" (no_upscale_no_crop) option regarding cropping? From its label, it seems that it should not crop the image. Right? Or maybe I'm missing something?

But with focal_point 2.1.0 and focal_point-3048398-37.patch → and no_upscale_no_crop option selected, it still crops the image:

Please see the config below:

uuid: 1fe0eeae-1be1-42ab-8d47-c8b8160ad6c1
langcode: en
status: true
dependencies:
  module:
    - focal_point
name: test
label: test
effects:
  ba78a9d7-fcd3-4acd-a764-bc0e3b4a3fc1:
    uuid: ba78a9d7-fcd3-4acd-a764-bc0e3b4a3fc1
    id: focal_point_scale_and_crop
    weight: 1
    data:
      width: 20
      height: 100
      crop_type: focal_point
      upscale: no_upscale_no_crop

Also, the "don't crop" part seems contradictory with focal_point module's mission:

Focal Point allows you to specify the portion of an image that is most important. This information can be used when the image is cropped or cropped and scaled so that you don't, for example, end up with an image that cuts off the subject's head.

If we do not intend to crop, can't we just use core's image_scale instead? It has an option to upscale or not.

MR 38 created from the patch #37 ✨ Add option to not upscale images that are too small to fit target scale Needs review . The patch 37 can be used for any composer builds and applies to the latest 2.x.

Just want to confirm, patch at #37 works perfectly with Drupal 10.3.10 and Focal Point 2.1.2. Would love to see this added to the module - it's essential.

Also, to follow up from @druplr #39, the option to Don't upscale and don't crop does work for me - as in if the original image is smaller than the Focal Point Crop and Scale size, then the image is left in its original pixel size/ratio. Not sure who might need this, but there might be a use case for it.