Separate MIME type mapping from ExtensionMimeTypeGuesser

thanks for opening the issue

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -921,13 +58,13 @@ public function guess($path) {
   +  public function getMapper() {
   
   +++ b/core/modules/system/src/Tests/File/MimeTypeTest.php
   @@ -84,7 +84,7 @@ public function testFileMimeTypeDetection() {
   +    $extension_guesser->getMapper()->setMapping($mapping);
   

   I do not see the reason to have this public getter, just retrieve the mapper from the $this->container

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,921 @@
   +class MimeTypeMapper {
   

   Needs an interface, so that its swappable

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -912,22 +49,11 @@ public function guess($path) {
        //   - awesome.image.jpeg
        while ($additional_part = array_pop($file_parts)) {
          $extension = strtolower($additional_part . ($extension ? '.' . $extension : ''));
   -      if (isset($this->mapping['extensions'][$extension])) {
   -        return $this->mapping['mimetypes'][$this->mapping['extensions'][$extension]];
   +      if (isset($mapping['extensions'][$extension])) {
   +        return $mapping['mimetypes'][$mapping['extensions'][$extension]];
          }
        }
   

   One thing that we were discussing was if and how much of this logic should move to the mapper?

   Right now, there is just getMapping(). But that's a very arbitrary structure, wouldn't it be nice to have getMimeTypes() too, if you just need those?

   And if we go there, would it be useful if the mapper would possibly even have a method like getMimeTypeForExtension($extension)? Then this would be re-usable if you'd just have a extension and not an actual file. Not sure if we should consider that at all or in a separate issue...

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapperInterface.php
   @@ -0,0 +1,36 @@
   +  /**
   +   * Replaces the mimetypes/extension mapping.
   +   *
   +   * @param array|null $mapping
   +   *   Passing a NULL mapping will reset to the default mapping.
   +   */
   +  public function setMapping(array $mapping = NULL);
   

   Why do we need a setter? If you want to alter it, use the hook, this seems strange.

totally agreed with 1.

2. is strange indeed and was there pre-conversion, see also #1921558-17: Convert file_get_mimetype() to use Symfony MimeTypeGuessers.
In other words, the hook will only fire if the default mapping is used: The setter overrides the hook.

I very much support to make the hook fire *always* and get rid of the setter...or the other way around

#2388749: Register symfony's mime guessers if they are supported is related.

#2388749: Register symfony's mime guessers if they are supported has been committed, and it removed hook_file_mimetype_mapping_alter().

Rerolled patch in #8 (see do-not-test patch), and removed the alter call here too. Interdiff against the reroll.

Would love to see this in as there was no straightforward way in contrib to map extensions to mimetypes before #2388749: Register symfony's mime guessers if they are supported and also hacks like this

/**
   * {@inheritdoc}
   */
  public function getMimeType($extension) {
    return $this->mimeTypeGuesser->guess('dummy.' . $extension);
  }

now no longer (legitimately I'd say) work.

What is the reasoning for removing the alter hook? This hook has been *very* useful for contrib to easily provide support for new file types when adding them to core would take much longer.

@dave see #2388749-10: Register symfony's mime guessers if they are supported

Fixed PHPUnit test failure.

I dont think we should expose the mapping array, it is implementation details.
Instead lets provide methods.
Also added a method addMapping($mimetype, $extension) which adds/overwrites mappings.
If we agree on it i ll go ahead and write tests

#17

I dont think we should expose the mapping array

+1, but then I suggest also to have a method to return supported extensions given a MIME type, that would be possible to be done elsewhere with the mapping array exposed, but not otherwise. Done here.

Also added a method addMapping($mimetype, $extension) which adds/overwrites mappings.

Great! It's what I was suggesting in #2388749-39: Register symfony's mime guessers if they are supported.

If we agree on it i ll go ahead and write tests

+1 :)

#2388749: Register symfony's mime guessers if they are supported has been reverted

#21 new patch coming soon.

New patch - combining parts of #8 and parts of #18. Interdiff against #8.

This keeps new methods introduced by @ParisLiakos in #17 and by me in #18, and keeps (but changed to protected) the getMapping method in #8 which is needed to allow the alter call to hook_file_mimetype_mapping_alter(). I'd say it would be probably better to change the signature of the hook to pass the MimeTypeMapper object and use addMapping() in hook implementations instead of letting hooks manipulate the array by reference, as pointed out in #17. But I guess that needs discussion.

Also added some tests for MimeTypeMapper::getExtensionsForMimeType() and MimeTypeMapper::addMapping().

+++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
@@ -0,0 +1,944 @@
+    if (!in_array($mimetype, $this->getMapping()['mimetypes'])) {
...
+    $key = array_search($mimetype, $this->getMapping()['mimetypes']);
...
+    $extensions = $this->getMapping()['extensions'];
+    return isset($extensions[$extension]) ? $this->getMapping()['mimetypes'][$extensions[$extension]] : NULL;
...
+    if (!in_array($mimetype, $this->getMapping()['mimetypes'])) {
...
+    $key = array_search($mimetype, $this->getMapping()['mimetypes']);
+    $extensions = array_keys($this->getMapping()['extensions'], $key, TRUE);

lets only call getMapping() once and store it in a local variable instead of doing unnecessary function calls in the same method

I'd say it would be probably better to change the signature of the hook to pass the MimeTypeMapper object and use addMapping() in hook implementations instead of letting hooks manipulate the array by reference

Good idea!

#24 done

1. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,946 @@
   +    $this->getMapping();
   

   can we add a comment here? why this is needed

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapperInterface.php
   @@ -0,0 +1,57 @@
   +  public function getMimeTypes();
   

   and maybe add a small test case for this method

3. +++ b/core/modules/system/file.api.php
   @@ -102,25 +102,30 @@ function hook_file_url_alter(&$uri) {
   + * Invoked by \Drupal\Core\File\MimeType\MimeTypeMapper::getMapping(). It
   ...
   + * should use the \Drupal\Core\File\MimeType\MimeTypeMapper::addMapping()
   ...
   + * \Drupal\Core\File\MimeType\MimeTypeMapper::getMimeTypeForExtension() and
   + * \Drupal\Core\File\MimeType\MimeTypeMapper::getExtensionsForMimeType()
   ...
   + * @see \Drupal\Core\File\MimeType\MimeTypeMapper::addMapping()
   + * @see \Drupal\Core\File\MimeType\MimeTypeMapper::getMimeTypeForExtension()
   + * @see \Drupal\Core\File\MimeType\MimeTypeMapper::getExtensionsForMimeType()
   

   Maybe those docs should refer to the interface methods ;)

#26 done.

#26.1 - this is needed to make sure that alter hook is invoked before adding any mapping. Maybe I am overcomplicating, but it could be a case that code outside of the hook implementation add a mapping, then the first time any other method is called the hook would fire and may override that mapping.

Also fixed a couple of doc issues.

Nice, thanks..so i guess all we need now is a beta evaluation template and updating https://www.drupal.org/node/2258015 with alter hook change.
other than that, this is rtbc

#28 updated change record draft

https://www.drupal.org/node/2258015/revisions/view/8067869/8077401

can anybody take care of the beta eval?

thank you
I think this is a bug since its a regression

1. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,961 @@
   +    if (!$this->isMappingAltered) {
   ...
   +    $this->getMapping();
   

   There's a weird recursion going on here when the alter hook is fired.

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,961 @@
   +
   +    return TRUE;
   

   This seems pointless - just return $this for a fluent interface.

3. I really like the new way of adding mappings in the alter - much nicer.

4. +++ b/core/modules/system/file.api.php
   @@ -102,25 +102,31 @@ function hook_file_url_alter(&$uri) {
     * Alter MIME type mappings used to determine MIME type from a file extension.
   

   This is not quite correct anymore.

Thanks @alexpott.

#31.1 - different approach here, alter hook is invoked at construction time before any method is called. Not sure it is OK.
#31.2 - done
#31.3 - :)
#31.4 - changed

also changed a small doc issue 'Contains \Drupal...' instead of 'Contains Drupal'

no, we should avoid doing stuff on the constructor other than injecting dependencis, especially invoking a hook.

There's a weird recursion going on here when the alter hook is fired.

which is fine since

$this->isMappingAltered = TRUE;

is called before the hook is called, so the function will just return $this->mapping, it wont do anything else.

I agree that doing the alter in the constructor is wrong. The alter should only be fired when necessary but also the recursive call during an alter is also weird. Plus we've lost the ability to remove mappings.

We are indeed facing recursion here, because if we call e.g. getMimeTypeForExtension, this will call getMapping which, on first run, will invoke the hook. Hook implementations then will call e.g. addMapping who in turn will call again getMapping. So here we need measures to avoid endless loops. I made a change here to make things I hope more explicit, starting back from #27. If any idea how to circumvent this, feel free :)

Next - adding removeMapping and removeMimeType, will post a patch soon.

Added ::removeMapping() and ::removeMimeType() methods.

I found an alternative, i.e. using the 'calls' parameter in service definition to execute the alter hook separately from other methods, but not in the constructor (kind of 'setter injection' according to Symfony docs). Sorry, I'm learning here...

@mondrake that looks like a really nice solution.

#2260061: Responsive image module does not support sizes/picture polyfill 2.2 just introduced the below

+++ b/core/modules/responsive_image/responsive_image.module
@@ -328,17 +445,38 @@ function responsive_image_get_image_dimensions($variables) {
+  // The MIME type guesser needs a full path, not just an extension, but the
+  // file doesn't have to exist.
+  $fake_path = 'responsive_image.' . ImageStyle::load($image_style_name)->getDerivativeExtension($extension);
+  return Drupal::service('file.mime_type.guesser.extension')->guess($fake_path);
+}

which is OKish, but a workaround. With the patch here this could become

  $extension = ImageStyle::load($image_style_name)->getDerivativeExtension($extension);
  return Drupal::service('file.mime_type.mapper')->getMimeTypeForExtension($extension);

is it something to be added to this issue?

Based on @yched's comment in #2426757-5: responsive_image_build_source_attributes() calls mime type guesser over and over, added #39. Also added a strtolower($extension) in MimeTypeMapper::addMapping to keep consistency with other methods.

Rerolled.

Any reviewers?

We were at RTBC back in #30 and @alexpott's feedback in #31 was addressed, see #38 also.

BTW we also have a published change record which is giving this for done, so it would have to be at least set back to draft while this is sorted out.

• Added lazy: true tag to the file.mime_type.mapper service, to match mime type guessing services definition
• fixed a glitch in the responsive_image module
• Fixed and added more dcoumentation to hook_file_mimetype_mapping_alter
• Changed String::format to SafeMarkup::format in MimeTypeTest

Reroll.

Added/updated the proxy classes.

IMO, this needs decision before RC:

• the changes to hook_file_mimetype_mapping_alter() signature (passing a MimeTypeMapperInterface object instead of the internal mapping array) will be hard to get in during 8.0 because of BC
• the published change record give the changes in this issue for done, so if this gets postponed, the CR will have to be reverted in some parts

Rerolled, conficts due to #2534066: Allow selecting the original image when creating a responsive image style, then made the logic in responsive_image_get_mime_type simpler to follow, and added comments.

This is a soft blocker for ImageMagick, see #2612590: Allow configuring in the UI the image formats supported by the toolkit. It would allow the ImageMagick toolkit to be configurable in terms of image formats accepted. Now the supported file extensions are hardcoded in ImagemagickToolkit::getSupportedExtensions() similarly to GD core toolkit, but the difference with ImageMagick is that if you want to add support for an image format you do not need to write specific code for that format as in GD - you just have to have a version of the executable that supports the format you need.

Just a reroll. However this is now no longer acceptable I think, it breaks BC. This issue is still blocking a stable release for the ImageMagick module.

An attempt to move on without BC break. This restores the hook_file_mimetype_mapping_alter to its original signature, still keeping the new service file.mime_type.mapper.

+++ b/core/core.services.yml
@@ -1554,6 +1554,11 @@ services:
+    lazy: true

Are we sure this needs to be lazy? Given this is already injected into a lazy service that always needs it I'm not sure.

@alexpott thanks, patch here removes lazy flag and proxy class.

Whoops..

Let's see this.

Removed no longer needed file docblocks. Anyone up for a review?

Reroll after #2796329: \Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser maps the 'js' extension to the 'application/x-javascript' MIME type instead of 'application/javascript'.

A problem with #79 was that, even if existing API were not changed, still any class that was extending from ExtensionMimeTypeGuesser and trying to access its protected $mapping and $defaultMapping properties would fail, since the mapping is moved to the new class.

Here I am using a magic __get method in ExtensionMimeTypeGuesser, so that any get attempt from ExtensionMimeTypeGuesser::$mapping or ExtensionMimeTypeGuesser::defaultMapping will be redirected to the mapper service, and fetched through two public methods there, introduced for BC and marked @internal.

This issue is pretty much stalled :(

I wonder whether it would make sense to take a different approach in the short/medium term, i.e. just add the methods that would be needed in contrib to the ExtensionMimeTypeGuesser service:

getMapping()
addMapping($mimetype, $extension)
removeMapping($extension)
removeMimeType($mimetype)
getMimeTypes()
getMimeTypeForExtension($extension)
getExtensionsForMimeType($mimetype)

and postpone introducing the separate service MimeTypeMapper to D9.

Any feedback?

Two years later... I would still like this to move on. Getting the list of available MIME types or the list of file extensions mapped to a MIME type is not possible, and workarounds are not good.

In the ImageMagick module, at one point I was extending a class from the guesser, but this was forcing the alter hook to be called again. Lately I am using reflection to get the value of the $mapping array after it has been processed by the alter hook, but again this smells bad.

Relaunching here. This patch (sorry no interdiff):

1. Defines a new file.mime_type.mapper service, implemented through a Drupal\Core\File\MimeType\MimeTypeMapper class (with a supporting Interface), that advocates the execution of the hook_file_mimetype_mapping_alter hook and provides OO methods to retrieve and manipulate the MIME type to extension mapping array.
2. Provides a Kernel test for that.
3. Changes the ExtensionMimeTypeGuesser class to use the new mapper; the ::setMapping method is deprecated, and deprecation tests are added.
4. Changes the signature of the hook_file_mimetype_mapping_alter hook to pass the mapper object in addition to the $mapping array. This for BC, in D9 the $mapping array should be removed. Hook implementations can, as of the release after which the changes are made, either manipulate the array directly or use the OO methods to alter the mapping.

+++ b/core/modules/file/tests/file_test/file_test.module
@@ -291,16 +292,13 @@ function file_test_file_url_alter(&$uri) {
-function file_test_file_mimetype_mapping_alter(&$mapping) {
+function file_test_file_mimetype_mapping_alter(&$mapping, MimeTypeMapperInterface $mime_type_mapper) {

this means no BC, so not sure it is possible in minor versions

OK, I agree it is confusing. Let me do sth else.

Instead of changing the hook signature, this is now deprecating hook_file_mimetype_mapping_alter and replacing it with a hook_mimetype_alter, where the mapper is passed as an argument.

If we agree this is the sustainable way forward, I will add a test for the hook deprecation.

Updated IS.

Added deprecation tests for hook_file_mimetype_mapping_alter.

FTR, if anyone interested, I released today a beta1 version of the Sophron module:

Features

• Enhances Drupal's MIME type detection based on file extension to recognise 1200+ MIME types from 1600+ file extensions (vs Drupal's 360 MIME types and 475 file extensions).
• Provides an extensive MIME type management API through FileEye/MimeMap.
• Optionally replaces Drupal's core MIME type extension-based guesser.

Re-roll of #93.

Seems like these test fails are unrelated so setting back to Needs Review & Triggering bots.

Re-rolled as patch was not applying on core.services.yml
Patch is smaller due to the similarity from ExtensionMimeTypeGuesser with MimeTypeMapper

diff --git a/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
similarity index 86%
copy from core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
copy to core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php

This was a bugsmash triage target. kim.pepper agreed it was a bug. So let's get a reroll here.

One last test failing.

ExtensionMimeTypeGuesserDeprecationTest::testConstructorDeprecation test is failing due to ExtensionMimeTypeGuesser::$mapping or ExtensionMimeTypeGuesser::$defaultMapping are not being redirected to the mapper service.

I am not sure if we should add setters or getters as added in #80 to maintain backwards compatibility.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.