The <link> for the Favicon defaults to image/x-icon, even when it would make much more sense to use a different MIME type. Here, we simply default to image/x-icon, but provide specific support for image/x-gif and image/x-png if the file looks like one of those (i.e., ends in the appropriate extension).

See also #400042: Favicon.ico defaults to 'type="image/x-icon"', which is an earlier version of this patch for v6.6, and http://en.wikipedia.org/wiki/Favicon , which describes the favicon "standard" (I use the term loosely).

Files: 
CommentFileSizeAuthor
#37 415710-favicon-no-tert_D6.patch2.58 KBGarrett Albright
#31 415710-favicon-no-tert.patch2.58 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 415710-favicon-no-tert_0.patch.
[ View ]
#29 415710-favicon-no-tert.patch2.58 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 415710-favicon-no-tert.patch.
[ View ]
#27 415710-favicon-type.patch2.49 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 415710-favicon-type.patch.
[ View ]
#23 favicon-mime-type-415710-23.patch2.73 KBdixon_
Passed: 11305 passes, 0 fails, 0 exceptions
[ View ]
#19 favicon-mime-type-415710-19.patch2.56 KBJohnAlbin
Passed: 11276 passes, 0 fails, 0 exceptions
[ View ]
#18 favicon-mime-type-415710-18.patch1.55 KBJohnAlbin
Passed: 11276 passes, 0 fails, 0 exceptions
[ View ]
#14 theme.finaltry.patch719 bytesBMDan
Passed: 11255 passes, 0 fails, 0 exceptions
[ View ]
#12 theme.finaltry.patch718 bytesBMDan
Failed: Failed to install HEAD.
[ View ]
#8 theme.inc_.head_.patch662 bytesBMDan
Failed: 10873 passes, 4 fails, 813795 exceptions
[ View ]
#6 theme.inc_.head_.patch644 bytesBMDan
Failed: Failed to apply patch.
[ View ]
#5 theme.inc_.head_.patch654 bytesBMDan
Failed: Failed to apply patch.
[ View ]
#3 favicon.patch675 bytesBMDan
Failed: Failed to apply patch.
[ View ]
favicon.patch.txt1016 bytesBMDan

Comments

BMDan’s picture

Status:Active» Needs review

The "type" for the Favicon defaults to image/x-icon, even when it would make much more sense to use a different MIME type. Here, we simply default to image/x-icon, but provide specific support for image/gif and image/png if the file looks like one of those (i.e., ends in the appropriate extension).

See also #400042: Favicon.ico defaults to 'type="image/x-icon"', which is an earlier version of this patch for v6.6, and http://en.wikipedia.org/wiki/Favicon , which describes the favicon "standard" (I use the term loosely).

c960657’s picture

Status:Needs review» Needs work

I suggest that you use file_get_mimetype().

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

BMDan’s picture

Status:Needs work» Needs review
Issue tags:+favicon
StatusFileSize
new675 bytes
Failed: Failed to apply patch.
[ View ]

Good suggestions, c960657. I've rolled them into this new patch, which also implements a "default to image/x-icon" feature for those using strange favicon names.

Status:Needs review» Needs work

The last submitted patch failed testing.

BMDan’s picture

Status:Needs work» Needs review
StatusFileSize
new654 bytes
Failed: Failed to apply patch.
[ View ]

theme.inc went through a bunch of changes in HEAD; this is the same patch, but freshly diff'd with new offsets and context to reflect those changes.

BMDan’s picture

StatusFileSize
new644 bytes
Failed: Failed to apply patch.
[ View ]

Slightly-better patch; I'd missed the change to _add from _set.

Status:Needs review» Needs work

The last submitted patch failed testing.

BMDan’s picture

StatusFileSize
new662 bytes
Failed: 10873 passes, 4 fails, 813795 exceptions
[ View ]

+1 to Berdir in IRC for pointing out that I was missing the directory name in my patch.

BMDan’s picture

Status:Needs work» Needs review

Switching state to "needs review".

Status:Needs review» Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Coding style issues in the patch.

BMDan’s picture

Status:Needs work» Needs review
StatusFileSize
new718 bytes
Failed: Failed to install HEAD.
[ View ]

The shinest and latest version of the patch, complete with the latest stylistic changes, and, as a bonus, a heaping helping of awesome. My thanks to Michelle and chx in IRC for their invaluable assistance. The fact that it works is their fault. Needless to say, the remaining bugs are mine.

Status:Needs review» Needs work

The last submitted patch failed testing.

BMDan’s picture

Status:Needs work» Needs review
StatusFileSize
new719 bytes
Passed: 11255 passes, 0 fails, 0 exceptions
[ View ]

/me mutters something dark about the auto-tester's mother.

chx’s picture

Status:Needs review» Needs work

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.

BMDan’s picture

Status:Needs work» Needs review

