ImageStyle::transformDimensions unable to deal with all effects.

Instead of a third parameter, maybe switch it to an $options array? It's nice to have a clean set of parameters rather than a ridiculous amount of arguments.

Would also have to be fixed in Drupal 8 first ;-) .

Just hit upon this from #1556244: Using 'manual crop' style, and leaving the minimum height and/or width fields blank results in missing image attributes. The Manual Crop module needs to know the source image in order to be able to look up the applied crop. Without knowing the source image in the dimensions callback, it's not posible to calculate the updated dimensions. Other cropping modules have hit upon the same limitation such as the ImageCrop module in #1392568: Add dimensions callback.

Attached patch adds the additional optional third paramater to image_style_transform_dimensions(), and ensures that it is used in theme_image_style().

However, I do see a potential problem with this because in order to get that new 'source' data into the the dimensions callback, I have to add it to $effect['data']['source'], but $effect['data'] is the data as returned from the effect's config form, which means that this could potentially cause a conflict with a module that implements an effect that defines a 'source' property on their effect config. Unlikely, but possible, and not sure how we could protect against that other than document the fact that the 'source' keyword is reserved.

Alternatively, we would have to a new third paramater to the dimensions callback itself, or change that callback so that the second paramater was a keyd array with 'data' holding the config data, and 'source' holding the new image source value. That would be the most flexible approach, but would require all dimension callbacks from core and contrib to be updated to accommodate it.

Here is the same patch for D7.

Maybe this on'll pass the test.

oops, typo.

Also updated D7 version to use 'path' rather than 'source' for consistency.

Is there a reason, why it is not in 7.15?

Update for this patch. Both style and file uri should be added as argument.

#11: 1364670-11-info-for-dimensions.patch queued for re-testing.

The patch from #9 applies smoothly on D7.x-19

2 #10

Is there a reason, why it is not in 7.15?

Similar thoughts...

RTBC - This is very important to get in D8 - especially now that we are close to RC.

There are contrib modules that cannot find dimensions unless they know the URI (e.g. manualcrop).

picture has been renamed to responsive_image, so patch needs a reroll

As per https://www.drupal.org/node/2120875 picture module removed from core.

Adding to @er.pushpinderrana's patch, responsive_image module updates.

Note: Interdiff is against #15.

Hm, the non standard base:// that is not a stream wrapper seems to bite us now ... :-/

A few notes:

• Besides transformDimensions() there is, since a month or 2, also a similar function to get the extension/image format of the derivative: getDerivativeExtension(). For similarity reasons, this method should be treated the same as transformDimensions(), thus also get the $uri passed.
• Why is the $uri parameter optional? it is always passed, so make the code more robust by making it non-optional.
• Th calling code in responsive_image.module (function responsive_image_get_image_dimensions()) uses "... instanceof Drupal\image\Entity\ImageStyle" instead of "... instanceof Drupal\image\Entity\ImageStyleInterface"? (this is not due to this patch, but this patch touches that piece of code).

Can someone reroll and take care of these points (at least the first 2), I will review.

@FabianX: unpublished those duplicate comments due to that d.o flakiness.

@mondrake pointed me to this issue. I'm coming from #2420789: Make RotateImageEffect's random rotation deterministic, so ::transformDimensions() can return the actual dimensions. That issue describes a very precise, well-defined use case for needing extra information to be passed to transformDimensions(). We're debating there whether $uri is acceptable, since it'd allow transformDimensions() to perform I/O, which AFAICT this interface was designed to specifically prevent.

However, given this issue, it'd seem it'd be better to pass either $file_uri or even just ImageInterface $image (consistent with public function applyEffect(ImageInterface $image) on the same interface).

Please read that other issue, perhaps we can merge it into this one.

Patch is rerolled - got a conflicts and reuploaded the patch after fixing them . Find details below

