Add favicon mimetype theme setting

subscribing

So now instead of loading the extension mapping on every page, we add a cache_get() on every page instead. I'm not sure that's too much of an improvement and prefer the variable version in #8 which looks more or less RTBC to me.

Also some more history on this one, doesn't affect the current patch but really, really glad to see this get fixed properly here since we didn't come up with this idea in #473652: file_get_mimetype() unneeded loop. If we get this in, we could possibly revert back to the single file extension mapping array which was replaced in that issue for similar reasons.

I think #8 will have to be resubmitted for the test bot to start liking it. And yes we can probably re-open #473652: file_get_mimetype() unneeded loop once this is in (or a new issue to tidy up differently if we don't want to do a straight revert there).

For posterity (and in case any of it is useful) here's the original version of this from #331171: Allow modules to alter the MIME extension mapping rather than setting a huge variable. I appreciate its simplicity. We set a sane default for the favicon_mimetype. And we add a custom favicon_mimetype setting to the settings array (if default icon is not used) when the theme settings form is submitted.

"image/vnd.microsoft.icon" is the type used in d7 currently, as determined by file_get_mimetype(). It's also officially correct according to http://en.wikipedia.org/wiki/ICO_%28file_format%29

Unfortunately getimagesize() didn't get icon support until PHP 5.3 according to http://www.php.net/function.getimagesize so I don't think it's an option.

BTW, random trivia from wikipedia article, "Windows Vista adds support for natively displaying 256×256 pixel icon images, and supports (but does not require) the compressed PNG format... It is recommended that all 256×256 icons should be stored in the ICO file in PNG format to reduce the overall size of the file." So what we'll commonly see is PNG-encoded icons, with "ico" extension and "image/vnd.microsoft.icon" as the mimetype even if the file is PNG not ICO format. (Firefox and Webkit already supported PNG-encoded icons for a long time).

aaron, the changelog lines are whitespace changes. your editor is removing trailing whitespace. when you generate the patch you can just explicitly name the files to diff if you want to ignore that or use one of the diff options to ignore whitespace.

personally i'd love to avoid using file_get_mimetype() on images because unlike getimagesize() it's it just looks at the extension rather than the contents of the file.

Not clear we even need a changelog entry for a new setting that developers shouldn't have to worry about. But if you can clean up the patch... :)

If someone is maintaining a list of things to redo when drupal moves to PHP >= 5.3, using getimagesize() here could go on that list. Meanwhile looking at the file extension isn't that bad, it's what apache is doing anyways :)

Patch at #16 still applies.

I'm not sure about removing file.inc from the maintenance theme. Seems like this could potentially break calls to file_*() functions during install or update? At least, that is my reading of http://api.drupal.org/api/function/_drupal_maintenance_theme/7

All looks good to me, can we maybe add some documentation as to why 'image/vnd.microsoft.icon' is the default?

Added comment about image/vnd.microsoft.icon

Can't find any other objections, great patch!

Let's have another look at #473652: file_get_mimetype() unneeded loop once this lands.

Committed to HEAD. Thanks!

Don't we have a little bit of a regression of #415710: Favicon.ico defaults to 'type="image/x-icon"' here?

If the file has a .ico extension, file_get_mimetype() correctly determines the mime type to be “image/vnd.microsoft.icon”. But if file_get_mimetype() can't determine the mime type and just returns application/octet-stream, we really should be using the generic MIME type for favicons, “image/x-icon”.

This new patch… bah! I just realized its insufficient. Themes can set the default favicon to have whatever file extension they want, .gif, .png, etc. And the code committed for this patch never checks the file extension of the default favicon; it just assumes its .ico. So the mime type will be set incorrectly for those themes using a non-.ico file extension.

Ok. I'm doing too much at the same time. Namely I'm trying to pack for an international flight. And now I can't even remember/don't have time to check if theme's can set a default favicon that isn't named favicon.ico. :-p

So the patch in #32 needs review.

So what makes image/x-icon a good default if it's not even a valid registered MIME type? Is there some spec or browser issue in play here?

Hmm… I stand corrected. The favicon “spec” used image/x-icon not because its the default mime type for an unknown favicon type, but because .ico files specifically didn't have a mime type before 2003. http://en.wikipedia.org/wiki/ICO_(icon_image_file_format)

But actually, that means we shouldn't be doing “Use the default MIME type for favicons if no other was found.” Instead, we really should set the mime type to be "application/octet-stream" if we come across an unknown image type. Right?

New patch just removes the check for "application/octet-stream".

JohnAlbin is correct, and the behavior in the proposed patch is technically correct. Committed to CVS HEAD.

Sorry about that! :\