Problem/Motivation

We want an OO alternative for file_get_mimetype()

Proposed resolution

Use MIME type guessers from Symfony's HttpFoundation! Its already there and way more flexible:)

Remaining tasks

Commit

User interface changes

None

API changes

file_get_mimetype() - deprecated, to be removed
file_mimetype_mapping() - removed
file_default_mimetype_mapping() - removed
Drupal/Core/StreamWrapper/StreamWrapperInterface::getMimeType() - removed
New service file.mimetype.guesser which should be called instead of file_get_mimetypes() now

Original report by @Dave Reid

Symfony provides a wonderful and aptly-named 'MIME type guesser' class of MimeTypeGuesser. We should integrate our logic into a instance of MimeTypeGuesserInterface and use MimeTypeGuesser to get the mime type of a file.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Note that file_get_mimetype should probably be a wrapper for registering the MimeTypeGuesser class with the Drupal guesser, and calling the guess() function.

Dave Reid’s picture

Status: Active » Needs review
FileSize
9.49 KB

I think this is how it should look. Guessing that I should be defining the default mimetype mapper the proper way, but I just wanted to see how this would come together.

Summary:

  • Adds a new DrupalMappingMimeTypeGuesser that implements MimeTypeGuesserInterface that takes either a custom extension to mimetype mapping array (or default from file_mimetype_mapping) to try and guess the mime type of a file.
  • Adds a new StreamWrapperMimeTypeGuesser that returns the result of the file's stream wrapper's getMimeType() function if it has one. This makes the getMimeType() method optional for stream wrappers since you only have to implement that method if you need custom logic for your stream wrapper, which I think makes sense.
  • Adds a new DrupalDefaultMimeTypeGuesser that extends Symfony's MimeTypeGuesser with the two Drupal guessers. It runs in the following order: mapping, stream wrapper, then Symfony defaults.
  • Converts file_get_mimetype() to use DrupalMappingMimeTypeGuesser by default, or a MimeTypeGuesserInterface object passed in via a parameter. This makes the function more flexible for 'custom' mimetype guessing since you can pass in whatever guesser object you want. An example of a custom mapping guesser is shown in the tests:
        $mapping = array(
          'mimetypes' => array(
            0 => 'application/java-archive',
            1 => 'image/jpeg',
          ),
          'extensions' => array(
             'jar' => 0,
             'jpg' => 1,
          )
        );
    
    //BEFORE:
        $mimetype = file_get_mimetype($uri, $mapping);
    
    //AFTER:
        $guesser = new DrupalMappingMimeTypeGuesser($mapping);
        $mimetype = file_get_mimetype($uri, $guesser);
    
    
Dave Reid’s picture

I noticed that Guzzle also has a default mimetype functionality in http://api.drupal.org/api/drupal/core%21vendor%21guzzle%21http%21Guzzle%.... I wonder if we can integrate these two somehow since it would be nice if it re-used the Symfony mappings. Likely not.

Dave Reid’s picture

Issue tags: +API change

I double checked all highly visible usages of file_get_mimetype() and the only call that actually uses $mapping is the tests. So I feel this is a safe API change to make for D8.

Dave Reid’s picture

Title: Use MimeTypeGuesser instead of file_get_mimetype() » Convert file_get_mimetype() and LocalStream::getMimeType to use Symfony MimeTypeGuessers
Damien Tournoud’s picture

Title: Convert file_get_mimetype() and LocalStream::getMimeType to use Symfony MimeTypeGuessers » Use MimeTypeGuesser instead of file_get_mimetype()
+function file_get_mimetype($uri, MimeTypeGuesserInterface $guesser = NULL, $default = FILE_DEFAULT_MIMETYPE) {
+  if (!isset($guesser)) {
+    $guesser = DrupalDefaultMimeTypeGuesser::getInstance();
   }
+  $result = $guesser->guess($uri);
+  return !empty($result) ? $result : $default;
 }

Could we move the default value inside the guesser? If we can, let's just kill this function altogether.

+      if (method_exists($wrapper, 'getMimeType')) {
+        return $wrapper::getMimeType($path);
+      }

I cannot figure out if this actually valid or not. Looks weird. I would stick with -> instead of ::.

   /**
    * Implements Drupal\Core\StreamWrapper\StreamWrapperInterface::getMimeType().
+   *
+   * Code moved to Drupal\Core\File\MimeType\DrupalMappingMimeTypeGuesser.
    */

Let's kill the comment. Also, at this point it would make sense to split the StreamWrapperInterface so that you don't have to implement an empty method.

Damien Tournoud’s picture

Title: Use MimeTypeGuesser instead of file_get_mimetype() » Convert file_get_mimetype() and LocalStream::getMimeType to use Symfony MimeTypeGuessers
Dave Reid’s picture

