Updated: Comment #22

Problem

This issue is a spin off of issue #2073759: Convert toolkit operations to plugins, more specifically comment #71 (#2073759-71: Convert toolkit operations to plugins).

In that issue we discovered that it is difficult/impossible to cleanly add toolkit specific code for new image effects. On working towards a solution we found that defining a new toolkit (e.g. Imagemagick toolkit) is also going to be hard with the current design.

This issue focuses on making it more easy/logical to implement an ImageToolkit in contrib.

Problems:

  • Image/ImageInterface: define a GD resource handle and methods getResource(), setResource(), hasResource(). These are in fact GD specific and will only be ballast for other toolkits. (What should an imagemagick toolkit return in hasResource()?)
  • Contrib toolkits will not use a GD resource, but will have their own data to store. Where to store this:
    • In image: iin PHP it is technically possible to add your own properties, but this is not considered a good practice.
    • In toolkit instance: this will tie a toolkit instance to a specific Image instance but allows contrib toolkits to add properties as needed.
    • In their own newly defined MyImage class: the image factory from core is not made up to this task, so this will be difficult to implement.

Proposed resolution

We chose to go for the 2nd option above: tie toolkit instances to specific Image instances, so a toolkit can store all data it needs in its own object.

Concrete changes this patch implements:

  • move the resource handle from the image class to the GDToolkit plugin.
  • move Image::getResource() and Image::setResource() to the GDToolkit as well.
  • drop Image::hasResource() and replace with an Image::isExisting() method that will just return if an image is existing and 'manageable' for the toolkit.
  • the ImageFactory creates a new instance of the toolkit when creating a new image. This is because in this approach each Image instance will get a separate toolkit instance, so that image specific information managed by the toolkit can be stored at toolkit level. ImageFactory is receiving the toolkit manager in the constructor then, no longer the toolkit itself, and we no longer need 'image.toolkit' as a service.
  • the setToolkit() method in the ImageFactory is replaced by a setToolkitId() method, as the ImageFactory no longer needs to store an instance of the toolkit. Every Image will get its own toolkit instance. Also, the ImageToolkitManager has a new getDefaultToolkitId() method to respond to ImageFactory request for the ID of the default toolkit plugin.
  • Image has also an Image::getToolkit() method to retrieve the toolkit object stored in the Image instance. That is needed to enable tests and to allow access to the toolkit object by contrib code.
  • the load() method in the GDtoolkit becomes an internal protected method to the GDtoolkit.

Remaining tasks

  • Review patch.
  • Write change notice. (Update current change notice?)

User interface changes

None, this is pure DX.

API changes

Image system will be changed. As this is already completely different from D7, and image related contrib maintainers actively participate in these issues, that should not be a problem.

- #2073759: Convert toolkit operations to plugins.
- #2103635: Remove effects from ImageInterface.

Comments

claudiu.cristea’s picture

Title:Allow contrib to more easily implement image toolkits» Move GD logic from ImageInterface to toolkit

Remove GD specific methods from ImageInterface: getResource(), setResource(), hasResource().

I agree 100% and I stand for this. We need to make ImageInteface toolkit-agnostic.

Split current class Image into 2 classes: BaseImage and GdImage (GDImage?).

It's not so clear to me why we would do that. While Image will became an abstraction having no toolkit logic it seems to me useless. Can you provide more details?

- Extend ImageToolkit with an image factory method, As Image's are toolkit specific, this is logical.
- Remove ImageFactory: this class becomes superfluous if toolkit takes over that role

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. For particular site you may use you own image class. And, again, I don't think we want Images that are toolkit specific. We want the default implementation of ImageInterface which is Image to work with any toolkit is provided. I'm against having different Image classes for different toolkits.

I would keep this issue only to remove the GD logic from image. But let's discuss this and if you are right I will add other tickets for that.

Changed the title and created a meta-issue for all image work here #2105863: [meta] Images, toolkits and operations.

fietserwin’s picture

- 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.

claudiu.cristea’s picture

Priority:Critical» Normal
fietserwin’s picture

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.

fietserwin’s picture

Issue summary:View changes

Added related issue.

mondrake’s picture

Issue summary:View changes
Status:Active» Needs review
Parent issue:» #2105863: [meta] Images, toolkits and operations
StatusFileSize
new43.96 KB
FAILED: [[SimpleTest]]: [MySQL] 59,691 pass(es), 4 fail(s), and 6 exception(s).
[ View ]
new26.42 KB

