[ticket/11912] Add mimetype guesser for proper mimetype guessing #1813
Conversation
Mimetype guesser will be used as front-end file for mimetype guessing. PHPBB3-11912
The content_guesser will try to use the function mime_content_type() if it's available. If that is not the case, the content_guesser will try to guess the mimetype using the file extension of the supplied file. Since this guesser will be registered after the other guessers, it will be only used if the other guessers are not available. PHPBB3-11912
PHPBB3-11912
PHPBB3-11912
…dows The filename of the files sent to the guesser by plupload do not contain the file extension. Therefore, it's impossible to guess the mimetype if only the content_guesser is available and the function mime_content_type() doesn't exist. By supplying the filename we can circumvent this issue. PHPBB3-11912
This will contain proper documentation of the required methods if anyone would implement a newer guesser. PHPBB3-11912
| @@ -0,0 +1,27 @@ | |||
| services: | |||
| mimetype.fileinfo_mimetype_guesser: | |||
There was a problem hiding this comment.
should we name them mimetype.symfony.* so its clear what their vendor is?
There was a problem hiding this comment.
don't think that matters
| $file_name = (empty($file_name)) ? $file : $file_name; | ||
| $mimetype = $this->map_extension_to_type($file_name); | ||
| } | ||
| return $mimetype; |
There was a problem hiding this comment.
These two different methods should go into two separate guessers.
|
For this, the order of the items added to the service collection is extremely important (meaning we have to be sure the order is going to be identical on ALL installations) and it's effectively not possible for an extension to add a mimetype guesser because (I believe) it will be added after our baseline guesser, which will always produce some result and thus theirs would never run. Because of these two issues, I would actually prefer that the current guessers be hardcoded in the file in an array (so we're 100% sure of the order) and an event be created allowing the array to be edited before the guessers are registered. |
|
Or rather, to avoid having to use events here, assign a priority to guessers and sort them by that. |
| @@ -128,7 +135,7 @@ public function handle_upload($form_name) | |||
| 'tmp_name' => $file_path, | |||
| 'name' => $this->request->variable('real_filename', ''), | |||
| 'size' => filesize($file_path), | |||
| 'type' => $file_info->getMimeType($file_path), | |||
There was a problem hiding this comment.
$file_info is no longer used?
There was a problem hiding this comment.
Yeah, should probably be removed if it's no longer used.
|
@naderman @EXreaction should that priority be assigned via the service definition or what did you have in mind? |
|
Having a priority setting would be best if you see a nice way of doing so. |
The content_guesser now only guesses the mimetype with the function mime_content_type() while the guessing by extension is done using the extension_guesser. PHPBB3-11912
The mimetype guesser priority can now be set through the service definition. Mimetypes will be guessed from the guesser with the highest priority to the one with the lowest priority. Standard priority types have been added to the service definition file. Any integer value can be used though. Standard mimetype guessers that do not have the methods get_priority and set_priority implemented, like the standard MimeTypeGuessers of symfony, will have the default priority with the value of 0. Lower priority guessers have values lower than 0 while high priority ones can be added with values higher than 0. PHPBB3-11912
| */ | ||
| public function test_content_guesser($expected, $guessers, $overload = false) | ||
| { | ||
| self::$function_exists = ($overload) ? false : true; |
There was a problem hiding this comment.
self::$function_exists = !$overload;
?
…tests PHPBB3-11912
| $mimetype = null; | ||
| if (function_exists('mime_content_type')) | ||
| { | ||
| $mimetype = mime_content_type($file); |
There was a problem hiding this comment.
return mime_content_type($file);
}
return null; // optional
| return mime_content_type($file); | ||
| } | ||
|
|
||
| return null; // optional |
There was a problem hiding this comment.
The comment is wrong, what EXreaction ment is, that this line is optional.
php returns null if there was no return sent.
so just remove it
| */ | ||
| public function is_supported() | ||
| { | ||
| return true; |
There was a problem hiding this comment.
Actually, this should return function_exists('mime_content_type'), shouldn't it?
There was a problem hiding this comment.
Now that the extension_guesser has been split off, it should do that.
|
This all looks great to me; please fix conflicts and I think we can merge this. |
…ists PHPBB3-11912
…/11912 Conflicts: phpBB/config/services.yml
[ticket/11912] Add mimetype guesser for proper mimetype guessing
This will first of prevent us hitting the LogicException that we would hit by just using the Symfony MimeTypeGuesser as described in the initial ticket:
http://tracker.phpbb.com/browse/PHPBB3-11912
Additionally, we'll now have a last fallback if no mimetype could be guessed with any of the available MimeTypeGuessers or even the function mime_content_type(): The content_guesser will fall back to mapping the file extensions of the supplied file to a mimetype.