Add crop anchor option to Scale and Crop image effect

Fix for failing test

Can you mark it as RTBC if it' working for you

Marking at as such, I assume it isn't a problem, but not my call

Fixing tag.

+++ b/modules/image/image.effects.inc
@@ -158,12 +158,18 @@ function image_crop_effect(&$image, $data) {
- * @return
...
+* @return

Bogus indentation change.

While being there, also add a blank phpDoc line before @return, please.

+++ b/modules/image/image.effects.inc
@@ -158,12 +158,18 @@ function image_crop_effect(&$image, $data) {
+ *   - "anchor": An anchor type, such as "center-center", used to set the top left coordinates of the crop area

Exceeds 80 chars, missing full-stop/period.

20 days to next Drupal core point release.

new patch attached

I'd like someone deeply familiar with the image toolkit API and Image module to RTBC this patch.

+++ b/modules/image/image.effects.incundefined
@@ -158,12 +159,19 @@ function image_crop_effect(&$image, $data) {
+ *   - "anchor": An anchor type, such as "center-center", used to set
+ *               the top left coordinates of the crop area.

Is there explanation about available values? Supose docs should list them

+++ b/includes/image.incundefined
@@ -158,6 +157,8 @@ function image_get_info($filepath, $toolkit = FALSE) {
+ * @param $anchor
+ *   The anchor type to use when cropping.

Same. Could use @see to point to a definition list

14 days to next Drupal core point release.

works as is for d7

Patch for D7, works on 7.12.

the patch is running. i tested in drupal 7.x cloned from github

Patches need to go into d8 first.

I'm writing this comment since I'd like to see this patch applied soon. The patch #21 is working fine for me.

#21: scale-and-crop.patch queued for re-testing.

Patch needs re roll for /core clicked the wrong link before.

#19: scale-and-crop-1252606-19.patch queued for re-testing.

There seems to be a problem with editing styles, which I noticed when doing functional testing on this patch. I'm setting this issue back to needs work until i can ascertain whether this issue was introduced by this patch, or if it was also present before patching.

It seems that editing an effect which has a position causes a effect to be added rather than editing the existing. If you save it without changing the position, then it does not create a duplicate effect.

https://skitch.com/e-alanevans/8qeq2/edit-test-post-patch-style-testd85....

Steps to reproduce:

1. Create a new image style
2. Add a scale and crop effect to it
3. Make sure it's saved ...
4. Edit the effect you just added
5. Change only the position, and save again
6. Note the image style now has 2 effects, but only one was intentionally added

OK, I don't think this patch introduced this issue (issue exists also on d8 when not patched), so setting back to needs review and we'll deal with the multiplying image effects elsewhere. The problem is that the effects are referred to by a name derived from their function, so editing an existing effect produces a new unique name, thereby adding it.

The multiplying image effects issue does not exist in D7 as effects are referred-to by an int ID, so that's a new issue in D8, but not introduced here. Created #1508872: Image effects duplicate on edit to track this.

I do have to wonder with this patch whether we should maybe display a description of the orientation in the effect summary. For example, here we have no way of knowing which effect has which orientation (well, you could check the url on hover over "edit", but that's not really usable). https://skitch.com/e-alanevans/8qe9k/edit-test-post-patch-style-testd85....

I'll aim to complete testing and review of this, as that's a minor point.

So, this works as designed in test:

• My example image (please don't judge my artistic talents ;) ): https://skitch.com/e-alanevans/8xu9f/untitled-1-100-center-rgb-8
• This landscape image is 600x400 Test image style setup is 100x100 in the orientations top right and center bottom
• This field display is set up as top right: https://skitch.com/e-alanevans/8xu9k/test-testd8crop.localhost
• This is the same image set up as center bottom https://skitch.com/e-alanevans/8xu9u/test-testd8crop.localhost
• Example test portrait image: https://skitch.com/e-alanevans/8xwys/untitled-2-100-left-rgb-8
• test using portrait image in center bottom: https://skitch.com/e-alanevans/8xwy8/test-vertical-testd8crop.localhost
• test using portrait image in top right: https://skitch.com/e-alanevans/8xwyi/test-vertical-testd8crop.localhost

Code review to follow.

I can't really fault this - it's a nice-looking piece of code that does what it sets out to do, so:

+1

Leaving the RTBC for image subsystem maintainers ...

Just a couple of thoughts which might improve it a bit:

• I'd stand by my previous comment that the Image Effect description should contain the anchor info as well: https://skitch.com/e-alanevans/8qe9k/edit-test-post-patch-style-testd85....
• is the commentary/messaging/UI generally sufficient? Does everyone understand what "according to anchor" means? I know exactly what you mean because the widget is familiar from using photoshop, but couldn't guarantee that the majority of users would get it. In that sense, the messaging to the end user is more important than the code comments (because you can assume some tech knowledge in people reading code). I'm thinking for the description something along the lines of: "crop the larger dimension, retaining as much image data as possible from the anchored area (the area furthest away from the anchor will be cropped first where needed)". I am sure my text there could be improved-upon though.
• another option is use photoshop-style visual cues for the widget, but I think that would be adding unnecessary complexity here.
• the test doesn't really change much - it tests exactly what it tested before, and in fact the default value for anchor on this effect would make this test still appropriate even without the explicit change in code, no? I would recommend testing a variety of anchors and retesting. It only tests the values, not the effect, but that is in keeping with existing tests (the test in this issue does not *need* to improve on the existing test strategy). I think it's borderline whether additional tests are needed on different anchor points in order for this to be RTBC, but I would think it would be preferred.
• Rambling .... I wonder if we actually can set up a proper automated functional test for images, but that is definitely something for a separate issue: something along the lines of setting up an example image (in a lossless format) in known colors, and after applying various effects, test the colors of particular pixels in the resulting image. I'm not 100% sure whether that's worth doing ... but it would increase our confidence in image effect changes in future though, as it would enable automated testing for those areas

How i can do it with d7?

@OFF:

I use the patch in comment #21.

I need this patch for D7. Seems to be cropping my images at the bottom, which is not the best place to be cropping.

Re-patched D7-dev. Fixed a problem with t() wrappers on the asserts.

Please, do not change version because that is what you have or want a patch for. First solve issues in the current development session, then backport it. BTW: As a backport would include UI text changes, I'm afraid this is a no go.

d8 patch needs re-roll

41: scale_and_crop_anchor-1252606-40.patch queued for re-testing.

Features goes to 8.2

Rerolled

This tag probably still makes sense

Here's a first (untested) pass at re-rolling this for Drupal 8.6.x. Other than the changes necessary to make it work with a newer Drupal core, this also changes the approach slightly for backwards compatibility: rather than changing the function signature of ImageInterface::scaleAndCrop() it adds a new ImageInterface::scaleAndCropWithAnchor(). This could also be done with a new interface, but since no one will probably ever use ImageInterface without Image, I think this is fine? If this extra work for BC isn't necessary, I'd be happy to switch this back to the old approach.

Anyway, next up is actually testing that it works :-)

Ha! I just tried that locally and it actually worked. I wasn't expecting that :-)

Here's a new patch to try and fix the schema errors.

Here's an attempt to fix the other failures

Looks like I missed one of the rest tests. Also, this patch adds coding style fixes

Here's an updated patch which fixes the existing test for scale and crop (basically, the "BC mode"), and also adds a new test to test the new functionality added here. I'm hoping this one will actually pass!

@rlhawk: Thanks for testing!

Reviewing the patch a little myself:

1. +++ b/core/modules/image/image.module
   @@ -126,6 +126,8 @@ function image_theme() {
   +        'x' => NULL,
   +        'y' => NULL,
   

   This is left over from an earlier version of the patch, but I don't think it's actually necessary. Should be removed.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php
   @@ -24,7 +24,8 @@ class StableTemplateOverrideTest extends KernelTestBase {
      protected $templatesToSkip = [
        'views-form-views-form',
   -    'entity-moderation-form'
   +    'entity-moderation-form',
   +    'image-scale-and-crop-summary',
      ];
   

   I'm not sure if this is the right thing to do because I don't know the motivation for this test. Is the idea that Stable makes it's own version of every template in core? If so, we should probably copy the template into Stable.

   The comment on the test is confusing it says "Ensures that Stable overrides all relevant core templates" - what makes a template "relevant"?

   It has copies of all the other image effect summary templates, so I guess we should follow suite...

Here's a new patch that fixes the things mentioned in #69. I can't think of anything else this patch needs - I think this is ready for wider review!

This issue could probably use an issue summary update, with the usual stuff from the template, but also explaining the backwards compatibility impact of this.

Updated the issue summary based on the template

Fix type-o and explain all the seemingly unrelated test changes in the issue summary

Ah, I did come up with one more thing to change. :-) Thinking about it more, ImageInterface::scaleAndCropWithAnchor() is a bad method name, because we're not passing in the anchor (that's in the image effect plugin), but the offset. So, this patch just renames that to ImageInterface::scaleAndCropWithOffset(). Updating the issue summary too.

IMHO, we should just avoid to make any change to ImageInterface and Image. Just call $image->apply('scale_and_crop', ['x' => $x, 'y' => $y, 'width' => $width, 'height' => $height]) in the effect and that's all. There was a lot of discussion back in #2073759: Convert toolkit operations to plugins whether to have or not the manipulation methods in the Image class - exactly because the 'apply' method is general whereas hardcoded 'scale', 'crop' etc. methods will have the drawback of leading to BC break if any change needed... and here's the smoking gun in that sense. See the comment #280 there.

See also #872206: Add possibility to not upscale for "Scale and crop" effect comments #152 and #153.

I also think we need an update function in order to update the effect configuration for existing image styles - see the patch in #872206-127: Add possibility to not upscale for "Scale and crop" effect for an example - that did not get in yet but was RTBC'ed at that point by the image system maintainer. NW for that.

Thanks, @mondrake!

Just call $image->apply('scale_and_crop', ['x' => $x, 'y' => $y, 'width' => $width, 'height' => $height]) in the effect and that's all.

That's a great idea! That avoids the most controversial part of this patch. +1!

I also think we need an update function in order to update the effect configuration for existing image styles

I don't really understand why this is necessary since the default will be used if missing and it'll match existing behavior. But looking at the other issue, it was this comment where an update function was first requested by the image subsystem maintainer:

#872206-105: Add possibility to not upscale for "Scale and crop" effect

So, I guess, if the maintainer wants it, we gotta do it :-)

Here's a new patch that removes the changes to ImageInterface and Image, because that was the easier change to make. :-) I've updated the issue summary as well. The update hook will come later.

Added patch for 8.4.x, noticed and removed some changes related to profile "demo_umami" which does not exist in 8.4.x

@sushyl feature requests are always targeted to the current development branch (8.6.x in this case). 8.4.x is unsupported now, so please do not change the version.

My bad!

Setting back to "Needs Review" state.

Thanks! Those changes look good to me. I'd RTBC, but I think I'm disqualified by working on earlier versions of the patch :-)

Looks really good to go

1. +++ b/core/modules/image/image.post_update.php
   @@ -20,3 +21,26 @@ function image_post_update_image_style_dependencies() {
   +
   +/**
   + * @addtogroup updates-8.6.0
   + * @{
   + */
   ...
   +/**
   + * @} End of "addtogroup updates-8.6.0".
   + */
   

   We no longer to do this.

2. +++ b/core/modules/image/image.post_update.php
   @@ -20,3 +21,26 @@ function image_post_update_image_style_dependencies() {
   +  foreach (ImageStyle::loadMultiple() as $image_style) {
   +    foreach ($image_style->getEffects() as $effect) {
   +      if ($effect->getPluginId() === 'image_scale_and_crop') {
   +        $image_style->save();
   +        break;
   +      }
   +    }
   

   There's a new scalable way to do this. We should always batch these type of updates as we have no idea how many image styles a site has. See #2949351: Add a helper class to make updating configuration simple

Thanks @rlhawk! Some notes on the conversion:

+++ b/core/modules/image/image.post_update.php
@@ -20,3 +21,23 @@ function image_post_update_image_style_dependencies() {
+    $updated = FALSE;
...
+        $image_style->save();
+        $updated = TRUE;
+        break;
...
+    return $updated;

1. Instead of the $updated variable we can simply return TRUE; inside the if
2. We should not explicitly save the image style, ConfigEntityUpdater will do that for us.

Awesome, looks perfect! Back to RTBC.

Sorry I missed the fact that we should have a change record for this change - since the UI and config are changing.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/ImageStyle/ImageStyleResourceTestBase.php
@@ -79,6 +80,7 @@ protected function getExpectedNormalizedEntity() {
diff --git a/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php

diff --git a/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php
index 510020f8d8..2fcda58fad 100644

index 510020f8d8..2fcda58fad 100644
--- a/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php

--- a/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php
+++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php

+++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php
+++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php
@@ -38,8 +38,12 @@ protected function validateArguments(array $arguments) {

@@ -38,8 +38,12 @@ protected function validateArguments(array $arguments) {
 
     $scaleFactor = max($arguments['width'] / $actualWidth, $arguments['height'] / $actualHeight);
 
-    $arguments['x'] = (int) round(($actualWidth * $scaleFactor - $arguments['width']) / 2);
-    $arguments['y'] = (int) round(($actualHeight * $scaleFactor - $arguments['height']) / 2);
+    $arguments['x'] = isset($arguments['x']) ?
+      (int) round($arguments['x']) :
+      (int) round(($actualWidth * $scaleFactor - $arguments['width']) / 2);
+    $arguments['y'] = isset($arguments['y']) ?
+      (int) round($arguments['y']) :
+      (int) round(($actualHeight * $scaleFactor - $arguments['height']) / 2);
     $arguments['resize'] = [
       'width' => (int) round($actualWidth * $scaleFactor),
       'height' => (int) round($actualHeight * $scaleFactor),

Uhmm.. I think we also need to add 'x' and 'y' arguments to the array in the ::arguments() method of the ScaleAndCrop image toolkit operation class. Currently, these are calculated in the ::validateArguments method, but with the change at hand they will be passed in by the image effect plugin.

Contrib toolkits like ImageMagick will have to adjust, also, if they want to support the new arguments.

Thanks!

the patch is running. i tested in drupal 8.x cloned from github

Adding review credit for patch reviews that affected the patch and explicit testing reviews.

Committed bf86499 and pushed to 8.6.x. Thanks!

The backport policy now is that a new issue for D7 should be opened - see https://www.drupal.org/core/backport-policy

@alexpott I think this chmod went in by mistake

diff --git a/core/scripts/run-tests.sh b/core/scripts/run-tests.sh
index 5dc391e..5dc391e 100644..100755
--- a/core/scripts/run-tests.sh
+++ b/core/scripts/run-tests.sh

Filed #2973804: Add crop anchor option to the Scale and Crop image toolkit operation to enable supporting the same in the ImageMagick module.

Setting back to Fixed and sent @alexpott a message about #107.

@alexpott,
Do you think we could cherry pick this for 8.5.x?