If the toggle_favicon variable is set to TRUE but a path to a custom favicon is not specified, phptemplate.engine outputs the following line to the page head:
<link rel="shortcut icon" href="" type="image/x-icon" />

While this might seem harmless, it has some unexpected side effects with some browsers. When rendering the page in Firefox (2.0.0.3), it assumes an empty shortcut icon path means that the current page *is* the favicon. The result is a duplicate request for every page served.

To reproduce:
- Fresh install Drupal
- Go to admin/build/themes/settings and uncheck "Use default shortcut icon"
- Implement this hook_menu() in your favorite test module:

function testmodule_menu($maycache) {
  if (!$maycache) {
    // Check for tricky favicons
    watchdog('favicon test', 'The requested page is '. $_GET['q']);
  }
}

- Visit some pages in your site (in Firefox).
- Go to admin/logs/watchdog. You'll see two duplicate log messages for every unique page you visited (one for the page, another for the supposed favicon).

Enabling the default favicon or entering a custom favicon will immediately correct the problem.

The attached patch simply checks for a favicon path before adding the shortcut icon code to the page.

This issue spawned out of imagefield being unable to upload an image: http://drupal.org/node/123977

CommentFileSizeAuthor
#22 emptyfavicon_0.patch574 bytesjohnalbin
#20 emptyfavicon.patch610 bytesjohnalbin
#14 icon.patch713 byteseaton
#11 drupal6-bug_empty_favicon.patch809 bytesGurpartap Singh
#5 no_favicon5.patch795 bytesquicksketch
#4 no_favicon.patch670 bytesquicksketch
phptemplate_favicon.patch434 bytesquicksketch

Comments

eaton’s picture

eaton commented
Priority: Minor » Critical

The code change is minor, but the priority isn't. This doubles the number of hits to any site that hasn't set up its favicon correctly, whenever it's viewed in Firefox. My brain hurts when I look at the thread where they tracked this bug down. ;-)

m3avrck’s picture

m3avrck commented
Status: Needs review » Reviewed & tested by the community

Looks good to me!

HEAD and 5.x!

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.1 » 6.x-dev
Status: Reviewed & tested by the community » Needs work

A proper boolean check, such as empty() or strlen() > 0 should be used.

quicksketch’s picture

quicksketch
he/him
commented
StatusFileSize
new no_favicon.patch670 bytes

Well looks like the location completely moved anyway... patch now applies to theme.inc where this logic was moved. However, it is impossible to test at the moment because theme settings aren't being retrieved properly. The 'favicon' variable is always set to the default no matter what the settings in the theme or global theme settings.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new no_favicon5.patch795 bytes

This patch is for the 5.x branch where I tested it successfully. Please apply the 6.x patch knowing that it would work if the variable were retrieved properly. We can fix the broken theme_get_setting in a future patch.

moshe weitzman’s picture

moshe weitzman commented

this has bugged me for a long long time. bravo!

m3avrck’s picture

m3avrck commented

!empty() would be easier to read and faster.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

it is impossible to test at the moment because theme settings aren't being retrieved properly.

Per comment #4 above, see bug 144162.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

trivial fix, and has the boolean logic suggested by drumm.

owen barton’s picture

owen barton commented

Let's get this in

empty() is significantly nicer than strlen() > 0 though

Gurpartap Singh’s picture

Gurpartap Singh commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new drupal6-bug_empty_favicon.patch809 bytes

This one affects theme.inc, and uses !empty(..).

RobRoy’s picture

RobRoy commented
Status: Needs review » Reviewed & tested by the community

Code looks good. Small change. RTBC to me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

This isn't the proper fix. The proper fix is the correct theme_get_setting().

eaton’s picture

eaton commented
Status: Needs work » Needs review
StatusFileSize
new icon.patch713 bytes

Like so?

eaton’s picture

eaton commented

No, not like that. I just thought this through and simply altering theme_get_setting() to solve this would makes it impossible to have NO favicon. Whether we fix theme_get_setting() or not, the theme should be smart enough to NOT blindly output a blank favicon URL.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

If a user unchecks “Use the default shortcut icon”, but doesn’t specify anything in “Path to custom icon”, the logical conclusion is that the user does not want a shortcut icon. And the user may not have noticed the “Toggle display: shortcut icon” checkbox since it is separated from the rest of the shortcut icon settings.

This seems to be a validation error.

So, for this type of situation, we have a few options:

  1. Dries suggestion: Silently modify toggle_favicon to be FALSE in theme_get_setting(). (As a side note, shouldn’t we be doing all the validation in theme_get_settings() instead of theme_get_setting()?)
  2. Add a system_theme_settings_validate() function that throws a validation error.

Option 2 seems to be a better solution for validation errors, but option 1 is easier to implement and gives a result that seems to be in-line with what the user wants. What does everyone else think?

owen barton’s picture

owen barton commented

* If we want to support the user having no favicon (rather than the Drupal default), without having to hard code the setting in their template.php then I think the phptemplate solution is the way to go (outputting links without a URL is just wrong!).
* If we want to *force* the user to have either the Drupal favicon, or their own - without a 'no favicon' option, then either the theme_get_setting(), or the validation option are the way to go.

Personally I think allowing no favicon is worth supporting, but if we do so we should make the interface more explicit. For example:

- We could have 3 radios (default favicon, no favicon, custom favicon), with the textbox associated with the third option only.

- Alternatively we could just add a description to the current textbox. I live this option less though, because of the dual-condition: "If default favicon is unchecked *and* the text box above is blank *then* no favicon will be displayed".

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Having no favicon is already supported. The issue is that the “Toggle display: shortcut icon” checkbox (which supports that feature and is at the top of the page) is easy to overlook when you are focusing on the “Shortcut icon settings” fieldset.

owen barton’s picture

owen barton commented

Oh. Duh :)
Agreed - this is not-obvious enough

I think silently unchecking the box is a reasonable course of action, and is a lot less annoying than a validation message like "You can't simultaneously have checkbox X checked and textbox Y blank".

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new emptyfavicon.patch610 bytes

Here’s the patch that modifies theme_get_setting() to silently set toggle_favicon to FALSE if default_favicon is FALSE and if favicon_path is empty.

dries’s picture

dries commented

John's patch works for me -- especially if we can fix that minor coding style issue that crept in. :)

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new emptyfavicon_0.patch574 bytes

Whoops! :-) Corrected patch…

owen barton’s picture

owen barton commented
Status: Needs review » Reviewed & tested by the community

RTBCing

dries’s picture

dries commented
Version: 6.x-dev » 5.x-dev

Committed to CVS HEAD. Probably needs to be backported.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to 5.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)