Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead

See #2066219: Decouple image type from image extension for some background info. This comment is purely informational, no standpoints are taken.

Extension:
$extension is the case sensitive extension of the file, not related to the type other than by common agreement.
- Thus jpg, JPG, jpeg, JPEG are different $extensions, but relate to probably the same file type.
- The change file format image effect from imagecache_actions changes the file format ($type) but cannot change the path/filename - including the extension) of the derivative image URL.

If we need to store $extension separately from source and thus keep it a s separate property is debatable. Current usages:
- ImageWidget: this is a bug and should be replaced by isExisting(). (We can do so in this issue)
- Tests: ImageTest and ToolkitGdTest (the former can be removed as it tests exactly this property, the latter can be changed to use image_type_to_extension().

If we see an image object also as a file object, the extension property is meaningful, but I guess that in that case we should implement many more properties from the file interface on the image interface as well.

"Commonly used ..." is indeed misleading and should be corrected where it still appears.

Mime type:
I think that mime type and image type are more closely related and thus one can be removed.

Image type:
Sometimes, I play with the idea to build some support for webp, but this imagetype thing will give me a first headache (there is a constant and GD support for in PHP5.5). But OK, the other issue decided that this idea can be ignored in core.

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -113,9 +100,9 @@ public function isExisting() {
   -  public function getExtension() {
   +  public function getDefaultExtension() {
        $this->processInfo();
   -    return $this->extension;
   +    return Unicode::strtolower(image_type_to_extension($this->type, FALSE));
      }
    
   

   We can completely remove this function: it is not used anywhere and in its current state the Image interface does not provide file functionality.
   (this includes removing its test, the use ...unicode... statement, and adapting another test.

   If it will be decide that the function remains, this will return jpeg instead of the more commonly used jpg. I would like an explicit replacement built in for that. (and according to 1 of the user comments on the function documentation, the same for tiff => tif, although we do not support that yet, but Image is happily unaware of what is and what is not supported and if i built a toolkit that supports tif, I don't want to create my own image class and factory just to override this 1 method).

2. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -171,7 +158,7 @@ public function getType() {
      public function getMimeType() {
        $this->processInfo();
   -    return $this->mimeType;
   +    return image_type_to_mime_type($this->type);
      }
    
   

   This can be removed as well:
   - imagetype_to_mime_type is always available (not part of GD) and can always be used as replacement.
   Usages:
   - image.module: this is use of an image object as a file object: no image processing, noly file properoties: better use a file object or streamwrapper here.
   - ImageStyleDownloadController: same, use a streamwrapper.
   - Logging in all image effects: remove from message, the file name only is enough identification.
   - 2 Tests: 1 can be removed as it tests this method that can be removed; the other test is probably testing an image as file object.

   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 that the file and stream wrapper functions only do so based on file extension, not contents.

   This could be solved by letting file also do a content check first, using the fast exif_imagetype() function (according to the doc much faster than getimagesize()), but that would probably introduce another extension dependency, though this could be a soft one.

So, we should answer first:

1. Is Image(Interface) about image processing only or is it also about (image) file handling?
2. Remove get(Defaul)tExtension() from ImageInterface?
3. Remove getMimeType() from ImageInterface?
4. Adopt getMimeType() on LocalStreamWrapper (and File and possibly even other file classes) to do a check on the contents first to determine the mime type?
5. Include exif functionality for that?

I would say: only image processing; remove; remove; adopt; include (the latter 3 may be done in a (or even 2) follow-up(s)).

1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -93,7 +85,7 @@ public function getType();
   -   *   The MIME type of the file, or an empty string if the file is invalid.
   +   *   The MIME type of the image file.
       */
   

   Returns 'application/octet-stream' when type is not set or is not a valid IMAGETYPE_xxx constant. However, I would prefer to return an empty string, which means that this documentation should remain as is but that the implementation in Image should be adapted (to check for this string and replace it with the empty string).

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.php
   @@ -253,7 +254,7 @@ function testManipulations() {
   -        $image->save($directory . '/' . $op . '.' . $image->getExtension());
   +        $image->save($directory . '/' . $op . Unicode::strtolower(image_type_to_extension($image->getType())));
    
   

   Why bother converting it to lower case? The created file (or name or extension) is not used anywhere in the test. In fact, the saving of the image is completely superfluous (including preparing the directory). We can simply remove these lines.

3. \Drupal\Tests\Core\Image\ImageTest::setUp() still contains a reference to extension and mime-type:

       $this->toolkit->expects($this->any())
         ->method('getInfo')
         ->will($this->returnValue(array(
           'width'     => 88,
           'height'    => 100,
           'extension' => 'png',
           'type'      => IMAGETYPE_PNG,
           'mime_type' => 'image/png',
         )));
   

With a follow-up for mime-type, we are almost done here.

This test should not be about saving but about the correct operation of the toolkit operations (manipulations). However, a quick search did not reveal any other place where GDToolkit::save() is tested, so I made a remark about this in #2109459: Review image test suite.

That makes this patch RTBC as soon as #2196067: Remove setWidth and setHeight from ImageInterface gets in (unmodified wrt to that test setup).

Reviewing #8 (which looks cool otherwise) , I wonder if some case-sensitivity may be lost at :

-        $image->save($directory . '/' . $op . '.' . $image->getExtension());
+        $image->save($directory . '/' . $op . image_type_to_extension($image->getType()));

Previously, original.JPEG would go in and come out ..derivative/path/original.JPEG .
Won't this change produce ..derivative/path/original.jpg ?

We'd expect this to have a knock-on effect in places, surely? If I'm wrong, ignore me.

Drupal8NZ

I've followed the reasoning, and agree with the API improvement and all.
Sorry I didn't notice that this was ONLY about the test (though I wonder if a test should be treated as a special case on that merit alone :-)
I had done a short search for image_type_to_extension() to see how widespread it was.

I was just raising a small flag about assumptions that may be made about filenames from naive outside contrib modules that aren't aware that the API change will affect them. I saw this in earlier versions of lightbox etc that did string-munging to deduce the 'original' so was on the look-out for this sort of switcheroo.

And .. well basically we are all good here. I like. Sorry for the noise, and thanks for the clarification.

8: 2217783_remove-ext_8.patch queued for re-testing.

Changes look sensible - since we're removing a static cache has we got any idea of the performance hit - i.e. how many times a page are the functions like ::getMimeType() called? Plus we're missing a change record which is not required before commit.

To address your concerns:
- Change record is mentioned in #12.
- getMimeType() is only called by the ImageStyleDownloadController, thus when an image derivative is created and/or served by Drupal. For derivatives on the public file system this is once for the request that creates it, on the private file system this is once on each request.
- getExtension() was not used (in a correct sense) anywhere, so no performance hit here either.

So setting this back to RTBC.

Thanks @fietserwin!

Committed 2176bfd and pushed to 8.x. Thanks!