Status: Needs review » Needs work

Could we move the default value inside the guesser? If we can, let's just kill this function altogether.

Last I know I can't override an existing method and change it's parameter declaration.

I cannot figure out if this actually valid or not. Looks weird. I would stick with -> instead of ::.

Since getMimeType is a public static method, can I use -> here?

Damien Tournoud’s picture

Last I know I can't override an existing method and change it's parameter declaration.

I meant, have DrupalMappingMimeTypeGuesser always return application/octet-stream as a last resort, and remove the (now useless) file_get_mimetype(). I don't see the point of the wrapper if it is just DrupalDefaultMimeTypeGuesser::getInstance()->guess($uri).

We also probably should register this in the container, so as to provide pluggability.

ParisLiakos’s picture

Assigned: Dave Reid » ParisLiakos
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Kill includes, +symfony
FileSize
14.72 KB

this should work.
file.mimetypes.inc is dead and file_get_mimetype() deprecated

ParisLiakos’s picture

Title: Convert file_get_mimetype() and LocalStream::getMimeType to use Symfony MimeTypeGuessers » Convert file_get_mimetype() to use Symfony MimeTypeGuessers
FileSize
14.85 KB
1.85 KB

i messed up the inheritdocs :P and forgot to register the mapping property

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterMimeTypeGuessersPass.php
    @@ -0,0 +1,61 @@
    +/**
    + * Adds guessers to the file.mime_type.guesser service.
    + */
    +class RegisterMimeTypeGuessersPass implements CompilerPassInterface {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function process(ContainerBuilder $container) {
    +    if (!$container->hasDefinition('file.mime_type.guesser')) {
    +      return;
    +    }
    +    $guesser = $container->getDefinition('file.mime_type.guesser');
    +    $definitions = array();
    +    // \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser::register()
    +    // does not support priorities, so we need to sort them here, before adding
    +    // the method calls.
    +    foreach ($container->findTaggedServiceIds('mime_type_guesser') as $id => $attributes) {
    +      $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
    +      $definitions[$priority][] = $id;
    +    }
    +    foreach ($this->sortDefinitions($definitions) as $id) {
    +      $guesser->addMethodCall('register', array(new Reference($id)));
    +    }
    +  }
    +
    +  /**
    +   * Sorts definitions according to priority.
    +   *
    +   * @param array $definitions
    +   *   An array of arrays of definition ids, keyed by priority.
    +   *
    +   * @return array
    +   *   A sorted array of definition ids.
    +   */
    +  protected function sortDefinitions(array $definitions) {
    +    $sorted = array();
    +    // The last call to register() will fire first so definitions with higher
    +    // priority should be registered last.
    +    ksort($definitions);
    +
    +    foreach ($definitions as $ids) {
    +      $sorted = array_merge($sorted, $ids);
    +    }
    +    return $sorted;
    +  }
    +
    +}
    

    #2213319: Create a single Container CompilerPass to collect + add handlers to consumer service definitions should allow to remove all this code.

  2. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
    @@ -2,39 +2,26 @@
    +class ExtensionMimeTypeGuesser implements MimeTypeGuesserInterface {
    

    Did you had a look how many of the mimetypes in symfony differ from the one in symfony? Could we just reuse \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser ?

  3. +++ b/core/lib/Drupal/Core/File/MimeType/StreamWrapperMimeTypeGuesser.php
    @@ -0,0 +1,59 @@
    +   */
    +  public static function getInstance() {
    +    if (static::$instance === NULL) {
    +        static::$instance = new static();
    +    }
    +
    +    return static::$instance;
    +  }
    +
    

    it would be great to fill an upstream patch for this.

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/File/MimeTypeTest.php
    @@ -50,14 +52,15 @@ public function testFileMimeTypeDetection() {
    +    $guesser = $this->container->get('file.mime_type.guesser');
    
    @@ -88,9 +91,10 @@ public function testFileMimeTypeDetection() {
    +    $guesser = new ExtensionMimeTypeGuesser($this->container->get('module_handler'), $mapping);
    

    Aren't we using \Drupal::service now?

ParisLiakos’s picture

Thanks for the review!

#2213319: Create a single Container CompilerPass to collect + add handlers to consumer service definitions should allow to remove all this code.

Yes unfortunately the priority here is not sorted as we do in other places. --> the priority have to be sorted in the compiler pass because of the symfony mime guesser, not supporting priorities on hist register() method.
You think we should also override the register method instead of do the sorting in the compiler pass though? I would be open to that.
(see also my comment)

// \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser::register()
// does not support priorities, so we need to sort them here, before adding
// the method calls.
Did you had a look how many of the mimetypes in symfony differ from the one in symfony? Could we just reuse \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser ?

Yes. Unfortunately this class is an extension guesser, not a mime type guesser ;)
This got me at first as well. eg you pass him a mimetype and it gives you the extension.. while we need the opposite; pass the extension and give us the mime type

it would be great to fill an upstream patch for this.

Yes, i wanted to do that, but sleep got me first..will open one and post the link here

Aren't we using \Drupal::service now?

Well no, #2087751: [policy, no patch] Stop using $this->container in web tests is not fixed yet and i completely disagree with it

dawehner’s picture

Doesn't the following piece of code ensure that the order is as expected ...

TaggedHandlerPass::119

        // Sort all handlers by priority.
        arsort($handlers, SORT_NUMERIC);
ParisLiakos’s picture

Doesn't the following piece of code ensure that the order is as expected

Ah, well it would, if it was calling asort and not arsort ;)

// The last call to register() will fire first so definitions with higher
// priority should be registered last.

btw opened for the self/static change:
https://github.com/symfony/symfony/pull/10790

ParisLiakos’s picture

seems that extending Symfony's mime type guesser we are making everyone's life harder..this class is really not reusable at all.
so lets follow what we do in rest of the core and as bonus get rid the compiler

dawehner’s picture

+++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
@@ -876,4 +863,64 @@ function file_default_mimetype_mapping() {
+  public function __construct(ModuleHandlerInterface $module_handler, array $mapping = array()) {
...
+    if (empty($this->mapping)) {
+      $mapping = $this->defaultMapping;
+      // Allow modules to alter the default mapping.
+      $this->moduleHandler->alter('file_mimetype_mapping', $mapping);
+      $this->mapping = $mapping;
+    }

It feels that a setter would be the beter way, tbh. It is really confusing that you don't execute the alter hook, in case you pass in some other mapping...

ParisLiakos’s picture

It is really confusing that you don't execute the alter hook, in case you pass in some other mapping

I agree..but thats how it is like in HEAD right now..i would rather change this behavior in a followup.

Very nice idea with the setter..it definitely feels more natural, thanks!

Also fixed some docs

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Okay, fine.

ParisLiakos’s picture

Issue summary: View changes
ParisLiakos’s picture

Issue summary: View changes
Dries’s picture

Haven't been able to review this in depth yet but it seems to make sense given that we ship with MimeTypeGuesserInterface. We may as well use it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: drupal-file_get_mimetype_symfony-1921558-18.patch, failed testing.

ParisLiakos’s picture

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: drupal-file_get_mimetype_symfony-1921558-18.patch, failed testing.

ParisLiakos’s picture

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community
xjm’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1797712 and pushed to 8.x. Thanks!

diff --git a/core/includes/file.inc b/core/includes/file.inc
index d44d96d..18c2820 100644
--- a/core/includes/file.inc
+++ b/core/includes/file.inc
@@ -6,7 +6,6 @@
  */
 
 use Drupal\Component\Utility\UrlHelper;
-use Drupal\Core\StreamWrapper\LocalStream;
 use Drupal\Component\PhpStorage\FileStorage;
 use Drupal\Component\Utility\Bytes;
 use Drupal\Component\Utility\String;
diff --git a/core/modules/system/src/Tests/File/MimeTypeTest.php b/core/modules/system/src/Tests/File/MimeTypeTest.php
index 06ef0fb..85d7e7f 100644
--- a/core/modules/system/src/Tests/File/MimeTypeTest.php
+++ b/core/modules/system/src/Tests/File/MimeTypeTest.php
@@ -7,8 +7,6 @@
 
 namespace Drupal\system\Tests\File;
 
-use Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser;
-
 /**
  * Tests for file_get_mimetype().
  */

  • Commit 1797712 on 8.x by alexpott:
    Issue #1921558 by ParisLiakos, xjm, Dave Reid: Convert file_get_mimetype...

Status: Fixed » Closed (fixed)

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

Arla’s picture

Status: Closed (fixed) » Active

Me and http://drupal.org/u/ChrisjuhB found a use case where file_mimetype_mapping() is needed. We suggest that its functionality be re-added somewhere appropriate.

We are working on the File entity module. There is a form for adding file types (e.g. audio, document) and we need to provide a list of all MIME types to associate to the file type.

Currently we're using a workaround by extending ExtensionMimeTypeGuesser with a public method to run the file_mimetype_mapping alter hooks and return the list. Obviously it does not make sense semantically to use a guesser for getting the whole list.

Opening this issue temporarily to get feedback. If it is agreed to change the behaviour as suggested, this should be closed and a new issue should be opened for that.

ParisLiakos’s picture

Status: Active » Closed (fixed)

yes, we can split the list and the alter hook in another class/service, in another issue

Arla’s picture

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.