So, I know we are still in outer space here, nevertheless I wanted to give a first try.

This patch is on top of
#2122605: Remove isAvailable() from ImageToolkitInterface
which itself is on top of
#1069140: Requirements should be provided by image toolkit, and
#2111263: Toolkit setup form displays settings for multiple toolkits.

the do-not-test patch is the topping one, the other is just for testing.

The idea:

  • move the resource handle from the image class to the GDToolkit plugin.
  • move getResource() and setResource() as well.
  • drop hasResource() and replace with an isExisting() method that will just return if an image is existing and 'manageable' for a toolkit.
  • the ImageFactory is not receiving a pre-instantiated toolkit anymore, but just creating a new instance of the toolkit when creating a new image. This is because in this approach each Image instance will get a separate toolkit instance. ImageFactory is receiving the toolkit manager in the constructor then, no longer the toolkit itself, and we no longer need 'image.toolkit' as a service.
  • Image has also to have a getToolkit() method for testing purposes, anticipation of #2108339: Image toolkit getter.
  • the load() method in the GDtoolkit becomes internal, maybe we can remove from the interface and make it protected; Imagemagick may take a different approach (i.e. just testing file existence)

Status:Needs review» Needs work

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

mondrake’s picture

StatusFileSize
new4 KB
new28.34 KB
new28.34 KB

Fixed around some test failures.

mondrake’s picture

Status:Needs review» Needs work

Testbot failed to update #5 to NW

mondrake’s picture

Status:Needs work» Needs review

Back to NR for bot to process #7

mondrake’s picture

StatusFileSize
new45.88 KB
PASSED: [[SimpleTest]]: [MySQL] 60,398 pass(es).
[ View ]

Oops

mondrake’s picture

StatusFileSize
new45.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es).
[ View ]

Re-roll of #7 after #2103635: Remove effects from ImageInterface went in. No changes, no interdiff.

mondrake’s picture

StatusFileSize
new24.33 KB
FAILED: [[SimpleTest]]: [MySQL] 64,101 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

A new patch, this time against head and doing the bare minimum (it's no longer on top of #2122605: Remove isAvailable() from ImageToolkitInterface). The patch size is halved and I hope this can get more reviews now.

This still does:

  • move the resource handle from the image class to the GDToolkit plugin.
  • move Image::getResource() and Image::setResource() to the GDToolkit as well.
  • drop Image::hasResource() and replace with an Image::isExisting() method that will just return if an image is existing and 'manageable' for a toolkit.
  • the ImageFactory is creating a new instance of the toolkit when creating a new image. This is because in this approach each Image instance will get a separate toolkit instance. ImageFactory is receiving the toolkit manager in the constructor then, no longer the toolkit itself, and we no longer need 'image.toolkit' as a service.
  • Image has also to have an Image::getToolkit() to retrieve the toolkit object stored in the Image instance. That is needed here just to enable some tests, but is an anticipation of #2108339: Image toolkit getter, which will become a duplicate if this moves in.
  • the load() method in the GDtoolkit becomes internal to the toolkit. We could even remove it from the interface and make it protected; Imagemagick may take a different approach (i.e. just testing file existence)
fietserwin’s picture

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

Status:Needs review» Needs work

The last submitted patch, 12: gd_move-2103621-12.patch, failed testing.

mondrake’s picture

Status:Needs work» Needs review
StatusFileSize
new4.24 KB
new24.53 KB
PASSED: [[SimpleTest]]: [MySQL] 64,333 pass(es).
[ View ]

a) I missed to revert one change to the ToolkitGdTest from patch in #11, corrected here.

b) Removed load() from ImageToolkitInterface and changed to a protected method in GDToolkit and TestToolkit.

c) In ToolkitTestBase, this change was not making sense

@@ -71,7 +71,7 @@ protected function getImage() {
     $image = $this->container->get('image.factory')
       ->setToolkit($this->toolkit)
       ->get($this->file);
-    $image->getResource();
+    $image->isExisting();
     return $image;
   }

so changed to an assert

@@ -71,7 +71,7 @@ protected function getImage() {
     $image = $this->container->get('image.factory')
       ->setToolkit($this->toolkit)
       ->get($this->file);
-    $image->getResource();
+    $this->assertTrue($image->isExisting(), 'Image was loaded.');
     return $image;
   }

