Currently file_get_mimetype() incorrectly reports any file with an uppercase (or mixed case) extension as 'application/octet-stream', since the comparison is only done on all-lowercase extensions. This patch converts the file extension to lowercase before performing the check. Note that there was also a single extension in the mime-type list, "pcf.Z" that included an uppercase character. I converted it to lowercase so it won't be broken by this change.

The use of drupal_strtolower() is not necessary here, because none of the extensions that we support contain any UTF8 or non-latin characters.

CommentFileSizeAuthor
#7 file_mime_case_sensitive_520664_03.patch5.91 KBrfay
#5 file_mime_case_sensitive_520664_02.patch5.91 KBrfay
drupal_file_mime_case_sensitive.patch1.23 KBquicksketch

Comments

quicksketch’s picture

quicksketch
he/him
commented
Issue tags: +D7FileAPIWishlist, +ImageInCore

This problem was made clear by uploading .JPG files for me in the new File module, so I'm tagging with the image tag. It'll be a clear problem if pictures are all being marked as application/octet-stream.

damien tournoud’s picture

damien tournoud commented

Indeed. Do we have tests that would be worth extending for that?

drewish’s picture

drewish commented
Status: Needs review » Needs work

While we're in here we need to fix up some documentation.

file_default_mimetype_mapping() doesn't document it's return value at all and could use a @see to file_get_mimetype(). Additionally it needs a comment where the extensions are set reminding future developers to ensure that they're all lowercase.

file_get_mimetype() also doesn't document that the extensions should be lowercase. I'd like it to mention that if no value is provided that the variable mime_extension_mapping is checked for a value and then file_default_mimetype_mapping() is called. And a @see to file_default_mimetype_mapping() would be good.

As dmaz notes, the tests need to be considered. We have tests in FileMimeTypeTest and testFileMimeTypeDetection() part actually passes in a custom mapping with 'pcf.Z' => 'application/octet-stream', in it but since it's using the default octet-stream it won't be catching our casting to lower case changes... though who ever committed this half baked MIME patch didn't require tests for setting a mapping through the mime_extension_mapping variable.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Assigned: quicksketch » rfay

Per drewish I will roll a new patch.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review
StatusFileSize
new file_mime_case_sensitive_520664_02.patch5.91 KB

OK, I think I covered the issues in #3.

I also added quite a lot of test coverage in the tests by adding sections that test with and without the variable set, and that test when you pass in a mapping.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Needs work

Nice! The extra tests look handy and thorough. Just a few code style fixes:

ID tag (though I think either way works technically):

+// $Id:  $

+// $Id$

No extra newline between @return and @see.

+ * @return
+ *   Array of mimetypes correlated to the extensions that relate to them.
+ *
+ * @see file_get_mimetype()

+ * @return
+ *   Array of mimetypes correlated to the extensions that relate to them.
+ * @see file_get_mimetype()

Sentence-case comments.

+    // extensions added to this list MUST be lower-case.

+    // Extensions added to this list MUST be lower-case.

Close parentheses at same level. 

+      'extensions' => array(
+         'jar' => 0,
+         'jpg' => 1,
+      ));

+      'extensions' => array(
+         'jar' => 0,
+         'jpg' => 1,
+      ),
+    );

End comments in a period.

+    // Now test the same when passing in the map

+    // Now test the same when passing in the map.

Other than that, this looks great!

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review
StatusFileSize
new file_mime_case_sensitive_520664_03.patch5.91 KB

I think this patch adds on the style issues in #6.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Reviewed & tested by the community

Looks fantastic!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -D7FileAPIWishlist, -ImageInCore

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

Issue tags: +D7FileAPIWishlist, +ImageInCore

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.