Follow-up to #2073759: Convert toolkit operations to plugins and for now postponed on that issue until it gets committed. Part of #2105863: [meta] Images, toolkits and operations.
Problem/Motivation
Image effects call the ImageInterface::apply method to perform the actual and toolkit dependent image manipulation. The image object forwards the apply method to the toolkit which forwards it to the appropriate image toolkit operation. This call chain separates image effects from image toolkit operations, yet they are actually strongly related. Each image effect in core has its accompanying image toolkit operation in the GD toolkit that does almost all the work, including calculations and logic that can directly be linked to the options on the image effects forms. This is not necessary, nor logical.
Examples:
- The crop operation accepts that either width or height may be empty and then decides that the aspect ration should be kept to compute the missing parameter as such.
- The scale operation has a boolean upscale parameter, to not perform the resize if that would mean upscaling the image.
- The scale and crop image effect has its own scale_and_crop operation, whereas it could just use the scale and crop operations.
As (the now closed) issue #2108307: Provide an abstract layer for image operations geometry pointed out, this leads to duplication of calculations and other logic that should better be put on a higher level.
Proposed resolution
Make the image toolkit operations dumber:
- they should perform only 1 basic image manipulation
- they should not call each other.
- they should not be to smart about missing parameters.
- they should more mimic the typical operations an actual image processing class offers (like GD or the Imagick class). Thus a resize should get the actual new length AND width, not just.
- they should preferably not do difficult calculations.
Make the image effects smarter:
- they should do the calculations themselves, especially those that are based on checkboxes and such that are offered on the UI of the image effects.
-they should be composed of more basic image manipulations.
Actual changes foreseen:
Operations:
- The scale operation will be removed.
- The scale_and_crop operation will be removed.
- The crop operation will require all 4 integer parameters.
Image effects:
- Crop should calculate the exact rectangle to crop to in the effect.
- Scale should do the aspect ratio based calculations itself, as well as the upscale check, and then call the resize operation.
- ScaleAndCrop should call the resize and crop operations itself.
- ...
Remaining tasks
- Wait for the main issue to get in.
- Find out how far we can go, considering that some calculations cannot be done by the in contrib to be dveloped ImageMagick toolkit, because the actual width and height may not be known (image (auto/random)rotate).
- Refactor the operations and the effects.
API changes
The operations provided by a toolkit are not seen as a real API, nor are their parameter definitions. So no API changes, though core and contrib effects and contrib toolkits may have to be adapted. So this should not be done too late in the beta stage.
User interface changes
None.
Comments
Comment #1
fietserwinComment #2
fietserwinComment #3
mgiffordSeems like it is committed.
Comment #4
andypostLooks more like 9.x material
Comment #5
joelpittetThis may be 9.0.x material, but let's unpostpone it and let someone decide who is working on this.
Comment #21
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #22
smustgrave commentedwanted to bump this 1 more time.