ImageEffectBase::transformDimensions() should have a sane default implementation

On it.

And voilà.

Note that the new implementation of transformDimensions() now is nicely symmetric with getDerivativeExtension():

  /**
   * {@inheritdoc}
   */
  public function transformDimensions(array &$dimensions) {
    // Most image effects will not change the dimensions. This base
    // implementation represents this behavior. Override this method if your
    // image effect does change the dimensions.
  }

  /**
   * {@inheritdoc}
   */
  public function getDerivativeExtension($extension) {
    // Most image effects will not change the extension. This base
    // implementation represents this behavior. Override this method if your
    // image effect does change the extension.
    return $extension;
  }

Finally: in core, we have 6 image effects. 2 of them can use this new default behavior, and hence can remove their overrides. The other 4 have their own specific implementations. Only one of them applies the behavior that was the previous default, but even then only in one case (added a comment to that one to clarify it).

Shortening the IS a bit.

#5: I think we can make it deterministically random, which would allow us to calculate the dimensions even in that case. Quoting myself at #2260061-272: Responsive image module does not support sizes/picture polyfill 2.2:

All dimensions can be calculated, with a sole exception: when you use the rotation image effect with the "random rotation" setting enabled. But I can't imagine that's a very frequently used one. And even then… we could make it "deterministically random" if we'd want to: we could take the hash of the file name and map that to a float/integer between 0 and 360, and voila: deterministically random.

+++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
@@ -52,6 +52,7 @@ public function transformDimensions(array &$dimensions) {
+    // The rotation is random, hence we cannot determine the new dimensions.
     else {
       $dimensions['width'] = $dimensions['height'] = NULL;
     }

No it's not just when the rotation is random, also when the rotation angle is not a modulo 90. There would be a #1551686: Image rotate dimensions callback can calculate new dimensions for every angle dealing with that.

Also looking at the parent issue, I am not sure about comment in #2260061-271: Responsive image module does not support sizes/picture polyfill 2.2

AFAIK all dimension are calculatable

holds - transformDimension() tries to derive dimensions just from an input set of width/height, without 'knowing' about the actual image. But in some cases this may not possible, think e.g. of an autorotate effect that uses EXIF information to orientate the image portrait vs. landscape (Imagemagick has such an effect). In this case, you have to know about the image to determine the dimensions, and the possibility to set NULL dimension is there so that image_style will not output inline styling for the "width" and "height" attributes (sorry, I do not grab the implications of that on responsive images).

This patch looks good from a cleanup point of view, just my concern is we should still accept some effects may need to set dimensions to NULL.

Easy fix.

This patch looks good from a cleanup point of view, just my concern is we should still accept some effects may need to set dimensions to NULL.

Absolutely, we don't remove that ability! We just change the default to be more sensible :)

EDIT: and thanks for pointing out the module 90 thing and linking to #1551686: Image rotate dimensions callback can calculate new dimensions for every angle, that looks very interesting!

#12 FWIW, and if anybody interested, in an Imagemagick toolkit conversion to D8 sandbox I have been working on I have developed a small component to manage rectangle rotation algebra (see here).

The Imagemagick toolkit is not managing images in memory like the GD one does, so it is ignorant about the actual size of an image during the effect application chain (kind of the same issue we are facing more generally here with transformDimensions), so dimensions upon rotation have to be calculated.

The good thing about that is that makes simple to calculate output dimensions based on input dimensions and rotation angle (any angle)

  $rect = new Rectangle($input_width, $input_height);
  $rect = $rect->rotate($angle);
  $output_width = $rect->getBoundingWidth();
  $output_height = $rect->getBoundingHeight();

If this could be of interest, I could try to take that in a patch for core (either here or separate issue), but please let me know

+++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
@@ -52,6 +52,7 @@ public function transformDimensions(array &$dimensions) {
+    // The rotation is random, hence we cannot determine the new dimensions.

I still believe the comment here at this stage should be

// The rotation is random or not a multiple of 90 degrees, hence we cannot determine the new dimensions.

Other than that, RTBC

#15: oops, I didn't realize I had to reroll the patch — you're absolutely right of course! Actually, then the comment becomes a pure negation of the comment for the if-branch… and hence we're just repeating ourselves. I wanted to add this comment to stress things, but clearly it's not an actual improvement. So, just removed the wrong comment instead. Hope you're okay with that.

Also added a beta evaluation.

+++ b/core/modules/image/src/ImageEffectBase.php
@@ -71,7 +71,9 @@ public static function create(ContainerInterface $container, array $configuratio
-    $dimensions['width'] = $dimensions['height'] = NULL;
+    // Most image effects will not change the dimensions. This base
+    // implementation represents this behavior. Override this method if your
+    // image effect does change the dimensions.

Sorry for nitpicking :(

Since we're at it, I think this comment could be better, and also filling a gap in documentation, something like (it's verbose maybe you can do better :))

This base implementation assumes that the image effect is not changing the dimensions of the image. Most image effects have this behavior. Override this method if your image effect does change the dimensions, by setting $dimensions['width'] and $dimensions['height'] to the target values that the effect, once applied, would change the image to. If the effect is not able to determine the target values, you can set $dimensions['width'] and $dimensions['height'] to NULL, in which case the image output through the 'image' theme will not output the "width" and "height" attributes.

What do you think?

EDIT - changed last sentence

See #3 — I made this comment entirely analogous to the one in the method below it.

But I agree, valuable information (the ability to set both dimensions to NULL) is now missing… but that actually points to a gap in the interface documentation! So I took what you said and applied it to \Drupal\image\ImageEffectInterface::transformDimensions instead. Hope you like it.

mondrake++ — thanks for the nitpicking! :)

Wonderful, thank you!

Makes sense. Wonder why we ever did that in the first place.

Committed and pushed to 8.0.x. Thanks!

Yes, yes.