Favicon.ico defaults to 'type="image/x-icon"'

I suggest that you use file_get_mimetype().

Also, there should be no space following the opening parenthesis in an if statement.

Coding style issues in the patch.

I do not why are you swearing the bot when there was a typo in the patch which it correctly catched. Anyways, now you call theme_get_setting('favicon') twice which calls for a variable.

Huh. It's just simpler to store the call of a function into a variable than call it twice. That's all.

Based on http://www.iana.org/assignments/media-types/image/, I see that the "ico" extension should be mapped to "vnd.microsoft.icon". New patch fixes that as well.

Also, I agree with chx. So I added a $favicon variable. This saves the overhead of calling a function twice. Creating a variable is not the same as creating a cache.

Lastly, $icontype doesn't follow the standards of our variable naming conventions. We like underscores so that they are more readable for non-native-English speakers.

Whoops! Forgot about template_preprocess_maintenance_page().

Although I don't like to have the word "microsoft" in the Drupal source code, the patch from #19 applies fine ;)

Everything seems to work as expected. +1 for this.

Yay! Dixon, patch-reviewer extraordinaire!

- We should escape $favicon_type because it is not guaranteed to be safe.

- Let's rename $favicon_type to $type.

- Let's document why we check for 'application/octet-stream'. I wonder why we decided to return 'application/octet-stream' instead of FALSE.

Okey, here is a patch I hope lives up to Dries review.

Committed to CVS HEAD. Thanks!

Does this apply to D6 as well? I am asking because #643808: favicon MIME is a duplicate then.

Reroll for D6, and changed a bit - did we still want to default to image/x-icon instead of image/vnd.microsoft.icon?

The way D7 does this sort of thing seems to be radically different now.

It's been more than a year now and I honestly don't understand why such a small fix doesn't get in. I think it's pretty much a no-brainer.

In IRC, pwolanin suggested I ditch the tertiary operator, so here's a reroll. I'm also using D6-style string concatenation (at least, I think I am; I despise D6-style string concatenation and have tended to ignore it in the past).

Looks better though using '==' instead of '===' would be more normal code style.

Meh. I generally prefer using '===' when there's no doubt that the variables will be the same type, but whatev - I need to fix my assignment operators anyway.

Check the D7 patch. If not matching file extension is found, then use 'image/x-icon'

…But see #18 above - not the patch, but the comment. Also, ignore the D7 patch, as it's been obliterated by other changes since it was committed. And with those other changes, D7 uses "image/vnd.microsoft.icon" for .ico files. Just check out a D7 installation which uses Garland, which uses an .ico file as its favicon. As I stated in #27, I believe that the use of image/x-icon as the default instead of image/vnd.microsoft.icon in earlier D7 patches was actually an error, and that what's reflected in my patch is the intended result.

Chatting further with pwolanin in IRC… We're both wondering why we fall back to "image/vnd.microsoft.icon" if file_get_mimetype() returns "application/octet-stream", even though we may be lying. Is using "application/octet-stream" so bad?

Nonetheless, the patch as it is now will match both D7's behavior and the behavior of previous versions of D6, given a file whose extension is not in file.inc's array. Thus, it's far better than what we have now and shouldn't break things either now or in the future.

right, I'd prefer to just omit:

+    if ($favicon_type == 'application/octet-stream') {
+      $favicon_type = 'image/vnd.microsoft.icon';
+    }

since in this situation it's an unknown file extension, but as above, this matches the current (incorrect?) D7 behavior.

The patch failed testing because the system tried to apply it to D6 core. However, it's still relevant and should be applied. Renaming so testbot doesn't try to test it anymore.

Not sure why this is "Needs review", there's no green patch in this thread.

Regardless, patch no longer applies:

curl -0 https://drupal.org/files/issues/415710-favicon-no-tert_D6.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2641  100  2641    0     0   1545      0  0:00:01  0:00:01 --:--:--  1965
error: file.inc: No such file or directory
error: theme.inc: No such file or directory
error: theme.maintenance.inc: No such file or directory

The patch in #37 has coding standards issues - missing spaces before and after . characters in string concatenation.

I can confirm the bug still exists in D6, and I can confirm that the patches in #37 and #31 are diverging from the current logic in D7 in a way that seems inappropriate for a backport, for example we don't seem to run the favicon type through check_plain() in D7.

Since this is a backport of something that has been fixed in D7+, I'm tagging it as such.

The issue summary should also be updated to explain the proposed resolution, as it currently only describes the problem.

#2094585: [policy, no patch] Core review bonus for #2072647: #theme 'maintenance_page' should support render arrays in #content.

Updated issue summary.