Problem

Follow-up from #2211227: Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType.

The image system should be restricted to image processing, not image-as-a-file handling.

How does this show:

  • ImageInterface and ImageToolkitInterface both have a method getMimeType(). This information has no use during image processing, nor when processing html img tags, only when serving an image file. (and when saving an image, but that is until #2168511: Allow to pass save options to ImageInterface::save gets in.
  • ImageInterface has a method getFileSize(). This information has no use during image processing, nor when processing html img tags, only when serving an image file.
  • ImageStyleDownloadController creates an image object when it serves an image derivative. It uses this image to get the source, mime type, and file size. The source was already passed to the image constructor, so there's no Image object needed to get that info. The mime type and file size can as well be served by already existing file functionality.
  • image_file_download() kind of mimics file_file_download (it actually call this hook) but, in the end, uses an image object to get the Content-Type and Content-Length headers.

Proposed resolution

  • Remove use of getMimeType() from ImageInterface outside the image subsystem.
  • Remove getFileSize() from ImageInterface and Image.
  • Let ImageStyleDownloadController use standard, already existing, file functionality.
  • Let image_file_download() use standard, already existing, file functionality.

Note 1: the image system determines mime type based on content (thereby opening and reading the file), the file system determines the mime type based on extension. This might fail when we have image derivatives that were saved in another image format (we can't change extension of image derivatives). However, as soon as an image derivative is created on the public file system, the webserver will do the same as our file system. So this only differs for first time image derivative requests (when the derivative gets created by Drupal) and for requests on the private file system (that always pass through Drupal). IMO, no big deal thus.

Notes added after writing the patch:

  • This existing file functionality does indeed exist but not in 1 place: for the file size, filesize() is a simple PHP core function that can be used.
  • For mime type, Symphony adds mime type guessers based on content (finfo) and a binary guesser (file -b --mime command on linux). Drupal adds an extension based checker and a multi guesser. This patch defines a service around this functionality. Please check the logic in core.services.yml as I did not really know what I was doing: a bit of copy and paste from the existing guesser and some thought up naming.
  • This patch solves a bug: file_validate_image_resolution() in file.module used $file->filesize, a property on Drupal\file\Entity\File whereas $file is a FileInterface and it thus cannot be expected that it will always be a Drupal\file\Entity\File. This patch changes this into setFileSize().
  • This patch solves a documentation issue in ImageStyle.php.
  • The current implementation of image_file_download() (implementation of hook_image_download) checks if the file is valid and of a supported image format. This check can fail in edge situations and reduces flexibility for contrib to add e.g. webp support. This patch removes the check.
  • The current implementation of image_file_download() does not return NULL in other case, as the docs prescribe. This patch adds this omission.
  • This patch does not remove getMimeType() from Image as there are currently some issues in the queue that explicitly depend on mime type being available in image. #2168511: Allow to pass save options to ImageInterface::save will revisit this part.
  • Even if we cannot get to an agreement there are some changes in this patch that can still be committed. The errors it solves, the documentation error and the mime type service. It was @Jelle_S who notified me of the existence of mime type checkers and that this service can be used in #2340699: Let GDToolkit support WEBP image format as well.

Remaining tasks

- Agree on proposal.
- Review and accept patch.

User interface changes

None.

API changes

Will be documented in the main change record of the image system overhaul.

CommentFileSizeAuthor
#9 interdiff.txt560 bytesfietserwin
#9 restrict_image_system-2257163-8.patch13.14 KBfietserwin
#8 interdiff.txt554 bytesfietserwin
#8 restrict_image_system-2257163-8.patch13.14 KBfietserwin
#6 interdiff.txt4.27 KBfietserwin
#6 restrict_image_system-2257163-6.patch13.14 KBfietserwin
#3 interdiff.txt5.13 KBfietserwin
#3 restrict_image_system-2257163-3.patch13.05 KBfietserwin
#1 restrict_image_system-2257163-1.patch11.92 KBfietserwin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Issue summary: View changes
Status: Active » Needs review
FileSize
restrict_image_system-2257163-1.patch11.92 KB

First try. I updated the issue summary to follow status of the current patch.

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented
Status: Needs review » Needs work

Just a quick review (I didn't do any manual testing):

  1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -53,12 +61,15 @@ class ImageStyleDownloadController extends FileDownloadController {
+   * @param \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface $mimeTypeGuesser
+   *   The image factory.

    Copy-paste error: This is not an image factory. 

  2. +++ b/core/core.services.yml
@@ -1039,9 +1039,24 @@ services:
+      - { name: mime_type_guesser_extension }
+  mime_type.guesser.finfo:
+    class: Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser
+    tags:
+      - { name: mime_type_guesser_finfo }
+  mime_type.guesser.binary:
+    class: Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser
+    arguments: ['@module_handler']
+    tags:
+      - { name: mime_type_guesser_binary }

    I would add priorities to the services so we can control which one gets called first. I believe if we do it in the way it is currently implemented in the patch, the MIME type guesser that guesses the MIME type based on the extension will get called first (because it comes first in the YAML file), which is not reliable and will always result in skipping the other guessers because it defaults to "application/octet-stream" (doc link) and the multi guesser provided by Drupal returns the first MIME type it gets (doc link) 

  3. +++ b/core/modules/image/image.module
@@ -170,17 +170,22 @@ function image_file_download($uri) {
+    // - if it is a supported image format: we cannot know for sure what toolkit
+    //   was used to create this derivative and thus if it is a supported image
+    //   format.

    This part of the comment isn't entirely clear to me:

    if it is a supported image format: we cannot know [...] if it is a supported image format

    Why not? If it's supported, it's supported? I don't really see what the toolkit has to do with it?

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
restrict_image_system-2257163-3.patch13.05 KB
interdiff.txt5.13 KB

re #2: thanks for this quick review.

#re 2.1: Good catch. In fact, the goal of this issue is to remove usage of the image sub system when not doing image manipulation. So the image factory property/constructor parameter should have been deleted.

#re 2.2: The order is OK, I had debugged that. In the absence of the priority parameter, the order is determined by the order of calls to addGuesser() and that is determined by the 3 lines in this part:

  file.mime_type.guesser.content_based:
    class: Drupal\Core\File\MimeType\MimeTypeGuesser
    tags:
      - { name: service_collector, tag: mime_type_guesser_finfo, call: addGuesser }
      - { name: service_collector, tag: mime_type_guesser_binary, call: addGuesser }
      - { name: service_collector, tag: mime_type_guesser_extension, call: addGuesser }

This can indeed be made more clear, but I have no clue if I can pass a 2nd parameter by using this declarative syntax only.

Note: the Symphony provided finfo guesser always returns the value from finfo->file() though that can be FALSE according to the PHPdoc. So I think that guesser violates the interface documentation and our multi guesser strictly follows that interface (guess() returns the mime type or NULL and thus not FALSE and the check is on strictly NULL). I don't think that we should do something about that here.

#re 2.3: This comment was more describing the change then documenting the new code. After some further analysis, I concluded that the check on file_exist() always returns TRUE and thus can be removed. I added some documentation about it in the function doc.
FYI: it is the toolkit that defines which image formats it supports. Thus an ImageMagick based toolkit in contrib can define that it supports webp, bmp, etc. without the need to change the core provided GD toolkit. The former check could instantiate the GD toolkit to ask if the file is a supported image format: incorrect fail.

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented

#3.1 Cool!

#3.2 To add the priority to the guessers just add them to the individual declaration of the services I highlighted in #2.2 as a tag (the same way you added the name tag). For a working example see http://cgit.drupalcode.org/drupal/tree/core/modules/image/image.services...

#3.3 Ok, thanks for clarifying.

I haven't reviewed your patch in #3 since I'm on my tablet now.

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented

So besides the priorities I spoke about in #4.2 I found three more nitpicks:

  1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -33,11 +33,11 @@ class ImageStyleDownloadController extends FileDownloadController {
+   * The mime type guesser.

    Nit: s/mime/MIME 

  2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -51,14 +51,14 @@ class ImageStyleDownloadController extends FileDownloadController {
+   *   The mime type provider.

    Nit: s/mime/MIME

    guesser in previous comment vs provider in this comment. I'd make in consistent and go with guesser. 

  3. +++ b/core/modules/image/image.module
@@ -170,25 +176,22 @@ function image_file_download($uri) {
+        // Send headers describing the image's size, and MIME-type...

    Nit: s/MIME-type/MIME type
    [...] the image's size and MIME type. (lose the three dots at the end)

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
FileSize
restrict_image_system-2257163-6.patch13.14 KB
interdiff.txt4.27 KB

Thanks, all solved. Added a line in image_file_download that was in the previous interdiff but not in the previous patch.

BTW: I was flabbergasted by the fact that a higher number has a higher priority. To me this is counter intuitive: priority 1 is top priority. I found this while manual testing the mime type guessing, so that ha been done :)

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented

One final little nitpick (sorry to keep doing this but I assume core committers will catch these things as well and won't commit it as long as it's in there)

+++ b/core/modules/image/image.module
@@ -156,6 +156,13 @@ function image_theme() {
+ * This hook allows access if the user has access to the original and denies
+ * access when the use has not access to the original.

s/when the use/when the user (forgot the r).

has not access => has no access or does not have access

The rest of the patch looks great. I'll try to do some manual testing later today or maybe tomorrow.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
FileSize
restrict_image_system-2257163-8.patch13.14 KB
interdiff.txt554 bytes

Corrected, and no problem, I do the same when I review.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
FileSize
restrict_image_system-2257163-8.patch13.14 KB
interdiff.txt560 bytes

Forget #8, it's not correct English (I think).

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed

Me thinks should be postponed at this point. It is too late to make into 8.0.x

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Postponed » Needs work

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.