Updated: Comment #33.

Problem/Motivation

  1. Right now ImageInterface::getExtension() is used to check if an image is a supported image type or if the file is an image file. This is a bad pattern because:
    • The extension is a filesystem component that can lie about the file's nature,
    • A file having .PNG extension on disk may have a JPEG or even a non-image content,
    • Image type is not the same thing as the image extension.
  2. Being a filesystem component, the extension is not something that is linked to the toolkit (ImageToolkitInterface) but to the image (ImageInterface). Toolkits should have image types, images should have image type (talking about content nature) and extension (talking about external, filesystem file extension).
  3. Image types are determined (badly, by extension) in GDToolkit::getInfo() based on a hardcoded list of extensions.
  4. If a custom module wants to add a new image type (extension), he normally has to extend GDToolkit and fork GDToolkit::getInfo(). Ugly!
  5. The list of possible image types is something that should be provided by the toolkit. Some toolkits may provide more types than the other.
  6. Extension should be decoupled from image type. Images created from image resources (images not on disk) or temporary images have no extension while they still have an image type.

Proposed resolution

  1. Add supportedTypes method to ImageToolkitInterface.
  2. supportedTypes should return a list of IMAGETYPE_* constants. Note that IMAGETYPE_* constants are PHP constants and are not bundled to GD library.
  3. Declare a simple ImageInterface::isSupported() method in ImageInterface with implementation in Image that will be used to test if a image type is supported by the current toolkit or if a file is a valid image.
  4. ImageInterface::getExtension() should ALWAYS return the filesystem extension and will not be used for other purposes anymore. What would be the logic of returning an extension different from the real filesystem extension as long we will not use the extension as a way to check the image type or if a file is image?. ImageInterface::getExtension() will fallback to image_type_to_extension() for images created from resources or images without any extension on filesystem (like temporary files).
  5. Declare ImageInterface::getType() that returns the corresponding IMAGETYPE_* constant.

Remaining tasks

Rework #2063373: Cannot save image created from scratch

User interface changes

No changes in UI.

API changes

  1. New ImageTookitInterface::supportedTypes() static method. Implementations must declare the supported image types.
  2. New ImageInterface::isSupported() method. This is used if a image type is supported OR if a file is a valid image.
  3. New ImageInterface::getType() method. Used to test if an image is of a specified type.

Original report by claudiu.cristea

It turned out in #2063373-20: Cannot save image created from scratch that Image::getExtension() needs to check if the image extension is a valid one (.png, .gif, .jpg). It's not the only place. Extension are already checked in GDToolkit::getInfo(). This is a code duplication and, worst, a hardcoded list of extensions.

If a custom module wants to add a new image extension, he normally has to extend GDToolkit and fork GDToolkit::getInfo(). This is really ugly! But, if #2063373: Cannot save image created from scratch will get in, then will have to extend Image too and fork the ::getExtension(). In fact #2063373-20: Cannot save image created from scratch is wrong because the list of possible extensions is something that should be provided by the toolkit. Some toolkits may provide more extensions than the other.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
0 bytes

Patch.

larowlan’s picture

Empty patch :)

claudiu.cristea’s picture

Sorry :)

larowlan’s picture

@@ -180,4 +180,13 @@ function getInfo(ImageInterface $image);
+  static function getAvailableExtensions();

Missing 'public' ?

Status: Needs review » Needs work

The last submitted patch, image-extensions-2066219-1.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

@larowlan, it shouldn't be declared as public in the interface. All interface methods are public. The other methods of the same interface are without "public" too.

larowlan’s picture

of course you're right :)

claudiu.cristea’s picture

The problem are the JPEG images (IMAGETYPE_JPEG). There's one type and 2 extensions. image_type_to_extension(IMAGETYPE_JPEG) always returns .jpeg extension. That's why we need an additional check to get the extension from the path. Even then, an empty extension may be returned (e.g. temporary uploaded files have no extension). In such situation fallback to .jpg.

Note that before this patch the returned extensions for JPEG images was always .jpg regardless of how the file was saved on disk,

tim.plunkett’s picture

Issue tags: +Needs tests

