Add possibility to not upscale for "Scale and crop" effect

I see no reason this should wait for D8. Yes, it's a minor WTF to have the defaults for the two effects be different, but I agree it's worth it to maintain BC with current HEAD at this point in the cycle. On the other hand, it's a real WTF to be forced into upscaling an image, especially since upscaling is rarely what you want anyway. So I do think the patch is needed. It's working beautifully on Drupal Gardens, btw.

Grammar nitpicks follow:

+++ modules/image/image.admin.inc	5 Aug 2010 23:53:37 -0000
@@ -548,6 +548,27 @@ function image_scale_form($data) {
+ * Note that this is not a complete form, it only contains the portion of the

Comma should be a semicolon here. (I know this is copy/paste from another form's docblock; probably it should be changed as well.)

+++ modules/image/image.admin.inc	5 Aug 2010 23:53:37 -0000
@@ -548,6 +548,27 @@ function image_scale_form($data) {
+ * form for configuring the scale and crop options. Therefore it does not not

One too many nots.

+++ modules/image/image.admin.inc	5 Aug 2010 23:53:37 -0000
@@ -548,6 +548,27 @@ function image_scale_form($data) {
+    '#title' => t('Allow Upscaling'),

s/Upscaling/upscaling (to preserve Dries's sanity).

Powered by Dreditor.

This patch changes the UI, so I'm giving it the "string freeze" tag. Not sure if it still has any chance for D7 or not.

subscribing

I'm rerolling this with the doc changes I suggested above, and moving it to D8. It seems like a backport candidate though since it's BC.

Oops, with patch this time.

I missed a documentation hunk, and a spelling error, in the last reroll. Try this instead.

BTW, I have tested this, and it still works.

Ah, this drives me nuts! +1

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Reroll for d7 core move.

D8, some cleanup and firsts steps to add upscale testing.

We still need more tests. Temporally NR for testbot.

To clarify, for images smaller than the specified dimensions that do not have the same aspect ratio (say, Resize and Crop 400x300, source image is 160x90), is the correct behavior to not scale while still cropping to the aspect ratio, or to neither scale nor crop and leave the original image intact?

Leaving the original intact seems the most intuitive when upscaling is turned off, as the image is still constrained within the given dimensions, though I'm curious as to what others think.

@davidwatson -- it doesn't scale but does crop when not upscaling. I tend to agree with you, or at least that there should be an option not to crop when image isn't scaled.

Just applied Michaelfavia's patch in #11, seems to be working well with my D7 install.

What more tests are needed to make this needs review?

@coderintherye, it needs automated tests that assert the feature works as expected. The patch includes one additional unit test assertion, but it would be good to add some more coverage to confirm that the feature works through the UI and various dimensions behave as expected. You can look in the existing image module tests to get an idea how other features are tested.

Patch from #12 doesn't apply to Drupal 8. It's broken. Tried both methods.
Will try to recreate it manually
It shows following:

Checking patch core/includes/image.inc...
Checking patch core/modules/image/image.admin.inc...
Hunk #1 succeeded at 507 (offset -45 lines).
Hunk #2 succeeded at 845 (offset -55 lines).
Checking patch core/modules/image/image.effects.inc...
error: while searching for:
 *   with the following items:
 *   - "width": An integer representing the desired width in pixels.
 *   - "height": An integer representing the desired height in pixels.
 * @return
 *   TRUE on success. FALSE on failure to scale and crop image.
 * @see image_scale_and_crop()
 */
function image_scale_and_crop_effect(&$image, $data) {
  if (!image_scale_and_crop($image, $data['width'], $data['height'])) {
    watchdog('image', 'Image scale and crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => $image->toolkit, '%path' => $image->source, '%mimetype' => $image->info['mime_type'], '%dimensions' => $image->info['width'] . 'x' . $image->info['height']), WATCHDOG_ERROR);
    return FALSE;
  }

error: patch failed: core/modules/image/image.effects.inc:200
error: core/modules/image/image.effects.inc: patch does not apply
Checking patch core/modules/image/image.module...
Hunk #1 succeeded at 210 (offset -2 lines).
Checking patch core/modules/image/image.test...
error: core/modules/image/image.test: does not exist in index
Checking patch core/modules/simpletest/tests/image_test.module...
error: core/modules/simpletest/tests/image_test.module: does not exist in index

Rebuilded manually for drupal 8 with test.

#19: image-upscale-optional-with-test-872206-19.patch queued for re-testing.

#19: image-upscale-optional-with-test-872206-19.patch queued for re-testing.

Minor:

+    $this->assertTrue($calls['crop'][0][5], t('Upscale was passed correctly'));

Should be:

+    $this->assertTrue($calls['crop'][0][5], 'Upscale was passed correctly');

Made the minor change from #23.

Tagging this as work done during the global sprint.

+++ b/core/modules/image/image.admin.incundefined
@@ -485,7 +485,28 @@ function image_scale_form($data) {
+    '#description' => t('Let scale make images larger than their original size'),

This should have a period at the end of the sentence.

+++ b/core/modules/image/image.admin.incundefined
@@ -809,6 +830,23 @@ function theme_image_scale_summary($variables) {
+function theme_image_scale_and_crop_summary($variables) {
+  $data = $variables['data'];
+  if (!isset($data['upscale'])) {
+    $data['upscale'] = TRUE;
+  }
+  return theme('image_resize_summary', array('data' => $data)) . ' ' . ($data['upscale'] ? '(' . t('upscaling allowed') . ')' : '');

I think the whitespace inserted between the text returned by resize summary and 'upscaling allowed' should be contained in the if statement so that it's not present if upscaling is off.

So change this line to something like the following:

return theme('image_resize_summary', array('data' => $data)) . ($data['upscale'] ? ' (' . t('upscaling allowed') . ')' : '');

+++ b/core/modules/image/image.effects.incundefined
@@ -200,12 +200,18 @@ function image_crop_effect($image, $data) {
  *   with the following items:
  *   - "width": An integer representing the desired width in pixels.
  *   - "height": An integer representing the desired height in pixels.
+ *   - "upscale": A Boolean indicating that the image should be upscaled if the
+ *     dimensions are larger than the original image.

Do we need to indicate that this is optional and can be left out, and will be defaulted to TRUE if omitted?

+++ b/core/modules/image/image.effects.incundefined
@@ -200,12 +200,18 @@ function image_crop_effect($image, $data) {
+  if (!isset($data['upscale'])) {
+        $data['upscale'] = TRUE;
+  }
+  if (!image_scale_and_crop($image, $data['width'], $data['height'], $data['upscale'])) {

Super picky, but there are a handful of these that I think should be abbreviated to:

$data['upscale'] = !isset($data['upscale']) : TRUE : FALSE;

And as per #17 this still needs functional tests.

Powered by Dreditor.

I'm quite confused as I think all of this code has already made it into D8. Please confirm what actually needs to be done. Maybe just the backport?

This has not made it into D8 yet as far as I can see.

@tidrif, the patch from #24 is against D8 but is obsolete. The codebase of D8 had dramatically changed since then.

Claudiu.cristea, yes the patch #24 is 10 months old and D8 has changed a lot since then. And the patch has not been added to D8.

I am working on creating a patch against the latest D8.

Adding a reroll as per the latest D8 snapshot.

If this hasn't made it into Drupal 8 yet, it's unfortunately not going to (see https://drupal.org/core/release-cycle).

Actually, it looks like that page is out of date as of #2135189: Proposal to manage the Drupal core release cycle. We will continue to ship new features like this in "minor" releases of Drupal 8.

+    if (!$upscale && $scale >= 1) {
+      $width = $width / $scale;
+      $height = $height / $scale;
+      $scale = 1;
+    }

Why do we do this? If the scale is bigger than 1, i think we should return TRUE without modifying the image.

1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -192,11 +192,14 @@ public function scale(ImageInterface $image, $width = NULL, $height = NULL, $ups
   +   *   (optional) Boolean indicating that files smaller than the dimensions will
   ...
   +   * @param bool $upscale
   ...
   +   *   be scaled up. This generally results in a low quality image.
   

   If is optional add the default to documentation. Add "Defaults to TRUE." at the end.

2. +++ b/core/modules/image/image.admin.inc
   @@ -238,6 +238,25 @@ function theme_image_scale_summary($variables) {
   + * @param $variables
   

   You need to add the type of param.

   @param array $variables
   

3. +++ b/core/modules/image/image.admin.inc
   @@ -238,6 +238,25 @@ function theme_image_scale_summary($variables) {
   +  $data['upscale'] = !isset($data['upscale']) ? TRUE : FALSE;
   

   This seems wrong. Maybe you wanted:

   $data['upscale'] = isset($data['upscale']) ? $data['upscale'] : TRUE;
   

4. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -24,11 +24,44 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +  ¶
   

   Remove trailing spaces.

Hi,

I modified the patch from #31. Removed the trailing spaces, modified the dimensions calculations, I modified the render summary to be french language compliant (we need the parenthesis in the translatable text) and fixed the other issues mentioned in #35.

forgot to change the status

The earlier patch seems to have failed some tests. I have made the following changes to the patch from #31:

• When upscaling is False and scale > 1, just using(return TRUE) the original image.
• suggestions mentioned above have been incorporated.
• Added similar logic while calculating dimensions. i.e return original dimensions if scale > 1 and upscaling is false.
  This was missing in all of the earlier patches inturn causing an issue where the dimensions in the html image tag were different from the image displayed.
• I have run the tests locally and it passed.

Hi,

Did you run the Unit tests also? I'm working on this patch from the morning and I can't get some unit tests working. Are you available on IRC to talk about this?

39: image-upscale-optional-with-test-872206-39.patch queued for re-testing.

I think it would be better to use my patch from #37, cause it is failing only in Imagetest unit tests. you don't resolve anything if you mark queue again the same patch file for re-testing.
I'm online on IRC #drupal-contribute if you want to discuss about your patch or we can discuss here also.

Hi skipyT,

I have queued for re-testing because, it failed because of some memory issue which might not be because of the patch.
and yes I ran the unit tests as well.
I am available on irc for another 30 mins..lets discuss..
my irc nick is : _rcp

i tested patch from #39 and it works for me

mathes, thanks for testing.
the patch in #39 passed the test as well. hope someone can review and mark it RTBC or commit if everything is fine.

+++ b/core/modules/image/image.admin.inc
@@ -238,6 +238,25 @@ function theme_image_scale_summary($variables) {
+  $data['upscale'] = isset($data['upscale']) ? $data['upscale'] : TRUE;

Is this ok? I think if the configuration is empty we shoudn't display the (upscaling allowed) text.

The others parts are ok for me.

Yes,
the default is true i.e when upscale is not set/defined it falls back to default which is TRUE in our case. this is the same logic we have implemented/used for the main scaleandcrop feature as well.

I just checked the theme functions for the other effects and the theme_image_crop_summary is just calling the theme_resize_summary, perhaps we shall do the same for scale and crop, to call the scale summary to avoid code duplication. what do you think?

Hi skipyT, thank you for the review.
yes, we can do that and call theme_image_scale_summary but that inturn calls theme_image_resize_summary i.e one more additional function call. I'm not sure which is better. So adding patches with both the cases.
And I've tested #48 again. Even though the code/logic is correct, we do not need to do it because 'upscale' will be set always in configurations as we have already handled the default configuration for upscale.

872206-51.patch :

• Modified theme_image_scale_summary which was adding a white space in case of upscaling false.
• Removing setting default value for 'upscale' variable in theme function which is not necessary.
• interdiff-872206-39-51.txt will give the interdiff between #39 and #51

872206-51-a.patch :

• Modified theme_image_scale_summary which was adding a white space in case of upscaling false.
• Removing setting default value for 'upscale' variable in theme function which is not necessary.
• Calling the 'theme_image_scale_summary' as suggested in #50
• interdiff-872206-39-51-a.txt will give the interdiff between #39 and #51-a.patch

51.patch vrs 51-a.patch : which one to use ?
The theme function 'theme_image_scale_and_crop_summary' calls 'theme_image_resize_summary' + (add upscale text if applicable) , which is same as what function 'theme_image_scale_summary' does.
Use 51-a.patch if additional function call is better than duplicate code(1 line). Otherwise use 51.patch
interdiff-872206-51-51-a.txt gives the diff between 51.patch and 51-a.patch.

I think, we have covered all possible glitches now :)

I definitively like the 51-a patch, I tested it and it seems ok, marked as RTBC.

@33: I stand corrected. :] Should we create a new issue to update those docs? Unless my memory/understanding fails me, point releases weren't used for new minor features in the past, which may be a source of confusion for those not in the know.

51: image-upscale-optional-with-test-872206-51-a.patch queued for re-testing.

Been in RTBC for more than 4 weeks.

51: image-upscale-optional-with-test-872206-51-a.patch queued for re-testing.

still green.

reviewing 51a since that is the one seems preferred here.
--

1. +++ b/core/lib/Drupal/Component/Utility/Image.php
   @@ -60,5 +60,4 @@ public static function scaleDimensions(array &$dimensions, $width = NULL, $heigh
        $dimensions['height'] = $height;
        return TRUE;
      }
   -
    }
   

   this is the only change in this file.
   this standards fix is out of scope of this issue.

   taking out.

   (if we want to do it, we would in another patch that had to change those lines anyway, or in something like #1518116: [meta] Make Core pass Coder Review, #1533246: Make image module pass Coder Review)

   doh! this was actually taking out the space that is standard according to
   https://drupal.org/node/608152#indenting

   anyway reverting this out of scope and incorrect change.

2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -192,11 +192,15 @@ public function scale(ImageInterface $image, $width = NULL, $height = NULL, $ups
   +   * @param bool $upscale
   +   *   (optional) Boolean indicating that files smaller than the dimensions will
   +   *   be scaled up. This generally results in a low quality image. Defaults
   +   *   to TRUE.
   

   a while ago we go into a habit of being very verbose with param descriptions, repeating information that was in the function definition.

   The docs requirements examples were corrected in https://drupal.org/node/1354#param
   "Optional parameters are indicated by (optional); include information about the default value only if it is not obvious from the function signature."

   fixed.

3. +++ b/core/modules/image/image.admin.inc
   @@ -223,7 +223,7 @@ function theme_image_resize_summary($variables) {
   - * @param $variables
   + * @param array $variables
   

   this change is out of scope of this issue.

   reverted.

   If this change were to be made, it would be because we had to change this line to make this patch function correctly, or as part of #1326608: Clean up API docs for image module. no that was too many little clean ups. I think we have something about correctly type hints and @param types. ... ok. I cant find such and issue, we could ping @jhodgdon to find out, or assume it is not appropriate right now, and we are just correcting this type of thing when we are in issue that already have to fix such lines for other reasons. Assuming we dont have a dedicated type hint @param related issue. Let's not do this unrelated change here.)

4. +++ b/core/modules/image/image.admin.inc
   @@ -234,7 +234,24 @@ function theme_image_scale_summary($variables) {
   + * @param $variables
   + *   An associative array containing:
   + *   - data: The current configuration for this scale and crop effect.
   ...
   +function theme_image_scale_and_crop_summary($variables) {
   

   heh. for example, here.
   this line is *added* by this patch.

   we should add it correctly, with the type in the param and the function declaration.

   fixed.

5. +++ b/core/modules/image/image.admin.inc
   @@ -234,7 +234,24 @@ function theme_image_scale_summary($variables) {
   + * @ingroup themeable
   

   hm. function theme_....()
   are we still adding theme_ functions?

   searched to find:
   #1757550: [Meta] Convert core theme functions to Twig templates

   I think this addition of theme_ is ok.
   but we should note that #1898420: image.module - Convert theme_ functions to Twig would need a reroll if this were committed first, or this would need a reroll if the other were committed.

   adding it as a related issue.

6. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -24,11 +24,63 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +      // When upscale is false and scale > 1, image will have original dimensions.
   

   nit (would not mark needs work for this on it's own, but since there are other core gates, fixing)

   this is > than 80 chars.
   "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, "
   https://drupal.org/node/1354#drupal

7. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageEffectsTest.php
   @@ -109,6 +109,7 @@ function testScaleAndCropEffect() {
   +    $this->assertTrue($calls['scaleAndCrop'][0][3], 'Upscale was passed correctly');
   
   +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitTest.php
   @@ -80,7 +80,7 @@ function testScale() {
   -    $this->assertTrue($this->image->scaleAndCrop(5, 10), 'Function returned the expected value.');
   +    $this->assertTrue($this->image->scaleAndCrop(5, 10, FALSE), 'Function returned the expected value.');
   
   @@ -88,6 +88,7 @@ function testScaleAndCrop() {
   +    $this->assertFalse($calls['scaleAndCrop'][0][3], 'Upscale was passed correctly');
   

   these is the only changes to tests.

   seems like we need more here.

--
summary, tagging needs tests. we at least need a close look at what we might be able to test here from someone more familiar with tests involving image effects.

should be needs work for that.

this could also use an issue summary update and before and after pictures of the UI changes. tagging.

Before screenshots attached (included screenshot of Scale showing the "Allow upscaling" option that's missing from Scale and Crop).

Patch does not apply.

As mentioned in #62, patch in #59 no longer applies.

Please review updated patch which applies cleanly.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -155,8 +155,9 @@ public function save(ImageInterface $image, $destination);
+   *   (optional) Upscale option that indicates whether files smaller than the

'images', not 'files'

+++ b/core/modules/image/image.admin.inc
@@ -195,3 +195,20 @@ function template_preprocess_image_anchor(&$variables) {
+/**
+ * Returns HTML for a summary of an image scale and crop effect.
+ *
+ * @param array $variables
+ *   An associative array containing:
+ *   - data: The current configuration for this scale and crop effect.
+ *
+ * @ingroup themeable
+ */
+function theme_image_scale_and_crop_summary(array $variables) {
+  $image_scale_summary = array(
+    '#theme' => 'image_scale_summary',
+    '#data' => $variables['data'],
+  );
+  return drupal_render($image_scale_summary);
+}

After image module conversion to Twig, we should not really be introducing new theme_*() functions. Please convert this to a Twig template, or maybe reuse the existing image-scale-summary.html.twig one?

+++ b/core/modules/image/image.module
@@ -138,6 +138,10 @@ function image_theme() {
+    'image_scale_and_crop_summary' => array(
+      'variables' => array('data' => NULL),
+      'file' => 'image.admin.inc',
+    ),

same here, use a 'template' key intead of a 'file' one. See existing examples.

+++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
@@ -24,11 +24,64 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
+  /**
+   * {@inheritdoc}
+   */
+  public function getSummary() {
+    return array(
+      '#theme' => 'image_scale_and_crop_summary',
+      '#data' => $this->configuration,
+    );
+  }

Let's make this aligned with other effects, i.e. add the parent::getSummary, like (from ScaleImageEffect)

  /**
   * {@inheritdoc}
   */
  public function getSummary() {
    $summary = array(
      '#theme' => 'image_scale_summary',
      '#data' => $this->configuration,
    );
    $summary += parent::getSummary();

    return $summary;
  }

Note that this will impact #2073759: Convert toolkit operations to plugins which IMHO has a priority now.

Tests are adjusted so I am removing the 'Needs tests' tag.

Reworked patch.

Patch no longer applies. It needs reroll.

Here is the updated patch which fixes this feature.

Updating the patch with the reroll changes.

Note that this issue is filed as a feature request, so it is bound by issue thresholds. We currently have 100 criticals and 640 majors.

@alexpott pointed out that the patch in #70 includes some out-of-scope changes in node.api.php.

Rerolled the patch and removed the changes to node.api.php.

Working on other things at the sprint for now, if anyone else wants to take a crack at this.

As a feature request, per the beta changes policy, this is an 8.1.x issue now. Sorry little patch. :(

This can be active against the future development minor (currently 8.2.x).

Hi,

Ran this against 8.2.x (but patch still applies to 8.1.x) based from the last patch by @rootwork with a few variation.

Variations:
Set upscale to FALSE by default.
Uses 1 instead of TRUE for upscaling testing. This is for uniformity sake as seen in scale effect.
Removed template in hook_theme for style summary.
Adding actual template file.
And other small variations.

I replicated scale and crop for the 'defaults' and other stuffs.

Thanks.

I will make further updates to address issues.

This is an update against:
https://www.drupal.org/node/872206#comment-10931269 && https://www.drupal.org/pift-ci-job/200361

I am not entirely sure how to address the remaining test that fails:

07:36:48 Drupal\KernelTests\Core\Theme\StableTemplateOverrideTest 0 passes 1 fails

Will need help from anyone who has an idea to fix this.

I guess I spoke too soon.

I updated against https://www.drupal.org/node/872206#comment-10931367 and added a theme override for image-scale-and-crop-summary.html.twig (new template to accommodate requirement of this ticket) in the core 'stable' theme.

This needs a re-roll as the patch no longer applies. Sorry this is a bit long in the tooth(old), but seems like it could be tackled at the sprint.

Hi i'm at drupal con dublin. I'm looking at rerolling this now

Rerolled patch on 8.3.x
Fixed merge conflict in migrate test

Please review

Removing the reroll tag as that's been taken care of.

Last patch doesn't really respect $upscale setting:

+++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
@@ -19,11 +21,79 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
+  public function transformDimensions(array &$dimensions, $uri) {
+    if ($dimensions['width'] && $dimensions['height']) {
+      Image::scaleDimensions($dimensions, $this->configuration['width'], $this->configuration['height'], $this->configuration['upscale']);
+    }
+    $dimensions['width'] = $this->configuration['width'];
+    $dimensions['height'] = $this->configuration['height'];
+  }

Dimensions would be set anyway, even if image wasn't upscaled. So I added condition that checks the result of Image::scaleDimensions() function. This will guarantee that image with smaller dimensions will save original ones.

  public function transformDimensions(array &$dimensions, $uri) {
    if ($dimensions['width'] && $dimensions['height']) {
      $is_scaled = Image::scaleDimensions($dimensions, $this->configuration['width'], $this->configuration['height'], $this->configuration['upscale']);

      // If image is not scaled save the original dimensions.
      if (!$is_scaled) {
        return TRUE;
      }
    }
    $dimensions['width'] = $this->configuration['width'];
    $dimensions['height'] = $this->configuration['height'];
  }

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -188,8 +188,8 @@ public function rotate($degrees, $background = NULL) {
   +    return $this->apply('scale_and_crop', array('width' => $width, 'height' => $height, 'upscale' => $upscale));
   

   It would be more readable if you put every array item to its own line.

   Also, what about to change old array syntax to shorten?

2. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,84 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +    if ($dimensions['width'] && $dimensions['height']) {
   

   Do we need to have isset() here?

3. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,84 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +        return TRUE;
   

   I guess method must return single type. Currently we have NULL and boolean.

4. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,84 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +    $summary += parent::getSummary();
   

   Let's do not merge array in that way. Just save value of parent::getSummary() into variable and override what you need.

Changed according to review notes.

Still needs:

• IS update
• Screenshots

And I'm adding "Needs change record".

Few comments...

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -139,7 +139,7 @@ public function save($destination = NULL) {
   -  public function apply($operation, array $arguments = array()) {
   +  public function apply($operation, array $arguments = []) {
   
   @@ -147,56 +147,77 @@ public function apply($operation, array $arguments = array()) {
   -    return $this->apply('create_new', array('width' => $width, 'height' => $height, 'extension' => $extension, 'transparent_color' => $transparent_color));
   +    return $this->apply('create_new', [
   +      'width' => $width,
   +      'height' => $height,
   +      'extension' => $extension,
   +      'transparent_color' => $transparent_color,
   +    ]);
   ...
   -    return $this->apply('convert', array('extension' => $extension));
   +    return $this->apply('convert', ['extension' => $extension]);
   ...
   -    return $this->apply('crop', array('x' => $x, 'y' => $y, 'width' => $width, 'height' => $height));
   +    return $this->apply('crop', [
   +      'x' => $x,
   +      'y' => $y,
   +      'width' => $width,
   +      'height' => $height,
   +    ]);
   ...
   -    return $this->apply('desaturate', array());
   +    return $this->apply('desaturate', []);
   ...
   -    return $this->apply('resize', array('width' => $width, 'height' => $height));
   +    return $this->apply('resize', ['width' => $width, 'height' => $height]);
   ...
   -    return $this->apply('rotate', array('degrees' => $degrees, 'background' => $background));
   +    return $this->apply('rotate', [
   +      'degrees' => $degrees,
   +      'background' => $background,
   +    ]);
   ...
   -    return $this->apply('scale', array('width' => $width, 'height' => $height, 'upscale' => $upscale));
   +    return $this->apply('scale', [
   +      'width' => $width,
   +      'height' => $height,
   +      'upscale' => $upscale,
   +    ]);
   
   +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,85 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   -      $this->logger->error('Image scale and crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => $image->getToolkitId(), '%path' => $image->getSource(), '%mimetype' => $image->getMimeType(), '%dimensions' => $image->getWidth() . 'x' . $image->getHeight()));
   ...
   +      $this->logger->error('Image scale and crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', [
   +        '%toolkit' => $image->getToolkitId(),
   +        '%path' => $image->getSource(),
   +        '%mimetype' => $image->getMimeType(),
   +        '%dimensions' => $image->getWidth() . 'x' . $image->getHeight(),
   +      ]);
   

   Unrelated. Please revert them.

2. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -163,11 +163,14 @@ public function scale($width, $height = NULL, $upscale = FALSE);
   +   * @param bool $upscale
   +   *   (optional) Boolean indicating that files smaller than the dimensions will
   +   *   be scaled up. This generally results in a low quality image.
   

   On optional params, we document the default value. At the end add: "Defaults to FALSE.".

3. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,85 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +    $summary = parent::getSummary();
   ...
   +    $summary['#data'] = $this->configuration;
   +    return $summary;
   

   More compact?

   return [
     '#theme' => 'image_scale_and_crop_summary',
   ] + parent::getSummary();
   

4. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,85 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +      '#title' => t('Allow Upscaling'),
   +      '#description' => t('Let scale make images larger than their original size'),
   

   s/t()/$this->t()/

5. +++ b/core/modules/image/tests/src/Kernel/Migrate/d7/MigrateImageStylesTest.php
   @@ -32,7 +32,7 @@ protected function setUp() {
   +    $this->assertEntity('custom_image_style_1', "Custom image style 1", ['image_scale_and_crop', 'image_desaturate'], [['width' => 55, 'height' => 55, 'upscale' => false], []]);
   

   s/false/FALSE

6. +++ b/core/modules/image/tests/src/Kernel/Migrate/d7/MigrateImageStylesTest.php
   @@ -32,7 +32,7 @@ protected function setUp() {
   diff --git a/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig b/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig
   
   diff --git a/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig b/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig
   new file mode 100644
   
   new file mode 100644
   index 0000000..a5190e2
   
   index 0000000..a5190e2
   --- /dev/null
   
   --- /dev/null
   +++ b/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig
   
   +++ b/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig
   +++ b/core/themes/stable/templates/admin/image-scale-and-crop-summary.html.twig
   @@ -0,0 +1,35 @@
   

   I see no difference between the module based theme and the "stable" theme implementation. Then why we need this duplication? Just curious, because I'm not strong enough in theming :)

1. Ok, let's not make this changes here.
2. Done
3. Agree that $summary['#data'] = $this->configuration; is redundant here.
   As for union I'd rather agree with @BR0kEN, no need to perform operation while we can omit it, event though it's shorter.
4. Done
5. Done
6. I'm not sure too, probably to make kinda bootstrap theme from 'stable', so you could just copy it and start to override whatever you need.

I've created Draft change record, please review https://www.drupal.org/node/2825390

Is it ready now? I guess. I'd love to have this in core.

Thank you. It starts to look good. However, some small changes are still required. The most important is that it needs an upgrade path to fix existing image styles that contain this effect with the default value.

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -188,8 +188,8 @@ public function rotate($degrees, $background = NULL) {
   +    return $this->apply('scale_and_crop', array('width' => $width, 'height' => $height, 'upscale' => $upscale));
   

   s/array()/[]

2. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,85 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +    $summary['#data'] = $this->configuration;
   

   This is useless. The parent is doing this already.

3. +++ b/core/modules/image/config/schema/image.schema.yml
   @@ -76,6 +76,10 @@ image.effect.image_desaturate:
   +  mapping:
   +    upscale:
   +      type: boolean
   +      label: 'Upscale'
   

   This needs an upgrade path. We need to update existing image style configurations that have 'scale and crop' effects by adding the default value (which is FALSE).

Looking into the upgrade path - I need something similar for Image Effects, #2702205-16: Text overlay: add an hook to alter the overlaid text and use it to enable setting maximum number of characters displayed

#105:

1. done
2. done
3. I added an update function to refresh all image styles with the new 'upscale' parameter. I do not know how to write a test for that, though. @claudiu.cristea is there any example you can point to?

Unassigning.

+++ b/core/modules/image/image.install
@@ -61,3 +63,32 @@ function image_requirements($phase) {
+function image_update_8301() {

This should not live here. This is more suitable for a post_update update because it uses the full API (ie it doesn't have to repair the database). See https://www.drupal.org/node/2563673.

See \Drupal\image\Tests\Update\ImageUpdateTest for a test example.

I think the test can be added to \Drupal\image\Tests\Update\ImageUpdateTest because setDatabaseDumpFiles() would be the same.

#110: The shipped image styles, though, do not contain any 'Scale and crop' effects. We would need to add a test style with a 'Scale and crop' effect as it can be configured before this patch is applied, so that we can test the update function and ensure that it adds the 'upscale' parameter. How to do that?

Bad. Then you'll have to apply some fixtures to the shipped DB. See \Drupal\views\Tests\Update\BooleanFilterValuesUpdateTest::setDatabaseDumpFiles() for an example. Then you'll need to add the test in a new file/class. In the fixture file you'll need to add the effect on the lowest API level you can. If you use the image style API it will automatically add the default 'upscale', because it will use the current code API, but you don't want that. So probably you want to read a style directly from DB {config} table, add your effect (w/o upscale) and write it back in DB.

Only moved the update from image.install to image.post_update.php, as per #108.

Update test missing, #112 looks tough...

@claudiu.cristea, I am not agree that we need to use short array syntax in exact place. Everywhere, in whole file, array() used and we need to change them all or go ahead with old variant.

s/array()/[]

@BR0kEN, there's no rule it should apply to whole file. The Drupal coding standards page states about short syntax:

When used, try to apply it consistently to at least a whole method or function.

So, not "whole file".

See: https://www.drupal.org/docs/develop/standards/coding-standards#array

@claudiu.cristea, not sure that this make sense. It looks strange when different parts of the same file uses different coding standards. Why not to create separate issue for this?

@BR0kEN because that is new code. If we touched that line and we can convert the array to the new syntax, by following the rules that I mentioned in #116, then we do it. If we only edit that line to convert the array then is out-of-scope. But we already touched that line to add 'upscale' => $upscale, so it's OK to change it.

Giving a try to upgrade path test, based on instructions in #112.

See this.

Looks nice!

+++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
@@ -0,0 +1,45 @@
+ * @group Update

It's more about 'image': @group image

+++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
@@ -0,0 +1,45 @@
+    $image_style = ImageStyle::load('test_scale_and_crop_upscale');
...
+    $effect = $image_style->getEffect('637b75a0-a80a-4bcc-8300-66994a27871d');

When you use $image_style->getEffect(), the API will apply the defaults and 'upscale' will be added with the default. You should use the config system:

$effect_data = $this->config('image.style.test_scale_and_crop_upscale')->get('effects.637b75a0-a80a-4bcc-8300-66994a27871d.data');
$this->assertFalse(array_key_exists('upscale, $effect_data));

+++ b/core/modules/image/image.post_update.php
@@ -20,3 +21,32 @@ function image_post_update_image_style_dependencies() {
+        $configuration = $effect->getConfiguration();
+        $configuration['data']['upscale'] = FALSE;
+        $effect->setConfiguration($configuration);
+        $edited = TRUE;
...
+    if ($edited) {
+      $image_style->save();
+    }

For the same reason, it should be possible that we skip this sequence and simply...

$image_style->save();
break;

because, probably, the 'upscale' is automatically added with FALSE on load. To be tested!

EDIT: Should work:

foreach (ImageStyle::loadMultiple() as $image_style) {
  foreach ($image_style->getEffects() as $effect) {
    if ($effect->getPluginId() === 'image_scale_and_crop') {
      $image_style->save();
      break;
    }
  }
}

Thanks @claudiu.cristea! Let's see what bot thinks.

+++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
@@ -0,0 +1,43 @@
+use Drupal\image\Entity\ImageStyle;

Probably this is unused now.

#126: indeed...

Looks good to me.

1. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,84 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +      $is_scaled = Image::scaleDimensions($dimensions, $this->configuration['width'], $this->configuration['height'], $this->configuration['upscale']);
   +
   +      // If image is not scaled save the original dimensions.
   +      if (!$is_scaled) {
   

   I would also simplyfy this.

2. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,84 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +  public function getSummary() {
   +    return [
   +      '#theme' => 'image_scale_and_crop_summary',
   +    ] + parent::getSummary();
   +  }
   ...
   +  public function defaultConfiguration() {
   +    $configuration = parent::defaultConfiguration();
   +    $configuration['upscale'] = FALSE;
   +    return $configuration;
   +  }
   

   If we have already changed this lines ten let's cast it to one style.

3. +++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
   @@ -0,0 +1,42 @@
   +    $this->assertTrue(array_key_exists('upscale', $effect_data));
   +    $this->assertFalse($effect_data['upscale']);
   

   Assertion doesn't guarantee that $effect_data['upscale'] exists. It will raise a notice, let's avoid it.

Some more things.

1. +++ b/core/modules/image/image.post_update.php
   @@ -20,3 +21,27 @@ function image_post_update_image_style_dependencies() {
   +        break;
   +
   

   Redundant newline

2. +++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
   @@ -0,0 +1,46 @@
   +      $this->fail("Confirming that 'upscale' parameter is updated properly");
   

   Oops, my bad mistake in fail message.

#129.1; Disagree with that. Now the line is too long. It's unreadable.

+1 for #129.2.

#129.3: Then:

if ($this->assertTrue(array_key_exists('upscale', $effect_data))) {
  $this->assertFalse($effect_data['upscale']);
}

@claudiu.cristea

1. It was longer than now.
2. -
3. No need additional assertion since below it'll be covered by $this->fail().

OK for 1.

+++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
@@ -0,0 +1,46 @@
+    if (array_key_exists('upscale', $effect_data)) {
+      $this->assertFalse($effect_data['upscale']);
+    }
+    else {
+      $this->fail("Confirming that 'upscale' parameter is updated properly");
+    }

This is an unusual way to assert that the key 'upscale' exists in an array. If we can use an assertion, why not using it?

@claudiu.cristea, it's unusual, I agree. But more safe for preventing PHP notice, when key will not be presented in array.

#134

more safe for preventing PHP notice

we usually do not care about that in test code. If it fails, it fails. Also, $this->fail() with a message is kind of obsolete as we are trying to remove, in general, assertion messages from tests that are being more and more migrated to phpunit. I still prefer

+++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
@@ -0,0 +1,42 @@
+    $this->assertTrue(array_key_exists('upscale', $effect_data));
+    $this->assertFalse($effect_data['upscale']);

it's more readable. If the first fails the assertion then we are already facing a failed test, then why caring if the second raises a notice?

@mondrake, I was 100% sure that someone will affect a theme regarding notices/warning/exceptions etc. in test code and tell that we may ignore them. But I think better to have fails/passes instead of them. If we can omit language-level warnings and having tools to do this, why we have to choose other way?

Not so unusual way of assertion, actually. Quite similar lines can be found in core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php

      if (strpos($generate_url, '.png') === FALSE ) {
        $this->fail('Confirming that private image styles are not appended require PNG file.');
      }
      else {
        // Check for PNG-Signature (cf. http://www.libpng.org/pub/png/book/chapter08.html#png.ch08.div.2) in the
        // response body.
        $this->assertNoRaw( chr(137) . chr(80) . chr(78) . chr(71) . chr(13) . chr(10) . chr(26) . chr(10), 'No PNG signature found in the response body.');
      }

And, yes - if we can prevent notice in such a simple way, why not to do this?

@BR0kEN, @l0ke, so let's agree we disagree :)

Moving back to RTBC, let's not hold the entire patch on such a minor point, and see the opinion of a core committer.

No, you cannot RTBC as you have contributed to the code. The test failures should not be fancy. The tests should be easy to read and understand. For this reason we have assertions and we are not using conditions that are calling pass() or fail().

So this should be reverted acording to #135 or #131. Note that in #131, assertTrue() returns a boolean, avoiding the notice. But I still stand with #135.

@ claudiu.cristea ok, as something in the middle of our point of view, do you think variant from #131 would be good enough for RTBC?

Looks like an unrelated failure.

I opened this #2828045: Tests failing with unrelated „Translation: The French translation is not available”.

+++ b/core/lib/Drupal/Core/Image/ImageInterface.php
@@ -163,11 +164,15 @@ public function scale($width, $height = NULL, $upscale = FALSE);
+  public function scaleAndCrop($width, $height, $upscale = FALSE);

We don't allow adding parameters to interfaces usually because it would break any direct implementations of the interface.

So there's two ways to deal with this:

1. If we're very confident that no-one is directly implementing ImageInterface and would only subclass Image, then it could be done in a minor as @internal - but this isn't a bug fix and Image isn't a base class, so not sure that'll work.

2. Create a new interface with a new method (scaleAndCropUpscale()?scaleCrop()?) extending from ImageInterface then have image implement the new interface.

Moving back to CNR for now. Didn't do a full review of the rest of the patch yet.

#152: do we need such a new method at all? After long discussion, these methods on ImageInterface were introduced in #2073759: Convert toolkit operations to plugins:

For DX convenience, core methods scale(), scaleAndCrop(), resize(), crop(), rotate(), desaturate() are reintroduced in ImageInterface and Image, as specific proxies to the canonical apply() call.

in order to keep some sort of backwards compatibility. But we could call in the effect

$image->apply('scale_and_crop', ['width' => $this->configuration['width'], 'height' => $this->configuration['height'], 'upscale' => $this->configuration['upscale']]);

instead of

$image->scaleAndCrop($this->configuration['width'], $this->configuration['height'], $this->configuration['upscale']);

and avoid the need to change interface or add more methods.

BTW - is the patch actually doing what is meant to do? I do not see any change to the GD toolkit that would somehow manage the newly introduced 'upscale' argument. Needs manual testing I guess.

#152
1. #153 makes sense
2. Looks a bit odd and overcomplicated

#153
Patch is doing what is has do and it was tested. No need to make any changes in GD, this option already exists, see example in Drupal\image\Plugin\ImageEffect\ScaleImageEffect

  public function transformDimensions(array &$dimensions, $uri) {
    if ($dimensions['width'] && $dimensions['height']) {
      Image::scaleDimensions($dimensions, $this->configuration['width'], $this->configuration['height'], $this->configuration['upscale']);
    }
  }

Just did the same in ScaleAndCropImageEffect

+++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
@@ -19,11 +21,82 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
+      if (!Image::scaleDimensions($dimensions, $this->configuration['width'], $this->configuration['height'], $this->configuration['upscale'])) {

#154: then, why do we need to pass the 'upscale' parameter to the ImageToolkitOperation if that is not used?

@mondrake, do not change the status of issue during the discussion. Parameter is used, check \Drupal\Component\Utility\Image::scaleDimensions().

@l0ke,

1. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,82 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
      public function applyEffect(ImageInterface $image) {
   -    if (!$image->scaleAndCrop($this->configuration['width'], $this->configuration['height'])) {
   -      $this->logger->error('Image scale and crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => $image->getToolkitId(), '%path' => $image->getSource(), '%mimetype' => $image->getMimeType(), '%dimensions' => $image->getWidth() . 'x' . $image->getHeight()));
   +    if (!$image->scaleAndCrop($this->configuration['width'], $this->configuration['height'], $this->configuration['upscale'])) {
   +      $this->logger->error('Image scale and crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', [
   +        '%toolkit' => $image->getToolkitId(),
   +        '%path' => $image->getSource(),
   +        '%mimetype' => $image->getMimeType(),
   +        '%dimensions' => $image->getWidth() . 'x' . $image->getHeight(),
   +      ]);
   

   I propose to reformat this method:

       $status = $image->scaleAndCrop(
         $this->configuration['width'],
         $this->configuration['height'],
         $this->configuration['upscale']
       );
   
       if (!$status) {
         $this->logger->error('Image scale and crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', [
           '%toolkit' => $image->getToolkitId(),
           '%path' => $image->getSource(),
           '%mimetype' => $image->getMimeType(),
           '%dimensions' => $image->getWidth() . 'x' . $image->getHeight(),
         ]);
       }
   
       return $status;
   

2. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,82 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +    $dimensions['width'] = $this->configuration['width'];
   +    $dimensions['height'] = $this->configuration['height'];
   

   Do we need this update? Note, that Image::scaleDimensions() above will do the same if upscale will be allowed.

#157

1. Not sure if it necessary, that kinda style'ish change

2.    * Scales image dimensions while maintaining aspect ratio.
      *
      * The resulting dimensions can be smaller for one or both target dimensions.
   

   Thing is Image::scaleDimensions() saves an aspect ratio of an original image. And this update is actually how "crop" performed. Without this image will not be cropped to precise dimesions.

3. +++ b/core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php
   @@ -19,11 +21,82 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
   +    $form['width']['#required'] = FALSE;
   +    $form['height']['#required'] = FALSE;
   ...
   +  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
   +    parent::validateConfigurationForm($form, $form_state);
   +    if ($form_state->isValueEmpty('width') && $form_state->isValueEmpty('height')) {
   +      $form_state->setErrorByName('data', $this->t('Width and height can not both be blank.'));
   +    }
   +  }
   
   +++ b/core/modules/image/src/Tests/Update/ScaleAndCropUpscaleUpdateTest.php
   @@ -0,0 +1,43 @@
   +    // Test that Scale and crop effect has 'upscale' parameter set.
   

   I'm also thinking now that this changes probably shouldn't take place. Skipping one dimension allowed in "Scale" to make second one auto, in the context of "Scale and crop" it dosen't seem to make sense. Any other thoughts about this point?

Tested both upscaled and normal images. Works well.

Tested both the normal and upscaled images and it works well.

We still need to deal with the discussion from #152 onwards before this can be committed.

Couple comments:

1) RE #158, agreed, we should not make width or height optional. Both should be required because crop needs to know the specific size. I think that part of the patch should be removed.

2) If an undersized image is uploaded (it's height and/or width are smaller than the defined height/width for the crop), what is the expected behavior when upscaling is disabled? This was mentioned many years ago in #14. My expected behavior is that the image is still cropped to the desired aspect ratio (calculated using the width/height of the desired crop size). Not sure if GD does that by default, but if it doesn't, then we need some logic to make that happen.

3) I think we need to update the arguments() method of \Drupal\system\Plugin\ImageToolkit\Operation\gd\ScaleAndCrop.php to add the new argument for upscale, just so it's documented.

For anyone that needs this support in the meantime, here's a contrib module I just created that adds it: https://www.drupal.org/project/image_scale_and_crop_without_upscale

2) If an undersized image is uploaded (it's height and/or width are smaller than the defined height/width for the crop), what is the expected behavior when upscaling is disabled? This was mentioned many years ago in #14. My expected behavior is that the image is still cropped to the desired aspect ratio (calculated using the width/height of the desired crop size). Not sure if GD does that by default, but if it doesn't, then we need some logic to make that happen.

When I specify a crop, it's because I want the image to be a specific size. If the image is too small to be that size, then I wouldn't want it cropped at all -- that would make the image even smaller, which to me makes the problem worse. My clients would get confused by that behavior.

For example, if I have a 1920x1080 space and the client uploads a 1080x1080 image, I'd leave the image alone and let there be black bars on the sides. I don't see any benefit to cropping the top and bottom off the image when it was big enough to fill the space vertically to begin with.

The benefit is that you may not want black bars to show up on your site.

If the image is too small and you absolutely need to fill all of the space anyway, then just use the Scale and Crop option as-is, since it upsamples to fit.