First, rewinding head to replay your work on top of it...
Applying: Applying patch from issue 1364670 comment 9245255
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging core/modules/responsive_image/responsive_image.module
CONFLICT (content): Merge conflict in core/modules/responsive_image/responsive_image.module
Auto-merging core/modules/image/src/Plugin/ImageEffect/DesaturateImageEffect.php
CONFLICT (content): Merge conflict in core/modules/image/src/Plugin/ImageEffect/DesaturateImageEffect.php
Auto-merging core/modules/image/src/ImageStyleInterface.php
Auto-merging core/modules/image/src/ImageEffectInterface.php
CONFLICT (content): Merge conflict in core/modules/image/src/ImageEffectInterface.php
Auto-merging core/modules/image/src/ImageEffectBase.php
CONFLICT (content): Merge conflict in core/modules/image/src/ImageEffectBase.php
Auto-merging core/modules/image/src/Entity/ImageStyle.php
Failed to merge in the changes.
Patch failed at 0001 Applying patch from issue 1364670 comment 9245255

Patch is clean now and reuploaded..

Re-roll the patch #45

Let's see what the testbot thinks of #48 before reviewing it.

@fietserwin Any idea why test has been failed ?

#51: it seems the reroll in #48 removed some code currently in HEAD. Here's a new patch based on #21 and current head - some responsive_image module code changed quite a bit since then.

This would be an API change, so not sure about chances for 8.0.x; also would need tests for newly added $uri parameter.

Fixed NullTestImageEffect.

Sorry, I mistyped the interdiff extension in #54 (.patch instead of .txt). The patch to review is 1364670-54.patch.

.

@mondrake thanks for rerolling. I had a quick look at the current patch and my points from #33 still stand.

