Problem/Motivation

The "Scale" effect provides a choice for whether to upscale. But "Scale and crop" provides no such choice and forces upscale.

Proposed resolution

Allow to choose whether upscaling should be performed or no in "Scale and crop" effect.

User interface changes

Added "Allow Upscaling" checkbox on "Scale and crop" effect edit form.

Files: 

Comments

effulgentsia’s picture

Status: Active » Needs review
FileSize
5.55 KB
PASSED: [[SimpleTest]]: [MySQL] 22,738 pass(es). View

This patch adds the "upscale" option and defaults it to TRUE. This way, we retain BC with current HEAD, and don't need an update function to set it to TRUE for existing effects in the database. Plus, since modules can implement hook_image_default_styles(), we don't want to break their behavior (i.e., they should be able to leave 'upscale' unspecified, and it should then default to TRUE).

Yeah, it kind of sucks for it default to FALSE in the "scale" effect and default to TRUE in the "scale and crop" effect, but that's the price we have to pay to not break BC. I still think it's better to at least have the option rather than not.

effulgentsia’s picture

Dries or webchick will probably be tempted to bump this to D8. Especially if that happens, but even if this patch lands, we need #875326: Add hook_image_effect_info_alter() to allow contrib modules to fill in any remaining holes.

ksenzee’s picture

Status: Needs review » Needs work

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.

David_Rothstein’s picture

Issue tags: +string freeze

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.

joeyabbs’s picture

subscribing

ksenzee’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Issue tags: -string freeze

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.

ksenzee’s picture

FileSize
5.4 KB
PASSED: [[SimpleTest]]: [MySQL] 29,409 pass(es). View

Oops, with patch this time.

ksenzee’s picture

FileSize
6.23 KB
PASSED: [[SimpleTest]]: [MySQL] 29,410 pass(es). View

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.

effulgentsia’s picture

FileSize
5.8 KB
PASSED: [[SimpleTest]]: [MySQL] 33,635 pass(es). View

Reroll to chase HEAD.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

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.

michaelfavia’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 33,764 pass(es). View

Reroll for d7 core move.

oriol_e9g’s picture

Issue tags: +Needs tests
FileSize
6.49 KB
PASSED: [[SimpleTest]]: [MySQL] 34,464 pass(es). View

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

We still need more tests. Temporally NR for testbot.

oriol_e9g’s picture

Status: Needs review » Needs work
davidwatson’s picture

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.

aendrew’s picture

@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.

coderintherye’s picture

What more tests are needed to make this needs review?

xjm’s picture

@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.

iflista’s picture

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
iflista’s picture

FileSize
6.83 KB
PASSED: [[SimpleTest]]: [MySQL] 50,755 pass(es). View

Rebuilded manually for drupal 8 with test.

iflista’s picture

Status: Needs work » Needs review
YesCT’s picture

YesCT’s picture

oriol_e9g’s picture

Minor:

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

Should be:

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

FileSize
6.82 KB
PASSED: [[SimpleTest]]: [MySQL] 52,591 pass(es). View

Made the minor change from #23.

rootwork’s picture

Issue tags: +SprintWeekend2013

Tagging this as work done during the global sprint.

eojthebrave’s picture

Status: Needs review » Needs work
+++ 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.

Kevin Morse’s picture

Status: Needs work » Needs review

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?

thomas.fleming’s picture

Status: Needs review » Needs work

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

claudiu.cristea’s picture

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

chakrapani’s picture

Assigned: Unassigned » chakrapani
Issue summary: View changes

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.

chakrapani’s picture

Status: Needs work » Needs review
FileSize
8.71 KB
PASSED: [[SimpleTest]]: [MySQL] 59,744 pass(es). View

Adding a reroll as per the latest D8 snapshot.

davidwatson’s picture

Version: 8.x-dev » 9.x-dev

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

webchick’s picture

Version: 9.x-dev » 8.x-dev

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.

skipyT’s picture

+    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.

claudiu.cristea’s picture

Status: Needs review » Needs work
  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.

skipyT’s picture

