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.

CommentFileSizeAuthor
#5 interdiff_4-5.patch20.22 KBmondrake
#5 2336811-5.patch171.9 KBmondrake
#4 interdiff_1-4.txt8.22 KBmondrake
#4 2336811-4.patch155.97 KBmondrake
#1 2336811-1-do-not-test.patch132.03 KBmondrake
#1 2336811-1.patch154.77 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Active » Needs review
FileSize
2336811-1.patch154.77 KB
2336811-1-do-not-test.patch132.03 KB

First go.

This patch, built on top of #2331481: Merge Image and ImageToolkitBase, removes the 'toolkit' layer and terminology from core, and converts 'images' to plugins. It introduces an ImageManager plugin manager that replaces ImageToolkitManager and ImageFactory. Temporarily, to reduce the size of the patch, the image.factory service remains as an alias to image.manager. That could be cleaned in a follow up.

To summarize current vs. patch

Current status:
a) when an image is needed, code calls ImageFactory - this instantiates an Image object, and an ImageToolkit plugin object via ImageToolkitManager, associates the ImageToolkit plugin to the Image object, and returns the Image object to the caller. Image::parseFile() is executed at construction time to get the image from the file system.
b) when an image needs to have an operation applied, code calls Image::apply(). In turns, apply() gets an ImageToolkitOperation plugin object from the ImageToolkitOperationManager, passing the ImageToolkit object to it, and calls the methods from the ImageToolkitOperation that modify the content of the ImageToolkit object.

With this patch:
a) when an image is needed, code calls ImageManager - this instantiates an ImageInterface plugin object, executes ImageInterface::createFromSource() to get the image from the file system, and returns the ImageInterface object to the caller.
b) when an image needs to have an operation applied, code calls ImageInterface::apply(). In turns, apply() gets an ImageOperation plugin object from the ImageOperationManager, passing the ImageInterface object to it, and calls the methods from the ImageOperation that modify the content of the ImageInterface object.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented

First part, I still have to go through a number of files, but these are my remarks so far.

  1. +++ b/core/CHANGELOG.txt
@@ -170,8 +170,8 @@ Drupal 7.0, 2011-01-05
-    * Image toolkits are now provided by modules (rather than requiring a
-      manual file copy to the includes directory).
+    * Image toolkits are now replaced by image plugins provided by modules
+      (rather than requiring a manual file copy to the includes directory).
     * Added an edit tab to taxonomy term pages.

    I don't understand this change to the 7.0 part of the changelog. Perhaps create an addition in 8.0? 

  2. +++ b/core/core.services.yml
@@ -835,17 +835,16 @@ services:
   image.factory:
-    class: Drupal\Core\Image\ImageFactory
-    arguments: ['@image.toolkit.manager']
+    alias: image.manager
   breadcrumb:

    @todo and/or already create a follow-up issue? 

  3. +++ b/core/lib/Drupal/Core/Image/ImageBase.php
@@ -2,84 +2,84 @@
+use Drupal\Core\Image\ImageInterface;
+use Drupal\Core\Plugin\PluginBase;

    No longer needed: same namespace 

  4. +++ b/core/lib/Drupal/Core/Image/ImageBase.php
@@ -142,17 +157,44 @@ public function save($destination = NULL) {
-    return $this->apply('crop', array('x' => $x, 'y' => $y, 'width' => $width, 'height' => $height));
+   return $this->apply('crop', array('x' => $x, 'y' => $y, 'width' => $width, 'height' => $height));
   }

    accidentally removed space: restore 

  5. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
@@ -96,7 +135,7 @@ public function getToolkitId();
    * @param string|null $destination
    *   (optional) Destination path where the image should be saved. If it is empty
@@ -104,8 +143,6 @@ public function apply($operation, array $arguments = array());

    > 80 chars. 

  6. +++ b/core/lib/Drupal/Core/Image/ImageOperationBase.php
@@ -45,35 +45,35 @@
-   * Image toolkit implementers should provide a trait that overrides this
+   * Image plugin implementers should provide a trait that overrides this
    * method to correctly document the return type of this getter. This provides

    trait is a left over from a former patch: let's change it to base class here. 

  7. +++ b/core/modules/image/image.module
