[HttpFoundation] Add mime type guessing by file extension #21985
Conversation
|
i am okay with adding such a "extension" to "mime-type" guesser, |
src/Symfony/Component/HttpFoundation/File/MimeType/MimeTypeByExtensionGuesser.php
Show resolved
Hide resolved
| * | ||
| * @var array | ||
| */ | ||
| protected $defaultExtensions = array( |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function guess($extension) |
There was a problem hiding this comment.
This is wrong. The interface states that guess() must expect the file path and not only its extension.
|
What about registering this also as an alternative in |
|
Alternatively, instead of inlining the map of extensions to mime types inside the class, we could include the original resources file and allow to pass the file to be used in the constructor (defaulting to the file that is shipped with the HttpFoundation component). |
This adds mime type guessing by file extension. Sometimes file information is only stored as metadata, so we don't have access to a file. So, for guessing the mime type this does it just by using a string (i.e. 'jpg). | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | no | License | MIT
|
Hi @xabbuh, thanks a lot for your reviews. Rebuilding the |
What do you mean with "rebuilding all that"?
This should indeed not happen inside the constructor to avoid overhead for requests that don't make use of the mime type guesser or for cases where one of the other guesser returned a result, but only when the extension based guesser actually does have to perform some work. I don't expect the parsing to be too heavy in these cases. But you may prove me wrong and we should then indeed think about some caching. |
| */ | ||
| public function guess($extension) | ||
| { | ||
| return isset($this->defaultExtensions[$extension]) ? $this->defaultExtensions[$extension] : null; |
There was a problem hiding this comment.
I don't know if it worth it, but PNG, JPEG, etc are valid extensions, what about lowercase the extension before use it?
| */ | ||
| public function testMimeTypeGuessing($extension, $mimeType) | ||
| { | ||
| $result = $this->guesser->guess($extension); |
There was a problem hiding this comment.
could be a bit shorter like this? Imho no setup method or member variable needed here?
$result = (new MimeTypeByExtensionGuesser())->guess($extension);| /** | ||
| * @dataProvider extensionDataProvider | ||
| * | ||
| * @param $extension |
There was a problem hiding this comment.
useless without any type info
|
I agree with @xabbuh about the list: we dont want to maintain it, so we definitely should build it from a reference source. This could happen with a cache warmer I guess. |
|
Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw. |
|
The list of extensions is already part of the |
|
This may not be the right place to raise the issue, but the DX around MIME type guessing is kind of broken. See #15460 (comment) for example. |
|
Closing in favor of #29896 |
This PR was squashed before being merged into the 4.3-dev branch (closes #29896). Discussion ---------- [Mime] Add the component | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #28832 #21985 makes #15460 trivial | License | MIT | Doc PR | symfony/symfony-docs#10886 This has been on my todo-list for X years :) Commits ------- bdca5d9 tweaked code 5268389 [Mime] added freedesktop as a source for mime types 74ca91d [Mime] added the component d7ee0ec [HttpFoundation] updated File code
This adds mime type guessing by file extension.
Sometimes file information is only stored as metadata, so we don't have access
to a file. So, for guessing the mime type this does it just by using a string (i.e. 'jpg).