I'm muttering about the auto-tester because it produces ridiculous results. http://testing.drupal.org/pifr/file/1/theme.finaltry.patch should say "parse error" or similar, not "failed to install HEAD" with no additional information. If I get a spare moment, I'll look into how to improve the auto-tester, but that is not a top priority for me right now.

Now, I feel I need to caveat the rest of this comment. I apologize if any of this seems combative, as that's not my intent. I'm trying to dissuade you from what is probably a deeply-held belief, namely that indiscriminately caching functions' return values in the manner you describe improves performance in a significant way while additionally not substantially detracting from legibility, maintainability, or other important code factors. Accordingly, I've been quite verbose in describing why I believe you are wrong on this count, and quite specific on the followup measures I'd like to see you take. You always have the option, however, to do as you please, including to ignore this patch entirely from now on, to delete this issue from the bug system if that is within your powers, to cause frogs to rain from the sky, etc., etc. I'm just providing a framework within which I believe further discussion will be most productive.

So, let me lay it out frankly: your suggestion is ridiculous. This is a singly-"include"d portion of the code, and the call to theme_get_setting is only made twice. http://api.drupal.org/api/function/theme_get_setting already caches the settings, so there are no calls to the DB; only the second half of the function is actually run. "Premature optimization is the root of all evil," etc., etc. It's the job of the compiler/bytecode cache/optimizer/some portion of the hierarchy generally much higher and smarter than us to determine what is static and what is not. See http://www.acm.org/ubiquity/views/v7i24_fallacy.html for more on this; worrying about things like caching the result of this call is micro-optimization; you could just as easily blow up a memory-constrained environment by optimizing this way as you could correct a performance hiccup in a CPU-bound one. In fact, it's easily arguable that you're more likely to cause a performance problem with the creation and subsequent GC of a twice-used variable than a single "wasted" call to what the optimizer can recognize as a static function.

To bring this firmly and thoroughly into the realm of the hyperbolic, imagine that we created a shared-memory architecture where these variables would only need to be loaded once per (server restart||child restart||insert reload trigger here). Now we've gained significant amounts of performance. Of course, what I've described is the static $settings variable in theme_get_setting--the API has already done the heavy lifting for us, in other words! Double caching, then, is clearly not the solution.

Now, as I see it in my admittedly-limited understanding of you, the project, my code, this dispute, and myself, and the universe as a whole (barring Pluto, which as we all know is not a planet) you have one of just a couple good paths ahead open to you:

  1. You can accept my argument and not change the status to "needs work" again.
  2. You can disagree with my argument, and take it upon yourself to fix the patch I submitted so that it meets your standards.
  3. You can point to a project policy document that says, "Without regard to whether or not it's pointless, stupid, and detrimental to performance, coding style, and potential race conditions, we will always cache the results of all function calls at all levels." At which point I will resubmit this patch in the style you suggest and immediately go to work fixing that policy document.
chx’s picture

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

JohnAlbin’s picture

StatusFileSize
new1.55 KB
Passed: 11276 passes, 0 fails, 0 exceptions
[ View ]

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.

JohnAlbin’s picture

StatusFileSize
new2.56 KB
Passed: 11276 passes, 0 fails, 0 exceptions
[ View ]

Whoops! Forgot about template_preprocess_maintenance_page().

dixon_’s picture

Status:Needs review» Reviewed & tested by the community

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.

JohnAlbin’s picture

Yay! Dixon, patch-reviewer extraordinaire!

Dries’s picture

Status:Reviewed & tested by the community» Needs work

- 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.

dixon_’s picture

Status:Needs work» Needs review
StatusFileSize
new2.73 KB
Passed: 11305 passes, 0 fails, 0 exceptions
[ View ]

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

Dries’s picture

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks!

Status:Fixed» Closed (fixed)

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

klonos’s picture

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

Garrett Albright’s picture

Version:7.x-dev» 6.x-dev
Priority:Minor» Normal
Status:Closed (fixed)» Needs review
StatusFileSize
new2.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 415710-favicon-type.patch.
[ View ]

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.

klonos’s picture

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.

Garrett Albright’s picture

StatusFileSize
new2.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 415710-favicon-no-tert.patch.
[ View ]

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).

pwolanin’s picture

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

Garrett Albright’s picture

StatusFileSize
new2.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 415710-favicon-no-tert_0.patch.
[ View ]

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.

pwolanin’s picture

Status:Needs review» Needs work

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

Garrett Albright’s picture

…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.

Garrett Albright’s picture

Status:Needs work» Needs review

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.

pwolanin’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 415710-favicon-no-tert.patch, failed testing.

Garrett Albright’s picture

StatusFileSize
new2.58 KB

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.

Garrett Albright’s picture

Status:Needs work» Needs review
thedavidmeister’s picture

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +Needs backport to 6.x

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: [meta] [experimental] Core review bonus for #2072647: #theme 'maintenance_page' should support render arrays in #content.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture