Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType

To add to the confusion:
- supportedTypes() is a static method on the toolkit interface but only called on instances.
- comment #2196067-11: Remove setWidth and setHeight from ImageInterface proposes to add these methods to the image factory as well: choose the toolkit to use based on support?

Thanks for taking a stab at it. I had a quick look at the patch. In general, it looks good, though I need to review in more detail, if all concerns are addressed.

• I especially like the getSupportedExtensions() as that is indeed what file.module needs and, as #2217783: Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead point out, should not be done outside the Image system as long as we use IMAGETYPE constants.
• Not sure though if that is the task of a factory.
• It does not rename the internal property $processed (which in turn may lead to renaming processInfo())
• We need to study if this handles creating new images without source (and if we want to handle that in this issue).
• I am not sure if I like a constructs like "$definition['class']::supportedTypes();" (I personally find call_user_func() a bit more explicit).

I will await the other issues (that are RTBC), to get a clearer view on this.

I think that your reasoning is correct: IMAGETYPE constants are GD specific (or at least oriented towards GD), so using it outside the GDToolkit is tricky at best, and your newest patch solves that.

I'm still not sure about a mime type getter on Image. So we can close the other issue if my question over there will be answered here.

Question:
The image system is about image manipulation not about file handling. If information about an image file is needed, think of extension or mime type, it should be approached as a file or stream (wrapper) not as an image object.
* Current usage 1: in image style delivery. An image derivative is created and subsequently served as the result of the request. This can be done by file handling.
* Current usage 2: image effects logging, why do we need to log the mime type, full file name is enough identification to do error hunting.

Changing title, but perhaps we even better move my question to issue #2217783: Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead. What do you think @mondrake?

usage 1a: function image_file_download($uri) in image.module
usage 1b: public function deliver(...) in ImageStyleDownloadController.php