I don't fully understand the need to add in that image_type_to_extension logic, can we have a test that justifies it?

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -221,8 +221,17 @@ public function getInfo(ImageInterface $image) {
    +      // image_type_to_extension(IMAGETYPE_JPEG) always returns 'jpeg' event if
    
    +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
    @@ -46,8 +46,17 @@ public function getInfo(ImageInterface $image) {
    +      // image_type_to_extension(IMAGETYPE_JPEG) always returns 'jpeg' event if
    

    s/event/even

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -221,8 +221,17 @@ public function getInfo(ImageInterface $image) {
    +        $extension = in_array($extension, self::getAvailableExtensions()) ? $extension : '';
    
    +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
    @@ -46,8 +46,17 @@ public function getInfo(ImageInterface $image) {
    +        $extension = in_array($extension, self::getAvailableExtensions()) ? $extension : '';
    

    s/self/static

it shouldn't be declared as public in the interface

grep -nr "public function" core | grep Interface.php | wc -l
1501
grep -nr "public static function" core | grep Interface.php | wc -l
22

grep -nr " function" core | grep Interface.php | wc -l
34
grep -nr " static function" core | grep Interface.php | wc -l
1

I'm going to have to disagree with you here. Which it is true you can't put protected/private methods on an interface, its in our coding standard to do so.

claudiu.cristea’s picture

Created followup for interface method declarations: #2066879: Apply coding standards for interface method declarations.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -221,8 +221,17 @@ public function getInfo(ImageInterface $image) {
+        $extension = in_array($extension, self::getAvailableExtensions()) ? $extension : '';

This should be static::getAvailableExtensions() in case someone wants to subclass.

claudiu.cristea’s picture

I don't fully understand the need to add in that image_type_to_extension logic, can we have a test that justifies it?

There's nothing special with image_type_to_extension(). It's the "PHP way" to map from image type (IMAGETYPE_* constants) to image extensions. What would be the alternative? Hardcoding another mapping array? I see this being the straight way for mapping (excepting the fact that is not completely handle the JPEG case).

I cleaned also the other interface methods by adding the "public" prefix.

I added also a test and a patch only with the test.

claudiu.cristea’s picture

Status: Needs work » Needs review

Oh, forgot to "needs review"

fietserwin’s picture

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.phpundefined
@@ -221,8 +221,17 @@ public function getInfo(ImageInterface $image) {
+      if ($data[2] == IMAGETYPE_JPEG) {
+        $extension = pathinfo($image->getSource(), PATHINFO_EXTENSION);
+        // It may be a temporary file, without extension. Fallback to 'jpg'.
+        $extension = in_array($extension, array('jpg', 'jpeg')) ? $extension : 'jpg';
+      }

Do we need to cater for extensions in uppercase? If so, this should be changed in the testtoolkit as well and it should be documented that the new method returns an array of lowercase extensions.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -179,5 +179,14 @@ function getInfo(ImageInterface $image);
+   *   A array of available image extensions.

I'm not a native English speaker but I think it is "An array ..."

claudiu.cristea’s picture

Good point, @fietserwin. The question is: How do we handle case inconsistencies?

Assuming that we'll change this line...

-        $extension = in_array($extension, array('jpg', 'jpeg')) ? $extension : 'jpg';
+        $extension = in_array(Unicode::strtolower($extension), array('jpg', 'jpeg')) ? $extension : 'jpg';

next Image::getExtension() may occur:

  • image.png => png
  • image.PNG => png
  • image.jpg => jpg
  • image.jpeg => jpeg
  • image (no extension but JPEG content) => jpg
  • image.JPG => JPG

You can see that .png and .jpg inconsistencies are handled not in the same way. PNGs (uppercase) are converted to lowercase while JPGs (uppercase) are kept as they are.

Needs decision! How will ImageToolkitInterface::getInfo return the extension?

  1. Lowercase regardless of the extension's case on filesystem.
  2. As they are on filesystem, lowercase for files created from resources or from valid image files with no extension (like temp files).
tim.plunkett’s picture

Just do $extension = Unicode::strtolower($extension); on the line before.

claudiu.cristea’s picture

I would bet on that :)

fietserwin’s picture

IMO, option 2 is better.

That said, and looking at the problems that might give, shouldn't the method being added be named getAvailableImageTypes (or getSupportedImageTypes) and return a list of IMAGETYPE_XXX constants. The getInfo() method would be changed to return the extension as is, or, if absent (e.g. new file), use image_type_to_extension() to define one and that one should be (will be) lowercase.

Though conceptually better, this might give new problems if e.g. the imagemagick toolkit wants to define more supported file types than GD supports. This because the IMAGETYPE_ constants and the image_type_to_extension() function are quite GD specific: Imagemagick toolkit would need to define its own constants and image_type_to_extension() may no longer be called in a non-strict GD environment (i.e. outside the GD toolkit class).

larowlan’s picture

Issue tags: -Needs tests

This looks ready to me

fietserwin’s picture

Status: Needs review » Needs work

I had a better look at the documentation for getInfo():

   * @return array
   *   FALSE, if the file could not be found or is not an image. Otherwise, a
   *   keyed array containing information about the image:
   *   - "width": Width, in pixels.
   *   - "height": Height, in pixels.
   *   - "extension": Commonly used file extension for the image.
   *   - "mime_type": MIME type ('image/jpeg', 'image/gif', 'image/png').

This changed my mind. It outdates #14 (case is not to be considered) and #18 (option 1 is actually better).

So IMO getInfo() should just return jpg (for jpg images, as that is the "commonly" used extension) and ignore the actual extension (jpeg vs jpg) and thus also the case (JPG vs jpg).

This will make the code easier as it only has to call image_type_to_extension() and thus does not have to do an if on type (jpg versus other) and does not have to call the pathinfo and strltolower functions.

Furthermore, I would still favor to decouple extensions and image formats a bit more, thus changing the name of the method to the one as suggested in #18 (getSupportedImageTypes) or to getSupportedImageFormats() (which I now prefer as imagetype is GD specific speak). It should return a list of strings that denote the supported image formats, probably being a list of the commonly used extensions. Thus 1 per image format, thus it should not return jpg and jpeg but only jpg.

claudiu.cristea’s picture

@fietserwin, remember that image_type_to_extension(IMAGETYPE_JPEG) always returns .jpeg

fietserwin’s picture

Not my preference or what I had expected, but I am OK with that. But: it will be an API change for getInfo() compared to the D7 image_gd_get_info() function, so perhaps handle that one differently?

$extension = str_replace('jpeg', 'jpg', image_type_to_extension($data[2], FALSE));
claudiu.cristea’s picture

Status: Needs work » Needs review

Thank you. Long story, indeed :)

For now I think that:

  1. Returning the right, lowercase extension for a file missing the extension (e.g. a temporary file).
  2. Returning the normalized, lowercase extension for files having case inconsistent extension.
  3. Preserving .jpg, .jpeg

... should be enough.

I'm not against abstracting more the toolkit and images objects but I think that should be a new issue. This issue was only to provide a unified way to check the extension of an image. Let's keep the issue inside the initial scope.

fietserwin’s picture

Fair enough, but then always return 'jpg' as D7 and current D8 do, otherwise we introduce an API change:

    $extensions = array('1' => 'gif', '2' => 'jpg', '3' => 'png');
    $extension = isset($extensions[$data[2]]) ?  $extensions[$data[2]] : '';
claudiu.cristea’s picture

That is a bug. The "official" extension for JPEG images is .jpeg. The short .jpg bacame popular because was used first on filesystems where the extension wS limited to 3 characters.

fietserwin’s picture

Bug or not, it should return the "Commonly used file extension for the image" (singular). Which one that is going to be is not that important to me, but it should be one, not 2 possibilities.

To prevent nasty bugs in code using this information, I prefer to not change the currently returned value thus going for jpg. But I will RTBC as well if it is consistently returning jpeg.

fietserwin’s picture

FileSize
5.91 KB
7.81 KB

This patch is BC with existing behavior (as it should IMO). We can do the suggested renaming (getSupportedImageFormats) in a later stage. but let's get this done so that the other issues blocked on this can progress.

claudiu.cristea’s picture

Status: Needs work » Needs review

I deliberately didn't came back with a patch because I needed more time to think on this. The question is: For what purpose is used, in fact, ImageInterface::getExtension()? (edited)

Let's see:

grep -nr --exclude-dir=core/vendor ">getExtension(" core 

This returns 16 occurrences:

  • 2 are unrelated -- one referring PHP $fileinfo->getExtension() and the other related to Twig.
  • 5 are used to test if file is image -- if ($image->getExtension()) {...}
  • only 9 (all in GDToolkit and Tests) are really care about extension.

So, yes, you're right, I agree that we should make now this step.

Brief plan:

  1. Add getSupportedTypes method to ImageToolkitInterface.
  2. [EDIT] getSupportedTypes should return one of the a list of IMAGETYPE_* constants or NULL. We need to test and make sure first that IMAGETYPE_* are NOT provided by GD extension but by PHP itself.
  3. Declare a simple isImage() method in ImageInterface with implementation in Image that will be used for file testing.
  4. ::getExtension() should ALWAYS return the filesystem extension and will not be used for other purposes anymore. What would be the logic of returning an extension different from the real filesystem extension as long we will not use the extension as a way to check the image type or if a file is image?
  5. ::getExtension() will fallback to image_type_to_extension() for images created from resources or images without any extension on filesystem (like temporary files)
claudiu.cristea’s picture

Title: Add getAvailableExtensions method to ImageToolkitInterface » Add getSupportedTypes() method to ImageToolkitInterface and isImage() to ImageInterface
Status: Needs review » Needs work

@fietserwin, I will start working on the fix based on #28. Can you check if IMAGETYPE_* are not GD dependant?

fietserwin’s picture

Status: Needs review » Needs work

In the PHP documentation they are introduced in http://www.php.net/manual/en/image.constants.php. Though the path does not contain GD in the url, it is found under: PHP Manual > Function Reference > Image Processing and Generation > GD > Predefined constants.

OTOH: The functions image_type_to_extension() and image_type_to_mime_type() have a note that This function does not require the GD image library.... I am not sure how to check this. I work on Windows and cannot (AFAIK) disable GD, but will check that tomorrow, to late now for me.

claudiu.cristea’s picture

@fietserwin, Yes the image_type_to_* functions are GD agnostic and it seems logical to me that IMAGETYPE_* must be too. But this is not documented anywhere.

fietserwin’s picture

#29:
It turns out that I could disable GD, some simple test after disabling revealed that:
- IMAGETYPE_xxx constants are available.
- Calling gd_info() resulted in a Fatal error: Call to undefined function gd_info().

#28:
- Not sure that we should go for IMAGETYPE_xxx constants, see my remark in last paragraph of #18. Why not going for mime types (for getSupportedTypes)? They are registered, so no confusion possible and they may be assumed to be complete (covering all image types that a toolkit may consider supporting). We then could use finfo_file (see http://www.php.net/manual/en/function.finfo-file.php) in isImage and check if it is supported with getSupportedTypes.
- getInfo should better remain BC (as in #27)

fietserwin’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updating summary.

claudiu.cristea’s picture

Title: Add getSupportedTypes() method to ImageToolkitInterface and isImage() to ImageInterface » Decouple image type from image extension
Assigned: Unassigned » claudiu.cristea

Updated title and summary according to #28.

claudiu.cristea’s picture

Issue summary: View changes

summary update

claudiu.cristea’s picture

Issue summary: View changes

Summary update.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
17.84 KB

Here's the patch based on #28 and the new summary.

EDIT (ADDED): There's no interdiff as is not relevant.

fietserwin’s picture

Status: Needs review » Needs work

I did a review purely on going through the diff (i.e. I did not even apply it and have a look at the resulting classes). So far, I did not find any shocking points. Below are some minor points I would like to see changed.

Personally, I still prefer to switch to mime types. We now have some kind of duplication with properties type versus mimeType and methods getType() versus getMimeType(). So why drop the concept of extension as base to decide image format specific code on and instead introduce a new concept 'type' while we already have mimeType. What, e.g., if a toolkit supports Google's WebP image format for which PHP has no IMAGETYPE_XXX constant?

But if you can convince me, I'm willing to accept the way as the current patch does.

  1. +++ b/core/lib/Drupal/Core/Image/Image.php
    @@ -63,6 +63,13 @@ class Image implements ImageInterface {
    +   * Image type (IMAGETYPE_JPEG, IMAGETYPE_GIF, IMAGETYPE_PNG).
    

    Can we make it clearer that these 3 are just examples, something like "One of the IMAGETYPE_XXX constants, e.g. IMAGETYPE_JPEG."

  2. +++ b/core/lib/Drupal/Core/Image/Image.php
    @@ -63,6 +63,13 @@ class Image implements ImageInterface {
        * MIME type ('image/jpeg', 'image/gif', 'image/png').
    

    Same here. We are doing this also to allow other toolkits to support more then just these 3 image formats.

  3. +++ b/core/lib/Drupal/Core/Image/Image.php
    @@ -99,6 +106,14 @@ public function __construct($source, ImageToolkitInterface $toolkit) {
    +    $this->processInfo();
    +    return in_array($this->type, $this->toolkit->supportedTypes());
    +  }
    

    return in_array($this->getType(), $this->toolkit->supportedTypes()); ???

  4. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -13,6 +13,14 @@
    +   * Check if the image is a valid image.
    
    @@ -65,6 +73,14 @@ public function setWidth($width);
    +   *   The image type (IMAGETYPE_PNG, IMAGETYPE_JPEG, IMAGETYPE_GIF).
    

    Checks if the image format is supported.

  5. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -65,6 +73,14 @@ public function setWidth($width);
    +   *   The image type (IMAGETYPE_PNG, IMAGETYPE_JPEG, IMAGETYPE_GIF).
    

    See above.

  6. +++ b/core/modules/file/file.module
    @@ -411,7 +411,7 @@ function file_validate_is_image(File $file) {
         $errors[] = t('Only JPEG, PNG and GIF images are allowed.');
    

    TODO: this error message becomes toolkit dependent...

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -98,7 +98,7 @@ public function rotate(ImageInterface $image, $degrees, $background = NULL) {
    +    if ($image->getType() == IMAGETYPE_GIF) {
    

    I Prefer === for clarity.

  8. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.php
    @@ -166,12 +166,12 @@ function save(ImageInterface $image, $destination);
        *   - "mime_type": MIME type ('image/jpeg', 'image/gif', 'image/png').
    

    see above.

  9. Removing extension from the array returned by getInfo() is IMO an API change and should be documented in the issue summary and later on in a change notice.
claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
3.34 KB
18.66 KB

@fietserwin, thank you for reviewing.

Personally, I still prefer to switch to mime types.

IMAGETYPE_* constants seems to be the most abstracted way in PHP to represent image types. Also, I'm afraid that mime types are not consistent enough. For example I see that there are 2 mime types for JPEG images: image/jpeg and image/pjpeg (http://en.wikipedia.org/wiki/Internet_media_type#Type_image).

Below some inputs on some of your points. The others are fixed in the interdiff.txt.

  • #35.3: As $this->type is a protected property and is accessible from the class, why calling the getter?
  • #35.7: I see no reason. We don't have casting issues there.
fietserwin’s picture

  • Well, ... IMAGETYPE_xxx constants are not complete enough. The inconsistency mentioned seems to be an IE internal thing only and is (AFAICS) not listed on the IANA site.
  • #35.3: just 1 method less that needs to know about processInfo(). Moreover, it's a safe and sound programming principle: if a class defines getters and setters, they should be used in its own code as well.

But this is RTBC for me, even if you keep disagreeing on those points.

claudiu.cristea’s picture

FileSize
542 bytes
18.64 KB

#35.3: just 1 method less that needs to know about processInfo().

That really makes sense to me (dropping one processInfo()). Let's change it :)

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

As said, RTBC, even while I prefer to switch to mime types., but let's move on (back) to the issues that are on the origin of this refactoring.

tim.plunkett’s picture

I've largely stayed out of this issue, mostly because @claudiu.cristea and @fietserwin both know the actual implications of the change better than I. But now that they've come to a consensus for this issue, I can vouch for the patch, and +1 RTBC.

alexpott’s picture

Title: Decouple image type from image extension » Change notice: Decouple image type from image extension
Priority: Normal » Major
Status: Reviewed & tested by the community » Active

Committed 156bdc4 and pushed to 8.x. Thanks!

claudiu.cristea’s picture

Title: Change notice: Decouple image type from image extension » Decouple image type from image extension
Priority: Major » Normal
Status: Active » Needs review
Issue tags: -Needs change record
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Change notice looks good.

fietserwin’s picture

Made some language corrections (singular -> plural), no conceptual changes, except changed "default toolkit" to "active toolkit".

claudiu.cristea’s picture

Assigned: claudiu.cristea » Unassigned
Status: Reviewed & tested by the community » Fixed

I think that it is safe to set this as fixed.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.