Make RotateImageEffect's random rotation deterministic, so ::transformDimensions() can return the actual dimensions

Hrm, this would require an API change; ImageEffectInterface::transformDimensions() would also have to receive the file URL. (Currently it only receives the dimensions of an image, which is insufficient — all images with the same dimensions would get the same rotation applied if we were to use that. That feels like it's "insufficiently random".)

But then it would be able to perform file operations. So it'd probably be better to receive a hash of the filename. Something like a $deterministic_randomness_hash parameter would probably make most sense.

Let's get feedback on that.

#4: yeah, it could make sense, but the danger in doing that is that operations that are now guaranteed to not cause I/O, might start causing I/O. That's why I proposed $deterministic_randomness_hash in #2.
So, yes, definitely another issue.

#2, note that #1364670: ImageStyle::transformDimensions unable to deal with all effects. is proposing to add the URI to transformDimensions() signature.

But then it would be able to perform file operations. So it'd probably be better to receive a hash of the filename. Something like a $deterministic_randomness_hash parameter would probably make most sense.

A 'deterministic randomness hash' seems to me too specific for this use case. In the end, it would be able to perform I/O, but shall we babysit what you can do with the URI? If you just calculate a hash with it, then no problem!

#6: well, that really is the fundamental question: do we guarantee no I/O happens at the interface level, or don't we? If we don't, we could just as well pass ImageInterface $image, which would be consistent with public function applyEffect(ImageInterface $image) on the same interface.

#7 well passing ImageInterface $image is 100% I/O guaranteed ;)

I looked at where transformDimension() is called. We have two instances in HEAD, that basically can be reconducted to processing themes 'image_style' and 'responsive_image'. In both cases we do not have there an ImageInterface object instantiated before calling transformDimensions(), just the URI passed in $variables to the theme. For image_style, the actual loading and application of effects to the image is deferred to the file download controller that only runs createDerivative() if the derivative file URI is missing - i.e. only once per source image.

So, that would mean to add getting the $image object from the image.factory in the theme preprocess before calling transformDimensions(). Since all new image objects parse their file upon construction via the toolkit, we would have file I/O always, even if not needed.

So IMHO if we want to add anything, the URI is still the best option -

1. no need to get image objects from the image factory, only pass an existing $variable to transformDimensions()
2. with the URI parameter in transformDimensions()

• most effects will just do nothing with it
• some effects may use it to calculate an hash (the case here)
• as last resort, some effects will use it to get an image object and get information from it or do other I/O operations directly on the file at the URI, who knows

I fully agree with #8 and as such propose to postpone this issue on #1364670: ImageStyle::transformDimensions unable to deal with all effects.. The other issue should add the parameter, this issue can then make use of it to get "deterministic randomness" for the random rotation effect.

I suppose this would also need to wait for #1551686: Image rotate dimensions callback can calculate new dimensions for every angle, as currently only rotations of multiples of 90 degrees can be managed by RotateImageEffect::transformDimensions.

#1364670: ImageStyle::transformDimensions unable to deal with all effects. has been committed.

Hurray!

Still postponed on another issue: #1551686: Image rotate dimensions callback can calculate new dimensions for every angle, as we do not yet calculate dimensions for angles that are not a multiple of 90 degrees. However the code changes/new features here will not interfere with code changes/new features in the other issue, it's only that the ultimate goal of this issue depends on both issues being resolved.

So, we could post a patch here that makes the random angle deterministic, but in itself that won't make transformDimensions() work for rotate in general. But because of this, not setting back to postponed.

Patch.

This is built on top of #1551686-29: Image rotate dimensions callback can calculate new dimensions for every angle. It's quite simple now :)

The do-not-test patch is the incremental one for review, the other is the full patch for testing.

Just realized that in random rotation, the degrees set in configuration define the maximum angle of rotation.

I didn't do a thorough review yet but saw that rotation angles are defined as int while they are floats: you can define an angle of 3.5 degrees. Besides some casts and phpdoc comments, this may also influence the % operator (not sure how that operator treats floats) and thus e.g. the check for angles that are supposed to be a multiple of 90 degrees.

#19 - both the 'degrees' field in the Image effect form, and the config schema, only accept integer values. The GD toolkit ops may take floats (but I wonder if that makes sense anyway...). Supporting floats at effect level would then be a different issue.

Hmmm, then that would be a regression from D7 where I tested it yesterday and where, as it appears, function image_effect_integer_validate() DOES accept float values and so it works. I have filed a separate issue for this: #2576639: "Regression": accept floats for degrees in rotate effect.

Rerolled against latest patch in #1551686: Image rotate dimensions callback can calculate new dimensions for every angle.

Also, since hash() returns a hex string, changed ::getRotationFromUri to convert that to integer via hexdec() instead of casting to int.

This is only for the changes made here compared to the patch in the related issue #1551686: Image rotate dimensions callback can calculate new dimensions for every angle.

1. +++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
   @@ -58,6 +56,27 @@ public function transformDimensions(array &$dimensions, $uri) {
   +   *   An integer angle in the range 0-360.
   

   Change to (something like):
   An angle in the range from -$max_degrees to +$max_degrees.

2. +++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
   @@ -58,6 +56,27 @@ public function transformDimensions(array &$dimensions, $uri) {
   +    if (!$max_degrees || $max_degrees === 0) {
   +      return 0;
   +    }
   

   the second check falls entirely within the first check. Simplify the whole body to:
   $max_degrees = abs((int) $max_degrees);
   return $max_degrees === 0 ? 0 : (hexdec(hash(...;

#1551686: Image rotate dimensions callback can calculate new dimensions for every angle has been committed, so this can be now worked on actively. Leaving to CNW for input in #23.

Rerolled and done #23.1 and .2

Reroll

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

This appears it could still be relevant as the code is still similar.

Tagging for an issue summary update though for steps and proposed solution that matches the fix.

This was implemented in contrib Image Effects module, #3246867: Introduce 'Rotate' effect and operations, alternative to Drupal core's. It's such a very specific use case I wonder if it makes sense to be addressed in core.

Also see #3240390: [policy, no patch] Move image rotation code from core into contrib.

If you're able to update the IS and reroll of the patch I can review again to help move the policy along.

Sorry, I'm not interested here. If and when the policy issue is approved, I will be happy to provide deprecation patches.

I would just suggest to won't fix this issue unless there's anyone really interested to move it forward.

Going to move to PNMI then since it was just sitting there for 7 years.

Honestly… let’s see if anyone is interested. In that case, please reopen.