Problem/Motivation
If we were to create an image handling library that supports multiple image handling toolkits from scratch, how would we architect it?
- We would look at currently existing toolkits like Gd, Imagick and Imagine and then embed a similar structure into Drupal (using managers, plugins, factories, etc.).
- So, we would define an Image processing plugin.
- So, we would create a manager that can create the image manipulation objects for a given toolkit.
- We would implement 1 plugin per supported toolkit. This plugin is able to do image manipulation using a given toolkit. This would mean it is able to create an in-memory image resource based on an image file, transform that resource and save it to a file in a given image format.
- We would make sure that we would restrict ourselves to image manipulation, not a file handler, only image file opening and saving interacts with files.
How do we currently do it?
- We have a system like the above, its named ImageToolkit (+interface, manager, image processing operations, etc.)
- However, we also have exactly 1 Image class, not meant to be subclassed or replaced.
- We have an image factory that can create exactly 1 kind of implementation of ImageInterface, namely the above Image class.
- This Image class forwards all calls to an image toolkit plugin that is 1-to-1 tied to it. Calls that it does not forward are an erroneous try to do some file handling in an image processing library.
- The outside world (image styles, effects and fields) talks to Image to do image manipulation. The toolkit is almost invisible.
- The outside world (ImageStyle download controller) misuses image to serve files.
- As image is not more than an empty proxy class, our tests contains lots of duplication. Tests at the toolkit level ar repeated at the image level.
Looking at it, we conclude that Image does not serve any explicit purpose and that we can as well merge Image and ImageToolkit into 1 plugin system.
Some discussions already happened in the past between @mondrake, @fietserwin and @claudiu.cristea to 'unify' Image and ImageToolkit, but that was premature at that stage. Now the Image object is much slimmer and the idea becomes more obvious.
Further explanation of the idea
Looking outside Drupal (at other Image libraries):
- If GD would be rewritten in a OOP way, there would be a GdImage class that contains the resource and provides all the operations.
- Imagick has already 1 class containing bot the image and the operations.
- Imagine offers 1 interface (ImageInterface) and various implementations (GdImage, ImagickImage, GMagickImage). ImageInterface defines "all" the operations like load, save, resize, rotate, etc. Thus also here: data and operations in the same object.
This suggests that sticking with 1 Image interface and an implementation of that per underlying toolkit is the normal way to go.
Looking at sound OO principles:
- Keep data and operations in 1 class.
- To much coupling between 2 components is a code smell. The 1-1 relation between Image and ImageToolkit is such a code smell.
- Proxy classes are a code smell: Image is quite empty, it forwards almost all calls to its strongly coupled toolkit brother.
Looking at open issues:
All these issues are addressing problems that (partly) originate due to current artificial separation
But why is it the way it is?
Simply: historical reasons. In D7, the image system was not OOP, there was an image object and toolkit operations were defined based on a naming scheme. OOP'ing this system in D8 was done by simply defining classes around existing concepts, not refactoring or rearchitecting the system.
Proposed resolution
This issue/patch:
- Merges Image and ImageToolkit (and its interfaces)
- Merges the image factory and image toolkit manager. By merging the above 2 classes/interfaces, Image becomes a plugin and thus we should have a manager, no longer a factory.
- [Optional] Rename everything to Image and drop the use of Toolkit in class, namespace and variable names. This also means moving classes to other folders. In documentation use "image plugin".
Remaining tasks
- The patch needs a reroll.
- Agree on naming: Image or ImageToolkit? Image is shorter and already used outside the subsystem,so hardly any changes required there. ImageToolkit is more explicit about the fact that it is an image handling library.
- restore the "value" behavior of Image objects. An image object should not be reused for different files or for creating different blank canvases. So there should be no methods to set the file or create a blank canvas, this should be part of the constructor.
User interface changes
None.
API changes
Image was already the interface that the outside world used. Toolkit was almost invisible to that outside world. So, if we rename ImageToolkit to Image not much is changing at the API level.
Think of things like:
ImageInterface::getToolkit() will be removed.ImageInterface::getToolkitId() will be replace by the already in the parent existing getPluginId().- ImageFactory becomes ImagePluginManager.
As usual, change record
https://www.drupal.org/node/2084547 →
(that details the new Image API) will be updated as soon as this patch gets committed.