@@ -52,7 +52,7 @@ function image_help($route_name, RouteMatchInterface $route_match) {
-      $output .= '<p>' . t('The Image module allows you to manipulate images on your website. It exposes a setting for using the <em>Image toolkit</em>, allows you to configure <em>Image styles</em> that can be used for resizing or adjusting images on display, and provides an <em>Image</em> field for attaching images to content. For more information, see the online handbook entry for <a href="@image">Image module</a>.', array('@image' => 'http://drupal.org/documentation/modules/image')) . '</p>';
+      $output .= '<p>' . t('The Image module allows you to manipulate images on your website. It exposes a setting for using the <em>Image plugins</em>, allows you to configure <em>Image styles</em> that can be used for resizing or adjusting images on display, and provides an <em>Image</em> field for attaching images to content. For more information, see the online handbook entry for <a href="@image">Image module</a>.', array('@image' => 'http://drupal.org/documentation/modules/image')) . '</p>';
       $output .= '<h3>' . t('Uses') . '</h3>';

    why the change to plural. Moreover, I think this text is bogus, it is system that provides a default image plugin and setting, not the image module... not sure though if this thus should go the system module help.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Title: Merge ImageFactory and ImageToolkitManager » Complete the merger of Image and ImageToolkit
Issue summary: View changes
Status: Needs review » Needs work 
  1. +++ b/core/modules/system/src/Form/ImagePluginForm.php
@@ -110,12 +110,12 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
diff --git a/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php b/core/modules/system/src/Plugin/Image/GDImage.php

diff --git a/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php b/core/modules/system/src/Plugin/Image/GDImage.php
similarity index 93%

    Should (and can) the create method be moved to ImageBase? Why should toolkits have to repeat this boilerplate code? 

  2. +++ b/core/modules/system/src/Tests/Image/GdImageTest.php
@@ -17,14 +17,14 @@
-class ToolkitGdTest extends DrupalUnitTestBase {
+class GdImageTest extends DrupalUnitTestBase {

    I wouldn't mind to change the parent class to KernelTestBase in this patch as well. 

  3. rename from core/modules/system/src/Tests/Image/ToolkitSetupFormTest.php
rename to core/modules/system/src/Tests/Image/ImagePluginFormTest.php

    If I look at GDImage, I see that the form methods are named with ConfigurationForm. So, instead of renaming to ImagePluginFormTest, I propose to rename the class to ImagePluginConfigurationFormTest and the method to either testForm (as the class name gives enough context, my preference) or testImagePluginConfigurationForm. 

  4. rename from core/modules/system/src/Tests/Image/ToolkitTestBase.php
rename to core/modules/system/src/Tests/Image/ImageTestBase.php

    Rename the property imageFactory of this class to imageManager and adapt the type as well, it is now referring to a non-existing type.

    If renaming adds too much patch size (because it will also effect child classes, add a @todo and follow-up (or a remark to the review image test suite issue.

  5. We now have 2 GdImageTest classes (different namespaces though), can we already kill one, or has that to wait for the image test suite review issue?

This finishes my review. I also updated the issue summary and title.

If this patch gets too large, we could defer many renamings (variables and methods) to a follow-up, we could leave getToolkitId()

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
FileSize
2336811-4.patch155.97 KB
interdiff_1-4.txt8.22 KB

#2.1 - my bad :( reverted
#2.2 - leaving as is at the moment, I'd say we need a cleanup followup to remove instances of 'image.factory' in the codebase, as well as variable names and docs
#2.3 - done
#2.4 - done
#2.5 - done
#2.6 - done
#2.7 - reverted to singular, as well as the menus in the system module 'Image toolkit' => 'Image plugin'; moving the entire text somewhere else I'd leave for some documnetation issue if that needs be

#3.1 - it's a static method that contains plugin specific services needed - I'd leave that to be set explicitly in each plugin
#3.2 - suggest for a followup (there's an issue open already)
#3.3 - will do in a new patch
#3.4 - done, but see also reply to #2.2
#3.5 - one is a PHP unit test, the other a Unit test, would leave any merger (if possible) to a later stage

also rerolled

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
FileSize
2336811-5.patch171.9 KB
interdiff_4-5.patch20.22 KB

This patch removes the alias to 'image.factory' and all the remaining references to it in the code base, variable names and documentation, so no need for follow-up. Also renames the image plugin form test as per #3.3.

Status: Needs review » Needs work

The last submitted patch, 5: interdiff_4-5.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: Complete the merger of Image and ImageToolkit » Convert Image to plugin system, remove the 'toolkit' layer
Status: Needs work » Needs review

Sorry for the misnaming of the interdiff. Changing the issue title to better reflect what this patch is doing, after the parent issue was closed as duplicate.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

In the parent issue I explained my opposition to this. The new issue title makes it even clearer. Images are NOT plugins! This makes no sense. Toolkits are plugins, images are value objects.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented

I think it will need a reroll after #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in.

RE #8: You are right. the title is misleading as we are actually removing image (moving some yet to be removed methods to toolkit), not toolkit. But then we are renaming toolkit to image as that is the public facing interface name. We could stay with ImageToolkit though and change calls to Image to ImageToolkit. Looking at other image manipulation tools , I see that they tend to name the toolkit class like Image but a bit different, like: Imagick or Imagine. I don't have a real preference.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Title: Convert Image to plugin system, remove the 'toolkit' layer » Remove Image, we only need ImageToolkit
Issue summary: View changes
Parent issue: #2331481: Merge Image and ImageToolkitBase »

As the parent never got committed, I merged the issue summaries of the (former) parent and this one and removed the reference to it. i also better explained our ideas (I hope) to avoid confusion and start discussing the real architectural improvements behind this issue.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Version: 8.0.x-dev » 9.x-dev
Status: Needs review » Active
Issue tags: -beta target

I do not think there is a chance to align on this for D8, also based on the beta criteria for issues. Matter for D9?

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented

API/Interface changes would be minimal, only effect (toolkit operations)/toolkit contributors would have to rename/move some parts of their work, not even really update their code. So 8.1 seems feasible to me (perhaps with some b.c. hacks like making ImageToolkit derive from Image instead of removing it).

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch as a volunteer commented
Version: 9.x-dev » 8.1.x-dev
Status: Active » Postponed

Yes let's see what doable in minor releases,

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.