FileSize
4.8 KB
9.8 KB
FAILED: [[SimpleTest]]: [MySQL] 63,194 pass(es), 1 fail(s), and 0 exception(s). View

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.

skipyT’s picture

Status: Needs work » Needs review
FileSize
9.8 KB
FAILED: [[SimpleTest]]: [MySQL] 62,991 pass(es), 1 fail(s), and 0 exception(s). View

forgot to change the status

Status: Needs review » Needs work

The last submitted patch, 37: image-upscale-optional-with-test-872206-36.patch, failed testing.

chakrapani’s picture

Status: Needs work » Needs review
FileSize
10.02 KB
PASSED: [[SimpleTest]]: [MySQL] 63,176 pass(es). View

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.
skipyT’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 39: image-upscale-optional-with-test-872206-39.patch, failed testing.

chakrapani’s picture

Status: Needs work » Needs review
skipyT’s picture

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.

chakrapani’s picture

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

mathes’s picture

i tested patch from #39 and it works for me

chakrapani’s picture

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.

The last submitted patch, 36: image-upscale-optional-with-test-872206-36.patch, failed testing.

skipyT’s picture

+++ 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.

chakrapani’s picture

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.

skipyT’s picture

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?

chakrapani’s picture

Issue tags: -Needs tests
FileSize
10.18 KB
PASSED: [[SimpleTest]]: [MySQL] 63,309 pass(es). View
10.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,466 pass(es). View
1.12 KB
1.12 KB
645 bytes

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 :)

skipyT’s picture

Status: Needs review » Reviewed & tested by the community

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

davidwatson’s picture

@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.

chakrapani’s picture

chakrapani’s picture

Assigned: chakrapani » Unassigned
Issue tags: +RTBC over 4 weeks

Been in RTBC for more than 4 weeks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: image-upscale-optional-with-test-872206-51-a.patch, failed testing.

vastav’s picture

vastav’s picture

Status: Needs work » Reviewed & tested by the community

still green.

YesCT’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests, +Needs issue summary update, +Needs screenshots
Related issues: +#1898420: image.module - Convert theme_ functions to Twig
FileSize
9.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,524 pass(es). View
3.19 KB

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.

tomreavley’s picture

Assigned: Unassigned » tomreavley
tomreavley’s picture

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

tomreavley’s picture

Assigned: tomreavley » Unassigned
Status: Needs review » Needs work

Patch does not apply.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
8.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,936 pass(es). View

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

Please review updated patch which applies cleanly.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
Related issues: +#2073759: Convert toolkit operations to plugins
+++ 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.

l0ke’s picture

Status: Needs work » Needs review
FileSize
8.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,267 pass(es). View

Reworked patch.

m1r1k’s picture

Issue tags: +#ams2014contest
Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: - +Needs reroll

Patch no longer applies. It needs reroll.

Jalandhar’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
9.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/image/src/Plugin/ImageEffect/ScaleAndCropImageEffect.php. View

Here is the updated patch which fixes this feature.

Status: Needs review » Needs work

The last submitted patch, 68: image-upscale-optional-872206-68.patch, failed testing.

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
9.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,643 pass(es), 10 fail(s), and 5 exception(s). View

Updating the patch with the reroll changes.

Status: Needs review » Needs work

The last submitted patch, 70: image-upscale-optional-872206-70.patch, failed testing.

xjm’s picture

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.

rootwork’s picture

Status: Needs work » Needs review
FileSize
7.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,758 pass(es), 10 fail(s), and 5 exception(s). View
9.67 KB

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

Status: Needs review » Needs work

The last submitted patch, 73: image-upscale-optional-872206-73.patch, failed testing.

rootwork’s picture

Issue tags: +Amsterdam2014

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

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Status: Postponed » Needs work

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

johnrosswvsu’s picture

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.

johnrosswvsu’s picture

johnrosswvsu’s picture

I will make further updates to address issues.

The last submitted patch, 79: drupal-core-image-upscale-optional-872206-79-8.1.x.patch, failed testing.

johnrosswvsu’s picture

johnrosswvsu’s picture

Status: Needs review » Needs work

The last submitted patch, 84: drupal-core-image-upscale-optional-872206-84-8.1.x.patch, failed testing.

johnrosswvsu’s picture

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.

johnrosswvsu’s picture

Status: Needs work » Needs review
FileSize
11.61 KB

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Issue tags: +Needs reroll, +Dublin2016, +Twig

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.

MobliMic’s picture

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

MobliMic’s picture

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

Please review

ZeiP’s picture

Issue tags: -Needs reroll

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

l0ke’s picture

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'];
  }
BR0kEN’s picture

  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.

l0ke’s picture

l0ke’s picture

BR0kEN’s picture

Status: Needs review » Reviewed & tested by the community
claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice, -SprintWeekend2013, -RTBC over 4 weeks, -Amsterdam2014, -Dublin2016 +Needs change record

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 :)

l0ke’s picture

  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.
l0ke’s picture

Status: Needs work » Needs review
l0ke’s picture

BR0kEN’s picture

l0ke’s picture

Issue tags: -Needs change record

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

BR0kEN’s picture

Status: Needs review » Reviewed & tested by the community

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

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs upgrade path, +Needs upgrade path tests

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).

mondrake’s picture

Assigned: Unassigned » mondrake
mondrake’s picture

#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.

claudiu.cristea’s picture

+++ 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.

mondrake’s picture

Status: Needs review » Needs work
claudiu.cristea’s picture

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

mondrake’s picture

#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?

claudiu.cristea’s picture

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
14.61 KB

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

Update test missing, #112 looks tough...

mondrake’s picture

Status: Needs review » Needs work
BR0kEN’s picture

@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()/[]

claudiu.cristea’s picture

@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

BR0kEN’s picture

@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?

claudiu.cristea’s picture

@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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
17.82 KB

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

mondrake’s picture

FileSize
17.88 KB
1.48 KB

See this.

The last submitted patch, 119: 872206-119.patch, failed testing.

claudiu.cristea’s picture

Looks nice!

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

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

Status: Needs review » Needs work

The last submitted patch, 120: 872206-120.patch, failed testing.

claudiu.cristea’s picture

+++ 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;
    }
  }
}
mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path, -Needs upgrade path tests
FileSize
2.71 KB
17.6 KB

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

claudiu.cristea’s picture

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

Probably this is unused now.

mondrake’s picture

FileSize
565 bytes
17.56 KB

#126: indeed...

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

l0ke’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.22 KB
17.58 KB
  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.

l0ke’s picture

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.

claudiu.cristea’s picture

Status: Needs review » Needs work

#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']);
}
BR0kEN’s picture

Status: Needs work » Needs review

@claudiu.cristea

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

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?

BR0kEN’s picture

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

mondrake’s picture

Status: Needs review » Needs work

#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?

BR0kEN’s picture

Status: Needs work » Needs review

@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?

l0ke’s picture

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?

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

@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.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work

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.

l0ke’s picture

@ 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?

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 140: drupal-core-image-upscale-optional-872206-140-8.3.x.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Looks like an unrelated failure.

Status: Needs review » Needs work

The last submitted patch, 140: drupal-core-image-upscale-optional-872206-140-8.3.x.patch, failed testing.

claudiu.cristea’s picture

Status: Needs review » Needs work

The last submitted patch, 140: drupal-core-image-upscale-optional-872206-140-8.3.x.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
l0ke’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 140: drupal-core-image-upscale-optional-872206-140-8.3.x.patch, failed testing.

BR0kEN’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

mondrake’s picture

Issue tags: +Needs manual testing

#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.

l0ke’s picture

Issue tags: -Needs manual testing

#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'])) {
mondrake’s picture

Status: Needs review » Needs work

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

BR0kEN’s picture

Status: Needs work » Needs review

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

BR0kEN’s picture

@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.

l0ke’s picture

#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?

darius.restivan’s picture

Tested both upscaled and normal images. Works well.

darius.restivan’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 140: drupal-core-image-upscale-optional-872206-140-8.3.x.patch, failed testing.