@fietserwin re #13 please open follow-ups for those, let keep this as small as possible

fietserwin’s picture

Title:Move GD logic from ImageInterface to toolkit» Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately
Related issues:+#2196067: Remove setWidth and setHeight from ImageInterface

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.

mondrake’s picture

StatusFileSize
new1.1 KB
new25.43 KB
PASSED: [[SimpleTest]]: [MySQL] 64,405 pass(es).
[ View ]

Found a problem.
Since in current head we have in Image a __call magic method that will pass any method not defined in Image to the underlying toolkit, setResource() and getResource() can still be invoked in a call from Image, as they are public methods in the toolkit (and need to be so, for testing purposes and to allow temp (contrib) code to access the GD resource if it needs).
This causes problems if there's temp code still making such calls through $image->getResource() and not through $image->getToolkit()->getResource(). (I had such a case in my wip conversion of Textimage).
This will go away when we drop the __call method from Image, but for the time being and to avoid mess, I introduced a check to fail hard with an exception in case of a $image->setResource (or getResource, or hasResource).

fietserwin’s picture

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.

mondrake’s picture

Well no, because if someone wants to swap the GDToolkit class with her/his own in the plugin, having additional operations in there (that would be the only alternative right now to extend the toolkit), s/he needs to access them through the current model $image->operation(...). The blacklist is really only for safety around the xxxResource() calls.

fietserwin’s picture

  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).
mondrake’s picture

StatusFileSize
new25.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,848 pass(es).
[ View ]

Reroll of #17 because of #2053153: Allow contrib modules to provide plugins on behalf of optional modules; no changes to the patch code.

mondrake’s picture

Issue summary:View changes
Issue tags:+API change

Updated issue summary with proposed resolution in #12.

mondrake’s picture

Issue summary:View changes
fietserwin’s picture

Issue summary:View changes

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

mondrake’s picture

StatusFileSize
new25.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,896 pass(es).
[ View ]
new1.52 KB

@fietserwin re #20

  • 1 - OK done here
  • 2 - disagree on a white list, I'd rather remove that entire piece. Someone may alter the class instantiated by the GDTollkit plugin and leverage on the __call method to call their own operations, and a white list would stop that.
  • 3 - OK done in #26 I agree but doing that here would imply lot of changes to the ImageFactory (the factory has an instance of the toolkit as a protected property, we would need to change it to the toolkit ID but also change the setToolkit method to setToolkitId method, adjust test etc. - will double the size of this patch, so I suggest to open a followup
  • 4, 5 - fine but followups please
  • 6 - OK done here
  • 8 - disagree, it should be the toolkit (or the ImageFactory) to tell me what types it can support. An instantiated Image has a specific type. If I want to change its type I should ask the toolkit if it's able to convert it to whatever new, and then change it.
  • 9 - OK done here.

EDIT - reflected changes to point 3 as implemented in #26

mondrake’s picture

StatusFileSize
new29.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,893 pass(es).
[ View ]
new6.21 KB

So... second thinking... point #20.3 addressed here. We are removing the setToolkit() method in ImageFactory and replacing with a setToolkitId() method. The concept is to avoid having a dummy, useless toolkit instantiated in the factory, as in any case the factory will instantiate a new one for every new Image object. This requires a mention in the issue summary.

mondrake’s picture

Issue summary:View changes

Updated issue summary for the changes in #26

fietserwin’s picture

Issue summary:View changes

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)

fietserwin’s picture

Status:Needs review» Needs work
  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.

mondrake’s picture

StatusFileSize
new30.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,084 pass(es).
[ View ]
new6.67 KB

#29:

1. I also do not like how processInfo() is used in each getter method in the Image class, but this is not introduced in this patch and the implementation of getToolkit() here is just following that pattern. I agree we can find a better way, but not here pls.

2. OK

3, 4, 11. Added a check if the type is supported in toolkit's getInfo() method.

5, 6, 7, 8, 9, 10, 12, 13. OK thanks

mondrake’s picture

Status:Needs work» Needs review
mondrake’s picture

Issue summary:View changes

Changes to issue summary in #27 were inadvertently reverted in #28 (xpost)

fietserwin’s picture

Status:Needs review» Reviewed & tested by the community
Related issues:+#2211227: Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType

#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.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed f437807 and pushed to 8.x. Thanks!

We need a change notice to cover all the change being implementing by #2105863: [meta] Images, toolkits and operations

mondrake’s picture

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.