Allow modules to alter the MIME extension mapping rather than setting a huge variable

really needs a test.

tests are an improvement but still needs some dedicated tests for the new and altered functions. we need to:
- call file_mime_extension_mapping() and make sure it returns an array
- call file_get_mimetype() with the default and a made up mapping and check that the right type is returned

i know that these functions are tested indirectly by other tests but we need some tests that specifically target these functions and their edge cases.

i transposed the function names in that last comment.

we should also test loading mappings from variables vs. parameters in file_get_mimetype().

Here's some tests. Also improved the PHPDoc and renamed the variable to match the function name (giving it some namespace protection).

I assume the typical use case for modifying the default mapping is adding or replacing a few entries, not supplying a completely different mapping. Wouldn't it be better to let the setting be applied on top of the default mapping?

I like this last approach.

Only one thing: the static in file_mime_extension_mapping() serves no purpose except hogging memory. It should be removed.

Reroll.

The last patch contained an unrelated change to registry.inc.

Your updates to the patch look sane.

Copy paste of my "crazy" idea which I posted at http://drupal.org/node/370958#comment-1435888

a idea that was mentioned in the hook_variable_info() issue was to create a table to store the mimetype array. I've also seen that there is work for a file.module, it could even provide a interface to add additional mimetypes. Or a contrib module could do it.

Some advantages such a table would imho have:
- Better handling of large amount of entries, does need less memory
- Doesn't need to be cached/saved in the variables table as a big blob
- Possibilites to manage the mime types and create a UI to manage the mimetypes
- We could reference to that table, instead of storing the mimetype for each file entry

@mfb
Oh, I see, I havent really looked at the patch before. A table might be a bit more flexible, but this is easier and don't need a update function and so on. I'm happy as long as we don't have to load the whole array from the cache on each request.

I really want this in, simply because it will cut the hook_variable_info() patch in half :)

I'd suggest adding the explanation in #29 to those tests. Just from looking at the tests, I'ts not obvious why substr() is needed there.

$this->assertEqual(substr($file1->filemime, 0, 5), 'image', t('A MIME type was set.'));

Other than that, it looks good to me.

Ok, another small point.

+    $this->assertResponse(array(200), 'Uploaded file is accessible using public download.');
+    $this->assertEqual(file_get_contents($file), $this->drupalGetContent(), 'Contents of the file are correct.');
+    // Verify file using private download method.
+    $this->drupalGet('system/files/' . $filename);
+    $this->assertResponse(array(200), 'Uploaded file is accessible using private download.');
+    $this->assertEqual(file_get_contents($file), $this->drupalGetContent(), 'Contents of the file are correct.');
+    $this->assertEqual($this->drupalGetHeader('Content-Type'), 'text/plain', t('A MIME type was set.'));

AFAIK, the assert messages at the end should all be translated, but some of these aren't...

Agreed.

Ok, patch looks fine to me, All tests pass and there are tests for the new features. This makes it easier to extend these mimetypes for other modules. Also, this removes a big part of the hook_variable_info() patch (and cache) and therefor simplifies that patch severly.

This is imho RTBC.

I do not get or agree with the array_reverse. You are altering, so alter the array instead of appending.

See also #473652: file_get_mimetype() unneeded loop.

in function file_get_mimetype() you load the mapping and call drupal_alter() on each function invocation?

@mfb - if this is only used at upload time, then I'd agree it's fine.

It is used for every request, for the favicon... http://api.drupal.org/api/function/template_preprocess_page/7

But it might make sense to remove that check and only do it when the favicon is changed.

subscribe

No, no, no. We don't want people to have to manually enter the favicon MIME type. Look at #531476: Add favicon mimetype theme setting first, which would take care of that. It's RTBC as soon as someone can give it a look-over.

Ah, misread the patch. Yeah, that looks okay. Though I think using getimagesize info for favicons would be better anyway. Mine's just a different approach, and we could redo that patch afterwards.

How about you remove the favicon MIME type out of this patch, since it's not really related, and we can give it a proper read, looking at the different options (drewish and i came up with two alternates at the other issue already).

For instance, I'm pretty sure (without testing) that this will end up with no favicon MIME type on maintenance pages, despite best intentions, as there's no access to the db. That's the benefit of using variable_get; we could set that in $conf in settings.php.

This looks really good to me now. You should probably put something in the changelog.txt about it, then I'd say it's RTBC. (Of course, we'll also need to update upgrading dox to note the change from the variable_get.)

awesome work! looks rtbc to me.

I'd love to avoid changing these lines:

-    $this->assertResponse(array(200), 'Uploaded ' . $filename . ' is accessible.');
+    $this->assertResponse(array(200), t('Uploaded %filename is accessible.', array('%filename' => $filename)));
     $this->assertTrue(strpos($this->drupalGetHeader('Content-Type'), 'text/plain') === 0, t('MIME type is text/plain.'));
-    $this->assertEqual(file_get_contents($file), $this->drupalGetContent(), 'Uploaded contents of ' . $filename . ' verified.');
+    $this->assertEqual(file_get_contents($file), $this->drupalGetContent(), t('Uploaded contents of %filename verified.', array('%filename' => $filename)));

since it's not in the scope of the issue and there's still discussion about using t() in assertion messages—translators hate them since they clutter things up and anyone doing core dev work has to be able to read english.

Yeah, I wrote in #331171-37: Allow modules to alter the MIME extension mapping rather than setting a huge variable that they should be translated, but in the meantime, the remove translation from tests issue has landed, so these should be removed again.

i don't think there's anything else holding this up now.

this doesn't make sense to me to have in the stream wrapper:

+      $mapping = file_default_mimetype_mapping();
+      // Allow modules to alter the default mapping.
+      drupal_alter('file_mimetype_mapping', $mapping);

Why would we call drupal_alter on it every time it's used, instead of just the first time?

something like this.

that makes way too much sense. mfb any objections?

As we introduce a new hook_file_mimetype_mapping_alter(), I wonder if we should document that in system.api.php? Otherwise, looks great to me.

This review is powered by Dreditor.

yes it should be added. New patch.

I was being blind, Peter, sorry about that... :P The patch at #81 has that. Thus, #81 is RTBC, and #85 now has a duplicate API description (which would probably break the API module -- should an issue be created to test that API hook_functions aren't duplicated?).

hmm, I wrote more extensive doxygen, so I merged my doxygen with the existing code example for the hook.

tagging

Reviewed this patch and it looks good. Committed to CVS HEAD. Thanks mfb, pwolanin, drewish et al. Good work.

Is there a Drupal 6 version?

Is there a Drupal 6 version?

Was this change announced on the 6/7 module update guide? If it was, or if it didn't need to be, please revert status/tag change.

I couldn't find anything about MIME types on the 6/7 module page, so I added this:
http://drupal.org/update/modules/6/7#mime-types
If someone wants to review, that could be useful -- I based it on the patch in #87, which appears to be the only one that was committed. I think it's OK...

Right. I figured most modules would have done a variable_get(), changed the array, and then done a variable_set() probably, which is kind of a "poor man's alter".