Beta phase evaluation
Issue category | Bug because the current code will lead to many wrong image effect implementations in D8. |
---|---|
Prioritized changes | This is not a prioritized change for the beta phase. |
Disruption | Disruption for core is zero. Disruption for contributed and custom modules is close to zero; it will only affect image effects that don't override the ::transformDimensions() method. And it's precisely those which we want to disrupt: they should make a conscious choice how they transform dimensions (as was the case in D7), and not get a default that 1) doesn't make sense, 2) has negative front-end performance repercussions for responsive images.It is also unlikely that more than a handful, if indeed any, image effects have been ported to D8, hence disruption is truly very, very low. |
Problem/Motivation
Problem (re-)discovered in #2260061: Responsive image module does not support sizes/picture polyfill 2.2.
Short version
The abstract base class ImageEffectBase
provides a default implementation of the transformDimensions()
method that makes no sense, because it's the implementation you want 1% of the time, and the implementation which you want to avoid if at all possible.
But because it's the default implementation, many developers will just roll with, hence causing problems further down the line. For details about that, see the long version below.
Long version
From #2260061-267: Responsive image module does not support sizes/picture polyfill 2.2:
[…]
All that being said… the entire reason this is a problem, is because
\Drupal\image\ImageEffectBase
does this:/** * {@inheritdoc} */ public function transformDimensions(array &$dimensions) { $dimensions['width'] = $dimensions['height'] = NULL; }
But this is an abstract base class. Why can't it just choose to not implement this method, hence forcing every single image effect to implement this, or at least consciously choose to set the dimensions to
NULL
? Right now, it's very easy to create an image effect that subclasses that abstract base class, but to forget to override the defaulttransformDimensions()
implementation from the abstract base class.Is this even necessary? It's undocumented in Drupal 8 why you would ever need/want to return
NULL
.So I did a bit of archeology. This was introduced in #1821854: Convert image effects into plugins (commit
a48f0d88
) and it seems zero consideration was given to this… because it was actually just moving code around. This funny$dimensions['width'] = $dimensions['height'] = NULL;
business originates from theImageStyle
class (see it in its full glory usinggit show a48f0d88^:core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php
). That in turn points to commit27f8cd4c
/#2027423: Make image style system flexible:$ git blame -L292,+18 a48f0d88^ -- core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 292) public function transformDimensions(array &$dimensions) { 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 293) module_load_include('inc', 'image', 'image.effects'); 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 294) 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 295) if (!empty($this->effects)) { 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 296) foreach ($this->effects as $effect) { 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 297) if (isset($effect['dimensions passthrough'])) { 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 298) continue; 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 299) } 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 300) 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 301) if (isset($effect['dimensions callback'])) { 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 302) $effect['dimensions callback']($dimensions, $effect['data']); 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 303) } 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 304) else { 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 305) $dimensions['width'] = $dimensions['height'] = NULL; 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 306) } 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 307) } 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 308) } 27f8cd4c (Alex Pott 2013-07-08 23:37:04 +0100 309) }
And that then came from
image.module/image_style_transform_dimensions()
, unchanged, so we need to dig further:$ git blame -L568,+1 27f8cd4c^ -- core/modules/image/image.module c16a978c (Dries 2012-03-23 16:21:49 -0600 568) $dimensions['width'] = $dimensions['height'] = NULL;
… before I went further, I figured I'd look at the docs for D7's
image_style_transform_dimensions()
, which also doesn't explain it. But the first comment there confirms what I thought: some image effects make it impossible to transform dimensions. Though it's not fully clear to me why. I'm guessing it's because we're transforming dimensions without actually applying the image effects, i.e. it's more about calculating the expected dimensions — actually generating the image style to just get the dimensions is considered too expensive?Conclusion: I don't see why this needs to be part of the abstract base class. […]
From #2260061-268: Responsive image module does not support sizes/picture polyfill 2.2:
#267.#265: It was briefly discussed in #2330899-6: Allow image effects to change the MIME type + extension, add a "convert" image effect and #2330899-7: Allow image effects to change the MIME type + extension, add a "convert" image effect. I was meaning to file an issue for it but I didn't get to it yet.
Proposed resolution
Change ImageEffectBase::transformDimensions()
to do what is most common for image effects: to not change the dimensions at all.
Remaining tasks
Do it.
User interface changes
None.
API changes
None.
Original report by @attiks
see #2260061-267: Responsive image module does not support sizes/picture polyfill 2.2
[…]
PS: Generating the image is way too expensive, so not an option.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2420751-18.patch | 5.36 KB | Wim Leers |
Comments
Comment #1
Wim LeersOn it.
Comment #2
Wim LeersComment #3
Wim LeersAnd voilà.
Note that the new implementation of
transformDimensions()
now is nicely symmetric withgetDerivativeExtension()
: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).
Comment #4
Wim LeersShortening the IS a bit.
Comment #5
attiks CreditAttribution: attiks commentedAssuming test bot is fine with this, and it should.
PS: Is anybody using the rotate random effect? I think we better remove it?
Comment #6
Wim LeersComment #7
Wim Leers#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:
Comment #9
mondrakeNo 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
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.
Comment #10
attiks CreditAttribution: attiks commentedI created #2420789: Make RotateImageEffect's random rotation deterministic, so ::transformDimensions() can return the actual dimensions
Comment #11
Wim LeersEasy fix.
Comment #12
Wim LeersAbsolutely, 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!
Comment #13
attiks CreditAttribution: attiks commented#9 Responsive images cannot output images in a srcset in combination with sizes, if the width is unknown, it would cause problems in the browser. Responsive images can work with images without a known dimension when the mapping is using a plain image effect.
Comment #14
mondrake#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)
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
Comment #15
mondrakeI 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
Comment #16
Wim Leers#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.
Comment #17
mondrakeSorry 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
Comment #18
Wim LeersSee #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! :)
Comment #19
mondrakeWonderful, thank you!
Comment #20
webchickMakes sense. Wonder why we ever did that in the first place.
Committed and pushed to 8.0.x. Thanks!
Comment #23
webchickYes, yes.