Merge Image and ImageToolkitBase

(As expected) I fully agree with this idea. I updated the summary with the original reasoning behind this idea and further follow-up work.

I will try to review shortly.

1. diff --git a/core/lib/Drupal/Core/Image/ImageInterface.php b/core/lib/Drupal/Core/Image/ImageInterface.php
   

   use Drupal\Core\Image\ImageInterface;
   is no longer used

2. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -13,6 +13,20 @@
      /**
   +   * Determines if a file contains a valid image.
   +   *
   +   * Drupal supports GIF, JPG and PNG file formats when used with the GD
   +   * toolkit, and may support others, depending on which toolkits are
   +   * installed.
   +   *
   +   * @param @todo
   +   *
   +   * @return bool
   +   *   TRUE if the file could be found and is an image, FALSE otherwise.
   +   */
   +  public function parseFile($source);
   +
   

   Does this have to be part of the interface? I cannot find any real usage, only test and upon creation? If it is needed for testing purposes, we should find another way as to not clutter the interface. (If it should be part of the interface there's a @todo.)

   In the merging we lost the source parameter on the constructor. I propose to reintroduce that. In doing so we can remove this method. Note that we currently cannot change the source after an Image object has been created and, IMO, that should remain so.

3. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
   @@ -79,23 +85,118 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
   +    if ($this->doParseFile()) {
   +      $this->fileSize = filesize($this->source);
   

   Add doParsefile() as an abstract method to document what toolkit implementers should do. Same for doSave() (see e.g. ImageToolkitOperationBase).

I'm happy that a first merge can be done with a relative small sized patch. I have identified some follow-ups but those will not really change the idea behind this patch and as such are not necessary to do directly in this patch.

ad 1) there's a now no longer used use statement at the top of that file.

ad 2) Then name it setSource() or createFromSource() as that mathces better with createNew())

This class used to be a value object. Whatever broken state it is in now was caused by the refactoring done since it was added.

re #16:
i did a new review and, given the follow-ups, I find this patch acceptable and a very good step into the right direction, therefore: RTBC, and I will create the follow-ups shortly.

re #17:
I am not sure what class you are talking about when you refer to "this class":
- if it is Image: that indeed used to be some kind of value object (not fully though), but that class will be gone with this patch.
- If you mean ImageToolkit (i.e. an object implementing ImageToolkitInterface): IMO this has never been a value object, it was and is the "in memory representation of an image (~ canvas) including a hint to its origin/destination". As such it has never been a value object, as we should be able to apply image operations to it that change the image canvas, as well as that we can save to another destination than its origin (image derivative), thereby changing the file info as well (that is currently also already done that way in Image).

If you are not happy with the createFromSource() method: neither am I, but due to @mondrake's reasoning in #14.2, I think that is better addressed in the follow up that will merge the image factory and image toolkit manager.

Created follow-up: #2336811: Remove Image, we only need ImageToolkit.

We need the issue summary to be updated with what the final patch is doing. And it'd be nice to have one CR that all the Image / ImageToolkit architecture issues link to.

I meant the class you are removing. Drupal\Core\Image\Image. It is a value object that represents an image, and now it has been consumed by ImageToolkit, which now represents two things.
And now the follow-up merges two more distinct classes.

By the time this is all done, we will have monolithic multipurpose classes, with no separation of concerns.

RE #21: Thanks for clarifying, but I can not fully agree with you.

IMO, Image is a mixture of 1) redundant file functionality and 2) a proxy to image toolkit functionality.

ad 1)
Image contains getFileSize, getMimeType and getSource(). To me this is functionality better handled by a file class. Looking at current usage, there is no place where an Image object is used as both a file and as an image canvas. To get these methods out of Image, I filed (a while ago) issue #2257163: Restrict image system to image processing.

ad 2) All other methods on Image are forwarded to the toolkit object. Most without any further processing. Some with some default processing that is assumed to be the same for all toolkits or is needed to keep its own state correct. The former can be handled by a base class implementation of that method or, as this patch does, via a template pattern. The latter disappears on merging.

The way this patch (and other issues) is heading to, is the way that the Imagine library handles different "toolkits". You have an Image interface defining methods to open, alter and save an image (file) and you have actual implementations in the form of a Gd\Imagine class or an Imagick\Imagine class, that alter the image using the given toolkit. This is simple and plain OO, data and operations in 1 object, common functionality in a base class, and in this case I don't see a need to make it more complicated than that.

I updated the title and issue summary. I will update the issue summary of the follow up as well.

The "overall" change record is https://www.drupal.org/node/2084547. As this change record is already published, we prefer to update it directly after committing issues.

In talking to @catch about this issue I ended up repeating some of the statements of the issue summary - if you want to treat an image as a file use the file object and I was wondering what Imagine does. So I went to look at Imagine and it does not really do this. If you look at documentation https://imagine.readthedocs.org/en/latest/ it is up to the app that uses image to decide what toolkit to use. The way in which Image and ImageToolkit are coupled using the plugin manager is how we do this in Drupal 8. So Image is a proxy - it is a proxy onto the chosen toolkit - I think this is okay.

But the image is not a plugin - the toolkit is - the image is composed of both file and an instance of the toolkit plugin. But yep if we are going to do this now we need to do whatever we do in one step - halfway steps aren't practical now since they move us further away from release - so I'm closing this issue to continue discussions on #2336811: Remove Image, we only need ImageToolkit