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.
Comment | File | Size | Author |
---|---|---|---|
#29 | 1921558-psr4-reroll.patch | 12.93 KB | xjm |
#18 | interdiff-mimetypes.txt | 3.44 KB | ParisLiakos |
#18 | drupal-file_get_mimetype_symfony-1921558-18.patch | 12.99 KB | ParisLiakos |
#16 | drupal-file_get_mimetype_symfony-1921558-12.patch | 12.62 KB | ParisLiakos |
#16 | interdiff-mimetypes.txt | 8.19 KB | ParisLiakos |
Comments
Comment #1
Dave ReidNote that file_get_mimetype should probably be a wrapper for registering the MimeTypeGuesser class with the Drupal guesser, and calling the guess() function.
Comment #2
Dave ReidI 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:
Comment #3
Dave ReidI 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.
Comment #4
Dave ReidI 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.
Comment #5
Dave ReidComment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedCould we move the default value inside the guesser? If we can, let's just kill this function altogether.
I cannot figure out if this actually valid or not. Looks weird. I would stick with
->
instead of::
.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.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #8
Dave ReidLast I know I can't override an existing method and change it's parameter declaration.
Since getMimeType is a public static method, can I use -> here?
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedI meant, have
DrupalMappingMimeTypeGuesser
always returnapplication/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 justDrupalDefaultMimeTypeGuesser::getInstance()->guess($uri)
.We also probably should register this in the container, so as to provide pluggability.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedthis should work.
file.mimetypes.inc is dead and file_get_mimetype() deprecated
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedi messed up the inheritdocs :P and forgot to register the mapping property
Comment #12
dawehner#2213319: Create a single Container CompilerPass to collect + add handlers to consumer service definitions should allow to remove all this code.
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 ?
it would be great to fill an upstream patch for this.
Aren't we using \Drupal::service now?
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedThanks for the review!
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)
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
Yes, i wanted to do that, but sleep got me first..will open one and post the link here
Well no, #2087751: [policy, no patch] Stop using $this->container in web tests is not fixed yet and i completely disagree with it
Comment #14
dawehnerDoesn't the following piece of code ensure that the order is as expected ...
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedAh, well it would, if it was calling
asort
and notarsort
;)btw opened for the self/static change:
https://github.com/symfony/symfony/pull/10790
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedseems 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
Comment #17
dawehnerIt 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...
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedI 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
Comment #19
dawehnerOkay, fine.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedComment #21
ParisLiakos CreditAttribution: ParisLiakos commentedComment #22
Dries CreditAttribution: Dries commentedHaven'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.
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commented18: drupal-file_get_mimetype_symfony-1921558-18.patch queued for re-testing.
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedComment #27
ParisLiakos CreditAttribution: ParisLiakos commented18: drupal-file_get_mimetype_symfony-1921558-18.patch queued for re-testing.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedComment #29
xjmReroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.
Comment #30
alexpottCommitted 1797712 and pushed to 8.x. Thanks!
Comment #33
ArlaMe 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.
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedyes, we can split the list and the alter hook in another class/service, in another issue
Comment #35
ArlaCreated new issue: #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser
Comment #36
Gábor HojtsyFix media tag.