Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately

- When working with the GDToolkit, an image object has a resource.
- When working with the Imagemagick toolkit, an image has a set of parameters to be provided to the convert(.exe) binary?
So where do you want to store this information?

In other words:
- What do you want to do with the (logic in the) methods getResource(), setResource(), hasResource()?

I see an image as a dataholder, thus IMO, resource belongs to Image and effects (operations) should stay out of Image.

I'm against having different Image classes for different toolkits.

Where do toolkits store their image related data (resource for GD, ops for ImageMagick)? We should be be careful to make toolkit instance <- >image instance a 1-1 relationship. There is no need for it and the current way of instantiating does not assume this. I could instantiate a new instance (at this moment I even have to, as I cannot get the toolkit from an image instance) of toolkit and do some operations on an existing image. When ,subsequently, the original toolkit instance does the save operation, it will go wrong if the toolkit stores image related data in its own instance.

- I oppose to adding properties to objects without them knowing it (becasue in PHP, I can do this on any object: $object->myFunkyProperty = 3;)

- I prefer adding a type property (resource, array ops) above defining some unnamed and untyped bag/collection of data that anyone can add its data to. D8 prefers clean defined interfaces above passing around arrays.

I admit that I cannot see a reason for that, because Image factory is a system service. Being a service, you can decide to swap the class to be used by that service.

Assuming that a toolkit defines the image type, and I still stand by that, it should not be the case that implementing a toolkit means also defining a new image factory that swaps out the system provided one. Especially not because toolkits can live alongside each other (to the extend that they can operate on different images, not the same).

So tying image type to a toolkit means that the toolkit should create the instance, and thus the image factory should either call that toolkit method, or the toolkit is also seen as being the image factory, thereby combining the 2 classes into 1.

Added related issue.

Tying Image and Toolkit on a 1-1 relation is not what I prefer, because this simply means that they are actually one and the same, so it would be better to merge the 2 interfaces.

However, to get some progress, I am willing to accept this for now. But would like to see the following changes as well, mostly aimed at further hiding the artificial (or if you like historically grown) separation between image and toolkit:

1. Move width and height handling to ImageToolkit:
   • Add a getWidth() to ImageToolkitInterface.
   • getWidth() of Image should forward the call to and return the value from ImageToolkitInterface.
   • In Gd it should return imagesx($this->resource).
   • Remove setWidth() from Imageinterface. Note that a setWidth() on Image is a completely insane idea anyway and is only there because of the (historically grown) separation between Image and toolkit. Now that the "resource" moves to the toolkit this is no longer needed. Also note that the GD toolkit does not need a setWidth() as the width should come from the resource anyway and should not be stored separately. If the IM toolkit needs one, the IM operations will know as they will be programmed against the IM Toolkit, not the ImageToolkitInterface.
2. Same for getHeight()/setHeight().
3. To further hide the toolkit from the rest of Drupal, we should look at all direct usages of toolkit and see if and how we can stream this through Image:
   • Add supportedTypes() to ImageInterface. If image has an isSupported() method, it should also have its brother supportedTypes().
   • Use this to remove usages of toolkit->supportedTypes() (notably in file module)

Regarding the list of your changes:

1. move the resource handle ... As written above, OK with me for now.
2. move Image::getResource() ...YES
3. drop Image::hasResource() ... YES
4. the ImageFactory is creating a new instance ... Logical consequence
5. Image has also to have an Image::getToolkit()... Not sure if this is still needed if we can completely hide toolkit from the outside world.
6. the load() method in the GDtoolkit ... Good

Follow up for width/height problem is OK. But if we are going to tie the toolkit 1-1 to an image, there should no longer be a need to instantiate a toolkit separately in other code:

+++ b/core/modules/file/file.module
@@ -406,7 +406,7 @@ function file_validate_is_image(File $file) {
-    $toolkit = \Drupal::service('image.toolkit');
+    $toolkit = \Drupal::service('image.toolkit.manager')->getDefaultToolkit();
     $extensions = array();

If the image say it is not supported then ask the image what is does support. We can easily do so by adding that method to Image as well.

I will try to review the patch later on.

I guess that the get/setResource() indeed must stay public. Not only for tests, but also to give the (yet to be committed) toolkit operations access to the resource and even allow them to replace it with another resource, which is typically done by canvas operations, but also by the core crop operation (IIRC).

However, the only calls that we expect via __call are the methods that were removed in #2103635: Remove effects from ImageInterface. So I would propose to use a white list containing those methods instead of the black list you added in #17.

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -112,6 +105,16 @@ public function isSupported() {
   +    if (!$this->processed) {
   +      $this->processInfo();
   +    }
   +    return $this->processed;
   

   Remove the if around the statement: processInfo() already checks on processed.

2. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -301,6 +286,14 @@ protected function processInfo() {
   +    // @todo Temporary to avoid that legacy GD setResource(), getResource(),
   +    //  hasResource() methods moved to GD toolkit in #2103621 get invoked
   +    //  from this class anyway through the magic __call. Will be removed
   +    //  through https://drupal.org/node/2073759, when call_user_func_array()
   +    //  will be replaced by $this->toolkit->apply($name, $this, $arguments).
   +    if (in_array($method, array('setResource', 'getResource', 'hasResource'))) {
   +      throw new \BadMethodCallException();
   +    }
        if (is_callable(array($this->toolkit, $method))) {
   

   I would really like to see the white list here. Your argument about swapping in a new toolkit with new methods does not hold. That was not possible before #2103635: Remove effects from ImageInterface and was also not the idea of that issue. (It only shows how necessary it is to continue working on the image system.) Moreover, your change from getToolkitId() to getToolkit() in this very patch makes it possible to call new methods on toolkits and thus should not be done by __call().

   To assure that none of the patches that go in until __call() is replaced by apply() abuses this hole, it is better to explicitly check for what is allowed/expected. In fact it should have been part of the patch of the other issue.

3. +++ b/core/lib/Drupal/Core/Image/ImageFactory.php
   @@ -55,7 +63,10 @@ public function setToolkit(ImageToolkitInterface $toolkit) {
   +    if (!$this->toolkit) {
   +      $this->toolkit = $this->toolkitManager->getDefaultToolkit();
   +    }
   +    return new Image($source, $this->toolkitManager->createInstance($this->toolkit->getPluginId()));
      }
   

   So we need (or create) a toolkit instance to create a new one (of the same type) that will be tied to the Image that we are creating. Ugly. This is either a follow-up issue (that should be created before RTBC'ing this issue, although a patch can be postponed until after this patch gets in) or should be solved here and now.

   To solve I think that the toolkit manager should work with toolkitId's instead of toolkit instances.

4. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -112,6 +105,16 @@ public function isSupported() {
   +    return $this->processed;
   +  }
   

   Looking at the code, the property processed does not mean processed but existing or valid. So I propose to rename the property to isValid. This is a small change that should arguably be part of this patch. This patch is not only about cleaning up the interface with GD specific methods, but also about replacing some "abused" or incorrectly named methods with a correctly named counterpart. So renaming this internal property should go with it.

5. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
   @@ -63,15 +63,26 @@ public function getInfo(ImageInterface $image) {
   +  protected function load($source, array $details) {
   +    $this->logCall('load', array($source, $details));
   +    return TRUE;
      }
    
   

   Shall we just remove this method (and the call to it from getInfo). This is a test toolkit and does not have to resemble the GdToolkit. This doesn't seem to be appropriate here (anymore).

6. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -21,6 +21,14 @@
   +   *   TRUE if the image exists, FALSE if it is invalid.
   +   */
   

   TRUE if the image exists and is a valid image, FALSE otherwise.

7. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -136,6 +117,14 @@ public function setSource($source);
   +  public function getToolkit();
   +
   

   BTW: I'm fine with leaving getToolkitId() in, thus not replacing it with getToolkit() but just adding the latter.

8. +++ b/core/modules/file/file.module
   @@ -406,7 +406,7 @@ function file_validate_is_image(File $file) {
   -    $toolkit = \Drupal::service('image.toolkit');
   +    $toolkit = \Drupal::service('image.toolkit.manager')->getDefaultToolkit();
        $extensions = array();
        foreach ($toolkit->supportedTypes() as $image_type) {
          $extensions[] = Unicode::strtoupper(image_type_to_extension($image_type));
   

   Add a supportedTypes() to ImageInterface, see comment #13 and #16.

9. +++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
   @@ -138,26 +138,12 @@ public function testGetMimeType() {
   +    $this->assertTrue(!empty($resource));
      }
   

   All of the following seem to better express what this test wants to test: assertFileExists, assertNotEmpty or (IMO the best) assertTrue(is_readable(...))

Some background info on my remarks:
My remarks may be seen as scope creep for this patch. But they are not. By moving the resource property from Image to the GdToolkit (and thus in contrib the ops array of IM to the IM toolkit itself), this patch chooses to tie Image and ImageToolkit instances into a 1-1 relation. Doing so has some very important consequences that should be handled by this patch itself, not by a follow up.

• By tying these 2 classes we actually admit that in an ideal world they should be merged into 1 class. We don't do so for now because of the impact on the Image system and, more important, its test set.
• By tying these 2 classes we should recognize that it is better to hide 1 of the classes as much as possible and making the other the public facing one. In this case, Image is the public facing class (it already was) and toolkit should ideally be a protected property of Image but that is not possible as other parts of the Image system, and only that part of Drupal!, need access to it.
• Creating or accessing a toolkit instance outside the context of an image (or Image system or tests) should no longer be needed (hence the supportedTypes() on Image).

Further updated the issue summary, to reflect the currently chosen solution.

Thanks for reworking the patch. I will review later, but I can already say that I will agree on not doing;
- 20.2 white list: OK we leave it as is, it's temporary anyway.
- 20.4/5: OK: I will create follow up issues.

However, I stick with the idea behind:
- 20.8: the toolkit becomes an (internal) implementation detail that should no longer be called directly by external code (that's why the getToolkitId() may (should) remain aside of getToolkit()). We do everything via the image: load, isSupported, all operations, save, etc, but not this 1 method? Doesn't make sense: so we should get the toolkit out of all other code that is not part of the image system.

There is something strange as well here: To find out if the image is supported, there is call to getType, which in turn calls processInfo(), which in turn calls the toolkit's getInfo(). If this latter method would really be toolkit dependent, I may expect an error/exception on non-supported types, but there is nowhere any check on return values. processInfo() returns true on all files: supported images, non-supported images, and even on non images).: time for a follow-up issue after all? (should be combined in a follow up with the processed->isValid rename)

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -224,6 +199,14 @@ public function getToolkitId() {
   +    $this->processInfo();
   +    return $this->toolkit;
   +  }
   

   It is not obvious why that call to processInfo() is here. It is because methods in the toolkit (load/save) expect the processInfo(), or actually getInfo() in the toolkit, has already run. So adding a comment would be nice.

2. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -301,6 +284,14 @@ protected function processInfo() {
   +    //  through https://drupal.org/node/2073759, when call_user_func_array()
   +    //  will be replaced by $this->toolkit->apply($name, $this, $arguments).
   

   or will be replaced through issue 2110499 ...

3. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -21,6 +21,14 @@
   +   * Checks if the image is existing.
   +   *
   +   * @return bool
   +   *   TRUE if the image exists and is a valid image, FALSE otherwise.
   +   */
   +  public function isExisting();
   +
   

   If the image type is not supported, this method (or actually the implementation of it in Image) still returns TRUE. Is that what we want? Or are we going to sort that out in the follow-up?

4. +++ b/core/modules/image/lib/Drupal/image/Entity/ImageStyle.php
   @@ -288,7 +288,7 @@ public function createDerivative($original_uri, $derivative_uri) {
   +    if (!$image->isExisting()) {
          return FALSE;
   

   See above remark: && $image->isSupported(). Or are we going to sort that out in the follow-up?

5. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -22,6 +22,37 @@
   +   * An image file handle.
   +   *
   

   A GD image resource? See also remark below.

6. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -22,6 +22,37 @@
   +  /**
   +   * Sets the image file resource.
   +   *
   +   * @param resource $resource
   +   *   The image file handle.
   +   *
   +   * @return self
   +   *   Returns this image file.
   +   */
   +  public function setResource($resource) {
   +    $this->resource = $resource;
   +    return $this;
   +  }
   +
   

   - I would prefer consequent use of 'GD image resource' in the doc blocks (thus not image file handle, image file resource, as the resource has nothing to do with the file)
   - Returns this (GD)Toolkit

7. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -55,16 +86,17 @@ public function resize(ImageInterface $image, $width, $height) {
   +    $this
   +      ->setResource($res);
        $image
   

   1 line?

8. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -199,21 +232,29 @@ public function scaleAndCrop(ImageInterface $image, $width, $height) {
   +   * @param string $details
   +   *   An array of image details.
   +   *
   

   @param array $details

9. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -199,21 +232,29 @@ public function scaleAndCrop(ImageInterface $image, $width, $height) {
   +      $this->resource = $resource;
          if (!imageistruecolor($resource)) {
   

   Why don't we use the getter and setter internally as well? Using getters and setters internally is less error prone when overriding by inheritance or future change. (Multiple places)

10. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -199,21 +232,29 @@ public function scaleAndCrop(ImageInterface $image, $width, $height) {
    +      return (bool) $this->resource;
    

    Same here, see remark above

11. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -274,14 +315,18 @@ public function getInfo(ImageInterface $image) {
    +    if ($details) {
    +      $this->load($image->getSource(), $details);
    +    }
         return $details;
    

    So we do load not supported image types. Part of the follow-up or solve here?

12. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -274,14 +315,18 @@ public function getInfo(ImageInterface $image) {
    +   * @param string $type
    +   *   An image type represented by a PHP IMAGETYPE_* constant (e.g.
    +   *   IMAGETYPE_JPEG, IMAGETYPE_PNG, etc.).
        * @param int $width
    

    @param int $type

13. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
    @@ -63,15 +63,26 @@ public function getInfo(ImageInterface $image) {
    +   * @param string $details
    +   *   An array of image details.
    +   *
    

    @param array $details

Unlike this list might suggest, we are getting close. I think that the exact definition of isExisting() versus isSupported() is better sorted out in a follow-up. It is not this patch that introduces this. n general, I notices that when we introduced the isSupported()/supportedTypes() we only focused on external use of that (by the image/file field) not on using this ourselves to do some sanity checks.

#30 re#29.1: This processInfo() is only called in getters that depend on the info being available/initialized/processed. Therefore we have these getters (we can do some initialization in it). However, getToolkit() is not such a getter as it does not in itself depend on the source image being processed. Therefore, I was a bit surprised to see it being called anyway, but then discovered that it probably is needed to prevent errors further on, when methods on the returned toolkit are called. Therefore I wanted the comment, but this is so minor that I'm going to RTBC it anyway

I created follow-up issue #2211227: Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType.