Problem/Motivation

With #1921558: Convert file_get_mimetype() to use Symfony MimeTypeGuessers, the function file_mimetype_mapping() was removed. The mapping was placed as a protected property of the ExtensionMimeTypeGuesser class, as its main purpose was for the guesser to use it. But there are cases when the mapping, or just the list of MIME types, should be used in itself, outside the context of guessing. For instance, a form in the File entity module should present known MIME types for the user.

Proposed resolution

Move the default mapping to a new service and class, Drupal\Core\File\MimeType\MimeTypeMapper.

Introduce an alterMapping() method which invokes the file_mimetype_mapping alter hook upon service instantiation to allow modules to play with MIME type<->extension mapping.

Introduce getter/setter methods.

Add tests.

Remaining tasks

Commit

User interface changes

None

API changes

A new service file.mime_type.mapper.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because functionality in previous version is now missing
Issue priority Major because it is a small regression. In D7 one could access the mapping out of the context of guessing, and could eg get a list of all known mimetypes . After #1921558: Convert file_get_mimetype() to use Symfony MimeTypeGuessers its impossible without extending the class, because the mapping is a protected property
Disruption It changes hook_file_mimetype_mapping_alter's signature, but this hook is not used much (i saw somewhere that only one contrib module used it in d7) No changes at interface level, but implementations extending from ExtensionMimeTypeGuesser that rely on the protected $defaultMapping property will need to be revised.

None, a BC layer covers the case of custom/contrib classes extending from ExtensionMimeTypeGuesser.

Files: 

Comments

Arla’s picture

Issue summary: View changes
Arla’s picture

Assigned: Unassigned » Arla
Status: Active » Needs review
FileSize
54.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,988 pass(es). View

This patch moves the mapping from ExtensionMimeTypeGuesser to the new MimeTypeMapper, which is a service, and lets ExtensionMimeTypeGuesser use that mapper.

ParisLiakos’s picture

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

Arla’s picture

FileSize
55.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,303 pass(es). View
5.11 KB

Adjusted from the feedback in #3.

Berdir’s picture

  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.

Arla’s picture

2. I'm not sure why setMapping() would be needed, but it is tested in MimeTypeTest. I guess testing the alter hook would be more appropriate, and remove setMapping()?

ParisLiakos’s picture

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

Arla’s picture

FileSize
57.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch separate_mime_type-2311679-8.patch. Unable to apply patch. See the log in the details link for more information. View
4.63 KB

Here the Mapper has also getMimeTypes() and getMimeTypeForExtension(). The test for custom mapping is removed. Is it desirable to add a test for the alter hook?

The alter hook is only run on the first call to getMapping(). Unless the guesser is used before registering all hook implementations, this should not be a problem.

Status: Needs review » Needs work

The last submitted patch, 8: separate_mime_type-2311679-8.patch, failed testing.

alexpott’s picture

mondrake’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
53.94 KB
53.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,857 pass(es), 14 fail(s), and 0 exception(s). View
2.58 KB

#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.

Dave Reid’s picture

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.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 12: separate_mime_type-2311679-12.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
882 bytes
53.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,934 pass(es). View

Fixed PHPUnit test failure.

ParisLiakos’s picture

FileSize
53.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,900 pass(es). View
4.21 KB

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

mondrake’s picture

Related issues: +#2388749: Register symfony's mime guessers if they are supported
FileSize
1.53 KB
54.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch separate_mime_type-2311679-18.patch. Unable to apply patch. See the log in the details link for more information. View

#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 :)

Status: Needs review » Needs work

The last submitted patch, 18: separate_mime_type-2311679-18.patch, failed testing.

alexpott’s picture

mondrake’s picture

#21 new patch coming soon.

mondrake’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
59.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,939 pass(es). View

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().

ParisLiakos’s picture

+++ 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!

mondrake’s picture

FileSize
8.51 KB
64.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,938 pass(es). View

#24 done

ParisLiakos’s picture

  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 ;)

mondrake’s picture

FileSize
6.05 KB
65.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,589 pass(es). View

#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.

ParisLiakos’s picture

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

mondrake’s picture

#28 updated change record draft

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

can anybody take care of the beta eval?

ParisLiakos’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.12 KB
64.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,628 pass(es). View

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'

ParisLiakos’s picture

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.

alexpott’s picture

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.

mondrake’s picture

FileSize
65.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,727 pass(es). View
2.51 KB

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.

mondrake’s picture

FileSize
4.43 KB
67.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,751 pass(es). View

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

mondrake’s picture

FileSize
4.9 KB
67.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,053 pass(es). View

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...

alexpott’s picture

@mondrake that looks like a really nice solution.

mondrake’s picture

#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?

mondrake’s picture

Related issues: +#2426757: responsive_image_build_source_attributes() calls mime type guesser over and over
FileSize
1.48 KB
68.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch separate_mime_type-2311679-40.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 40: separate_mime_type-2311679-40.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
68.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,790 pass(es). View

Rerolled.

mondrake’s picture

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.

mondrake’s picture

Issue summary: View changes
FileSize
6.35 KB
69.73 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch separate_mime_type-2311679-47.patch. Unable to apply patch. See the log in the details link for more information. View
  • 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
slashrsm’s picture

Issue tags: +sprint
Dave Reid’s picture

Issue tags: -sprint +D8Media

Status: Needs review » Needs work

The last submitted patch, 47: separate_mime_type-2311679-47.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
69.66 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 97,709 pass(es), 8 fail(s), and 33,185 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 52: separate_mime_type-2311679-52.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.59 KB
74.24 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,567 pass(es). View

Added/updated the proxy classes.

Arla’s picture

Assigned: Arla » Unassigned
mondrake’s picture

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

Status: Needs review » Needs work

The last submitted patch, 54: separate_mime_type-2311679-54.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
75.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,825 pass(es). View

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.

mondrake’s picture

mondrake’s picture

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.

mondrake’s picture

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

Status: Needs review » Needs work

The last submitted patch, 59: separate_mime_type-2311679-59.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Contributed project soft blocker +Contributed project blocker
FileSize
75.03 KB

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.

mondrake’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 65: 2311679-65.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
66.97 KB

Status: Needs review » Needs work

The last submitted patch, 67: 2311679-67.patch, failed testing.

alexpott’s picture

+++ 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.

mondrake’s picture

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

mondrake’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: 2311679-70.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -rc deadline
FileSize
62.97 KB
516 bytes

Whoops..

Status: Needs review » Needs work

The last submitted patch, 73: 2311679-73.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.35 KB
62.85 KB

Let's see this.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

FileSize
873 bytes
62.69 KB

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

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.

mondrake’s picture

FileSize
64.88 KB
4.2 KB

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.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

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?

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.