- It does not make sense to make $uri optional:
* All usages pass it, so no need t make it optional for the calling sides. If new usages would not want to pass it, they could always pass the empty string or null themselves.
* The ImageEffectInterface is changed, so all effects need to be changed anyway (they cannot not declare the optional parameter, even if they don't use it).

+++ b/core/modules/image/src/ImageEffectInterface.php
@@ -43,8 +43,10 @@ public function applyEffect(ImageInterface $image);
+   * @param string $uri
+   *   The path of the image file relative to the Drupal files directory.
    */

Is that so? I guess that $uri either contains the stream wrapper name (public://...) or else it will be an absolute or relative (to the Drupal root) file path.

@fietserwin

re #57:

It does not make sense to make $uri optional

Agreed.

I guess that $uri either contains the stream wrapper name (public://...) ...

Changed docs to be consistent with the doc of ImageStyleInterface::createDerivative

re #33:

...getDerivativeExtension(). For similarity reasons, this method should be treated the same as transformDimensions(), thus also get the $uri passed.

Not sure why you believe so, but in any case not in scope of this issue.

The calling code in responsive_image.module (function responsive_image_get_image_dimensions()) uses "... instanceof Drupal\image\Entity\ImageStyle" instead of "... instanceof Drupal\image\Entity\ImageStyleInterface"?

Absolutely right... but it seems to me that check is redundant, the line above is calling ImageStyle::load, what could be the class of $entity at that point if not an instance of ImageStyleInterface?? I'd leave that to a follow-up cleanup.

Also - the IS needs update as the proposed solution here is different, we are passing the URI of the original image, not the one of the derivative.

Actually, removing the default NULL to $uri helped to spot that we were missing to add the parameter in the most obvious place... :)

Also, changed my mind on #58

Absolutely right... but it seems to me that check is redundant, the line above is calling ImageStyle::load, what could be the class of $entity at that point if not an instance of ImageStyleInterface?? I'd leave that to a follow-up cleanup.

and made the fix here.

Added a test effect in image_module_test. This effect resizes the image based on the original image file extension - both ::transformDimensions and ::applyEffect. Updated ImageDimensionsTest to test for that.

Thanks for writing the patch and adding tests. On reviewing I found a few minor points:

1. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/UriDependentTestImageEffect.php
   @@ -0,0 +1,72 @@
   +    if (!$image->resize($dimensions['width'], $dimensions['height'])) {
   +      return FALSE;
   +    }
   +    return TRUE;
   +  }
   

   return $image->resize($dimensions['width'], $dimensions['height']);

2. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/ImageEffect/UriDependentTestImageEffect.php
   @@ -0,0 +1,72 @@
   +    switch ($extension) {
   

   Make this switch more robust, even if it is only for testing:
   - use strtolower()
   - also check for 'jpg' (and/or use default): you are only testing the gif and png. If you would have tested the jpg test file as well, it would have failed.

3. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -415,11 +418,8 @@ function responsive_image_get_image_dimensions($image_style_name, array $dimensi
   -  else {
   -    $entity = ImageStyle::load($image_style_name);
   -    if ($entity instanceof Drupal\image\Entity\ImageStyle) {
   -      $entity->transformDimensions($dimensions);
   -    }
   +  elseif ($entity = ImageStyle::load($image_style_name)) {
   +    $entity->transformDimensions($dimensions, $uri);
      }
    
   

   ImageStyle is not an injected service, so checking for ImageStyleInterface instead of ImageStyle - as I originally suggested - is not needed. And as Entity::load() returns static or NULL, indeed no check is needed at all. So this is good: keep this change.

I also think we should still update the other method (responsive_image_get_image_dimensions()) as well, even if there is no direct current contrib effect blocked (it is a new method after all). Reasons to do it now and here:
- Keep the API consistent.
- Introduce these kind of changes all at once, minimizing the number of times for contrib to make similar changes to their effects.
(- There still are none or very few D8 contrib effects developed.)

I updated the issue summary to state the current situation of the patch.

However, in doing so, I found that this issue also proposes to fall back to getimagesize() if either of width or height is not returned. This would result in only 1 palce where I/O is done so that image effects should never have to do I/O: they can set width and/or height to NULL if they cannot set it to correct values. Note that some D7 contrib effects currently do so, but that no transformDimensions() method does check for this... Anyway, we still should decide on what to do with this part of the suggested solution. I think it should be part of this patch but won't block RTBC'íng it, if others think differently.

#62:

1, 2 - done
3 - ok

I also think we should still update the other method (responsive_image_get_image_dimensions())...

what needs to be done?

I found that this issue also proposes to fall back to getimagesize()...

That's the problem - before the edit of the IS, the proposal was to fallback to retrieving dimensions via getimagesize() on the transformed image. But ImageStyle::transformDimension is run at a point when the derived image has not (necessarily) been created yet - see the flow in template_preprocess_image_style: ImageStyle::transformDimension is called first, then ImageStyle::buildUrl; ImageStyle::createDerivative is only invoked in a separate request by the download controller.

So if we were to access the derivative image, we would necessarily have to run ImageStyle::createDerivative within ImageStyle::transformDimension, if the file of derived image is not existing at destination URI. But this would break the entire concept of splitting the generation of the derivative URI from its actual creation.

My proposal would be not to do anything like that, and just stick to what we have in the current patch i.e. only use the URI of the original image file. Thoughts?

RE#63:
- My fault: should be ImageStyle::getDerivativeExtension() and its call chain. This method is like transformDimensions() about finding out what an image style will deliver without actually applying that style. If we acknowledge that transformDimensions() may need to know the URI to give a correct answer, we should also acknowledge that getDerivativeExtension() may need to know the URI as well.

- You are right about using getimagesize(), but this will only be once (per derivative). Subsequent executions of template_preprocess_image_style() for the same derivative could have access to the derivative file. But if the result of this first execution gets cached we are in trouble anyway, so we should prevent caching of this if transformDimensions() does not result a correct result. This is indeed difficult stuff that makes this complex, so perhaps better to say that with the URI passed each effect should be able to return correct values with transformDimensions() and thus there will be no need any more for this fallback.

@fietserwin re #64:

1. ImageStyle::getDerivativeExtension() - I am still of my opinion in #58, if we want that I would suggest to file a separate issue, and discuss the use cases there. Honestly I do not see how we could make an extension dependent on the original file URI, given also that we have a ConvertImageEffect that can control that on a style level. But of course I can be mistaken :)

2.

...so perhaps better to say that with the URI passed each effect should be able to return correct values with transformDimensions() and thus there will be no need any more for this fallback.

I agree. BTW another use case is Imagemagick autorotate effect - ::transformDimension will only be able to return expected derivative dimensions if it has access to EXIF data of the original file. Currently it is voiding height/width and this issue would be able to solve that.

Use cases would be to change the format only if the image does not contain transparency or only if the format is not well optimized, but that can be handled by looking only at the extension as already gets passed in. But having said that, I am sure that eventually someone will come up with a use case for his web site (showing private pictures in a non lossy full quality way, showing public pictures in a lossy reduced quality way?)

But OK, let's postpone that if it would be for the sake of the feature in itself only. If we say we want to keep the API consistent then it should be done here. But I am deferring that decision to a core committer.

So
- code reviewed and improved upon review
- tests added
RTBC

and the status change itself...

At this point in the process, this will need a beta evaluation.

Yep #69, we do need a beta evaluation. We also need a change record. Since this is an API breaking change - code that used to work with not. We need to make the case that the disruption is worth the cost - since this type of change can only be made under committer discretion part...

Additionally, certain changes that reduce fragility are also prioritized. For example: Narrowing an API, removing unused and untested functionality, correcting misspelled or wholly inaccurate method names. Whether or not an issue reduces fragility is at core maintainer discretion and should be discussed on a case-by-case basis.

Draft change record: https://www.drupal.org/node/2534082

Tagging for D7 backport given the OP and initial comments.

Anybody up for beta evaluation?

Added beta evaluation and rephrased the change record.

And back to RTBC.

1. I think the solution looks good and will have minimal impact on contrib whilst solving the problem.
2. The issue summary says:

   Core committer: Accept inconsistency in API.

   What is the inconsistency we're accepting?

3. +++ b/core/modules/image/src/Tests/ImageDimensionsTest.php
   @@ -231,6 +232,48 @@ function testImageDimensions() {
   +    $generated_uri = 'public://styles/test_uri/public/'. drupal_basename($original_uri);
   ...
   +    $generated_uri = 'public://styles/test_uri/public/'. drupal_basename($original_uri);
   

   drupal_basename() is deprecated - use \Drupal::service('file_system')->basename() instead.

4. +++ b/core/modules/image/src/ImageEffectInterface.php
   @@ -43,8 +43,10 @@ public function applyEffect(ImageInterface $image);
   +   * @param string $uri
   +   *   Original image file URI.
   
   +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -127,8 +127,10 @@ public function createDerivative($original_uri, $derivative_uri);
   +   * @param string $uri
   +   *   Original image file URI.
   

   I think this needs more docs as to why it's being passed and what it can be used for - and the dangers of additional IO.

#75:

2.

What is the inconsistency we're accepting?

It's about the fact that there is a proposal in #33 and later to also include a $uri argument to ::getDerivativeExtension, which is not done here.

Working on fixing 3. and 4.

Fixed #75.3 and #75.4

The changes correctly address the issues as posted in #75.

I don't know how this could be back-ported to D7 seems tricky without breaking APIs. Removing tag.

Thank you for addressing my feedback. I don't think this patch makes the API inconsistent and it fixes the bug. so +1 for the approach. Committed 19f7369 and pushed to 8.0.x. Thanks!

Thanks you for adding the beta evaluation to the issue summary.

D7 issue added, thanks to a data array it's possible to fix this in a BC-compatible way for D7:

#3035571: [D7] ImageStyle::transformDimensions unable to deal with all effects.