Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Add mime type guessing by file extension#21985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Hanmac commentedMar 13, 2017
i am okay with adding such a "extension" to "mime-type" guesser, |
src/Symfony/Component/HttpFoundation/File/MimeType/MimeTypeByExtensionGuesser.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| * | ||
| * @var array | ||
| */ | ||
| protected $defaultExtensions = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
private
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function guess($extension) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is wrong. The interface states thatguess() must expect the file path and not only its extension.
xabbuh commentedMar 14, 2017
What about registering this also as an alternative in |
xabbuh commentedMar 14, 2017
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'thave access to a file. So, for guessing the mime type this does it justby 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
ipf commentedMar 14, 2017
Hi@xabbuh, thanks a lot for your reviews. Rebuilding the |
xabbuh commentedMar 14, 2017
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could be a bit shorter like this? Imho no setup method or member variable needed here?
$result = (newMimeTypeByExtensionGuesser())->guess($extension);
| /** | ||
| * @dataProvider extensionDataProvider | ||
| * | ||
| * @param $extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
useless without any type info
nicolas-grekas commentedSep 26, 2017
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. |
nicolas-grekas commentedOct 8, 2017
Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw. |
fabpot commentedMar 31, 2018
The list of extensions is already part of the |
teohhanhui commentedApr 10, 2018
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. |
fabpot commentedJan 16, 2019
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 <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#28832#21985 makes#15460 trivial| License | MIT| Doc PR |symfony/symfony-docs#10886This has been on my todo-list for X years :)Commits-------bdca5d9 tweaked code5268389 [Mime] added freedesktop as a source for mime types74ca91d [Mime] added the componentd7ee0ec [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).