usage 2: public function applyEffect(...) in all image effects: watchdog('image', 'Image crop failed using the %toolkit toolkit on %path (%mimetype, %dimensions)'

An example where a File is already used to get the mime type of an image fle: core\vendor\symfony\http-foundation\Symfony\Component\HttpFoundation\Tests\File\FileTest.php, public function testGetMimeTypeUsesMimeTypeGuessers()

Usage 1 is of the tricky type as I described in comment #5 of #2217783-5: Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead:
However, things will get a bit tricky here. Currently, our image class correctly determines mime type based on file contents, not file extension. It looks like the file and stream wrapper functions only do so based on file extension, not contents.
But on the public file system that is only the first time (on creation), after that Apache serves the mime type header based on extension. So I think the problem is not that big.

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -20,7 +20,7 @@
      /**
   -   * String specifying the path of the image file.
   +   * Path of the image file.
       *
       * @var string
       */
     protected $source;
   

   Initialize $source to '', so it always returns a string according to the specs. It would be better to also update the docs on this method to specify that it returns an empty string if no source is set (yet).

2. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -178,33 +152,34 @@ public function save($destination = NULL) {
   +    if (!is_file($source) && !is_uploaded_file($source)) {
          return FALSE;
   

   I think that is_uploaded_file() implies is_file(), so this looks a bit suspicious to me. Moreover, I'm afraid that this won't work with all stream wrappers, especially not remote stream wrappers (e.g. wrapping around a flickr image) or internal memory wrappers. As I don't think it is the task of the image system to restrict what type of sources can be read from, we should leave that check to the toolkit. Currently, that means calling getimagesize() (not GD specific, so can be moved to the base toolkit in another issue) which accepts all kinds of stream wrappers. If that returns FALSE, it is not an image, otherwise it is.

3. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -178,33 +152,34 @@ public function save($destination = NULL) {
   +    $this->setSource($source);
   +    if ($this->toolkit->parseFile($this)) {
   

   If I call setSource() directly (which I can, as it is part of the interface), nothing happens to the internal state of the image and toolkit object (ok, except for the property source). So I think that setSource() should call parseFile() and the constructor should call setSource() (if source was passed in).

4. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -178,33 +152,34 @@   protected function parseFile($source) {
   +      $this->valid = TRUE;
   +      return TRUE;
   

   On error, valid should be set to FALSE. Remember that we can change source along the way ...

5. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -13,20 +13,12 @@
   +   *   TRUE if the image objects contains a valid image, FALSE otherwise.
       */
   

   object (instead of objects)

6. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -185,19 +185,15 @@ public function scale(ImageInterface $image, $width = NULL, $height = NULL, $ups
   +   *   TRUE, if the file could be found and is an image. Otherwise, FALSE.
       */
   

   TRUE if ... , FALSE otherwise
   (remove comma after true and reverse last 2 words)

7. +++ b/core/modules/file/file.module
   @@ -409,13 +409,11 @@ function file_validate_size(File $file, $file_limit = 0, $user_limit = 0) {
   +    $errors[] = t('Image type not supported. Allowed types: %types', array('%types' => implode(' ', $supported_extensions)));
      }
   

   ... implode(' ,', ...); (add comma in glue string)

8. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -159,7 +171,11 @@ public function crop(ImageInterface $image, $x, $y, $width, $height) {
   +    if ($width < 0 || $height < 0) {
   +      return FALSE;
   +    }
   +
   

   I don't understand where this check is coming from. if we want a check here, I would check for <= 0 as resizing to 0 looks erroneous either.

9. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -291,15 +308,18 @@ public function parseFile(ImageInterface $image) {
   +    $source = $image->getSource();
   +
   +    $data = getimagesize($source);
    
        if (isset($data) && is_array($data) && in_array($data[2], static::supportedTypes())) {
   -      $details['type'] = $data[2];
   -      $this->load($image->getSource(), $details);
   +      $this->setType($data[2]);
   +      $this->load($source, $image->getType());
   +      return (bool) $this->getResource();
        }

   - empty line is not needed.
   - We can check on: if ($data && in_array(...)) { ... }
   - $this->getType()! (not $image->getType()). Probably passes tests due to __call in image :(

10. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    +  public function setType($type) {
    +    if (in_array($type, static::supportedTypes())) {
    +      $this->type = $type;
    +    }
    +    return $this;
    +  }
    

    Does this need to be public? To me, this is a bit like the setHeight/setWidth on image we removed because they do not make sense in isolation. I don't think that operations will need access to it, so protected is better. I also don't like the "error handling" or absence of it. So do not check what is coming in, or accept the consequences of checking: throw an error. The checking is done before calling this method and if we make it protected that remains so.

11. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
    @@ -62,35 +70,21 @@ public function settingsFormSubmit($form, &$form_state) {
    +    $source = $image->getSource();
    +
    +    $data = getimagesize($source);
    

    remove empty line.

12. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
    @@ -62,35 +70,21 @@ public function settingsFormSubmit($form, &$form_state) {
         if (isset($data) && is_array($data) && in_array($data[2], static::supportedTypes())) {
    -      $details['type'] = $data[2];
    

    if ($data && ...)

A few serious remarks, but most remarks are about small doc standards/typos.

What bothers me more is:
Image(Interface):
- Do we want to allow to change source after an image has been created? It really does complicate things: internal state like the valid flag, internal state on the toolkit. I think that we wouldn't loose any functionality by restricting an Image object to 1 source, being its initial source as passed in the constructor. Only upon saving and if the file gets saved to a different destination, we should change that property. If, at that point, we use direct access to the property to change it, we can remove the setSource() method.
- Do we really need getFileSize() and getMimeType() on ImageInterface? I think it is better done by leaving that up to File. The only place where it is used is in ImageStyleDownloadController where the image is handled as a file and thus a file object would be better.

GDToolkit:
- Use of $image as parameter in toolkit methods getWidth, getHeight and getMimeType. If I look at this in the code after this patch, it doesn't seem to make sense to me. I'm not sure why I didn't see it in the previous issue, but I would like to correct it here.
- Passing in $type to load(): I think that load() is capable of determining that itself, it is a property after all.

IMO, these 4 points can and should also be corrected in this issue. We are going halfway only now and I think this would really clean up the interfaces further.

Note: I also edited the summary a bit, but typos and small rephrasing only.

RE #15)

• 2a: IMO, those 3 lines should be removed completely as it is redoing what the toolkit has to do again and in more detail anyway. It would also mean that getimagesize() should be called with a @ in front to prevent warnings/notices on expected situations. However, as getimagesize() also emits an E_NOTICE if the file is not an image, this @ should be done anyway, even if we leave these line for now. Follow up?
• 2b: In PHP5.4 stream wrappers are handled completely transparently, and, on better reading the docs, is_file() will do as well, so that's no problem here.
• 2c: I guess you are right, though even then, it could serve as a base implementation that can be overridden by toolkits that want to support more file types.
• 3: This is so much better. So we now have 1 toolkit linked to exactly 1 image linked to 1 unchangeable source, except upon saving (and only then). So that means that we can also remove:
  - passing the source from Image::parseFile() to ImageToolkitInterface::parseFile(), provided we first set the source property, in the image constructor or in image parseFile).
  - passing the source to GDToolkit::load().
  - BTW: load is called with 2 arguments, while it has only 1 parameter left. Thus even if you do not change the above for now, that should be corrected.
• 4: It remains tricky as it relies on too much context and we keep the case of the need for reparsing after saving to a(nother) file. I would rather see something like (combining with 2a):
  

    protected function parseFile($source) {
    // Defer file parsing to toolkit.
      $this->valid = $this->toolkit->parseFile($this, $source);
      if ($this->valid) {
        ... (existing lines except the return)
      }
      return $this->valid;
    }

• 7: OK fine with me. I noticed that it had changed, so made a remark about it.
• 9: the check on is_array() is still there and still superfluous.
• 10 a: No, a GD resource has no type, so we can create one without having this info, but even if we want this info to be available during processing, it does not have to be public,.
• 10b: No, saving to another type is done by passing in save options. But as that issue is not in, leave it for now: I have added a comment to issue #2168511: Allow to pass save options to ImageInterface::save.
• Do we really need getFileSize() and getMimeType(): getWidth() and getHeight() are used when serving an html-img element, while file size and mime type are used when serving the image itself. So we indeed do not see a combined use of this in the (core) code base. I will file a separate issue, but under the image system, as the only current use is in image (image style download controller), so OK for now.
• Use of $image as parameter in toolkit methods: Your reasoning is OK, but should be solved differently. As a toolkit object is tied to exactly 1 image object, it is a natural case for a property (+ getter for the operations). However setting it is a bit difficult. It should be passed in via the constructor, but we can't, as we first create the toolkit object to pass that to the image constructor, so there is no image object upon construction time. We should solve this by adding a setImage(ImageInterface $image) method to the toolkit and calling that from the image constructor (subsequent calls should fail).
  If we do so, we can remove them from the operations as well. it would really clean up that part as well. (in operations it is only used to pass to getWidth/getHeight or to other operations (and a few error messages) ...)

[EDITED: response to 3.]

OK to defer:
- Removing $image parameter. Created child issue #2257587: Remove $image parameter in calls between and within Image and toolkit.
- 10: I already added a comment to that other issue.
- getFileSize() and getMimeType():I already created a child issue.

Let's do (based on #15)
- Let's do 2a/4, 3 , and 9 here (very small changes wrt to #15). (We let the other issue about using @ remain D7)
- Wrt to $source parameter: we create an image based on a source, the source cannot change after that anymore. So the source is an intrinsic property of an image object even if it is not valid. Moreover, let's not use source to determine validity, we did something like that with getResource() in the past and concluded that it was abuse of that getter. So let's change that here as well.

With those minor changes to #15, I will RTBC.

Nice cleanup.

I tried to test the thing about allowed file extensions, by adding svg to to the list of extensions on the image field on Article. However, the (js) client-side and server side validation kept telling me that only png gif jpg jpeg are allowed. This has nothing to do with this issue, but I could thus not manually test file_validate_is_image().

I will write the patch for child issue #2257587: Remove $image parameter in calls between and within Image and toolkit and post it as a combined patch (for the test bot) and as an interdiff (for manual review).

I added a comment to #1522348: Notice: getimagesize read error in image_gd_get_info() line 349 that it can remain D7.

A strange place to test it, as the logo will not undergo any manipulation, so the toolkit should not be involved. On testing I got:
- situation 1: placing an svg ifle on the root of the web site and adding its name into "Path to custom logo": no problem, logo works
- situation 2: Browsing the same file tos et the logo via the field "Upload logo image", error message:

    The specified file logo.svg could not be uploaded.
        - Image type not supported. Allowed types: png jpeg gif
        - Only files with the following extensions are allowed: jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp.
    The logo could not be uploaded.

I created a separate issue #2259567: Support SVG files for theme logo setting , but it did test the ImageFactory::getSupportedExtensions() method, so thanks.

Note to the core committer: the child issue #2257587: Remove $image parameter in calls between and within Image and toolkit contains a working and reviewed patch that combines this issue and that child issue. So, it is possible to:
- Review that patch in its entirety and commit/decline.
- Review this patch and the interdiff over there and commit/decline the patch over there (this as the interdiff over there is the difference between this patch and the combined patch in the child issue)
- Only review this patch and commit/decline, we will reroll the child issue by making the interdiff the patch.