[drupal-devel] [feature] favicon patch for themes

Steven drupal-devel at drupal.org
Wed May 25 06:02:51 UTC 2005


Issue status update for http://drupal.org/node/20809

 Project:      Drupal
 Version:      cvs
 Component:    theme system
 Category:     feature requests
 Priority:     normal
 Assigned to:  nysus
 Reported by:  nysus
 Updated by:   Steven
 Status:       patch

I committed a modified patch to HEAD:


- The favicon should be implemented as a Theme Engine option.
PHPTemplate already picked up shortcut icons if there was a favicon.ico
in the theme directory.
- The favicon should use drupal_set_html_head() for adding itself. That
way, it can be implemented like a proper theme engine feature which can
be enabled and disabled.


Now it should do all the things mentioned before: per theme,
user-configurable, optional favicons.


I also updated the contrib theme engines for this.




Steven



Previous comments:
------------------------------------------------------------------------

April 19, 2005 - 01:42 : nysus

Attachment: http://drupal.org/files/issues/favicon.patch (4.57 KB)

Here is a patch that allows a user to manually set the path to the
"favicon.ico" file.  It makes a single change to the user interface: it
adds a text field to the "Logo image setting" section of the
"themes/settings" page.


This is especially useful for sites sharing a single code base as you
no longer need to create a new theme just so you can have a distinct
favicon.ico.




------------------------------------------------------------------------

April 23, 2005 - 06:28 : Steven

There is a lot of mailing list discussion about this patch.


One more note though: the <link /> tags are missing the trailing slash
for XHTML.




------------------------------------------------------------------------

April 23, 2005 - 17:06 : nysus

Attachment: http://drupal.org/files/issues/favicon2.patch (6.01 KB)

Here is a revision to the patch based on the feedback received on the
discussion list.  You can now override the default favicon the same you
way you can override the default site logo.  There is one difference: 
When a logo is uploaded, it is checked to determine whether it is a
valid image file using PHP's getimagesize function.  Since this
function does not work with .ico files, no validation of the uploaded
file is performed.  Advice on how (or if) validation needs to be
done---or any other suggestions and corrections---is welcome.




------------------------------------------------------------------------

April 23, 2005 - 19:45 : Steven

There is still a slash missing in bluemarine.


This patch appears to have per-theme favicons, but the setting to
control this is in fact defined by the theme engine, not the template
(through the list of support theme features). I was more thinking of
simply checking for the presence of a favicon.ico in the theme's own
directory. But I'm not sure if we really need this: it does make it
very obvious to people editing a theme that there is an icon file they
can change. On the other hand, few people will use a theme's default
favicon. It's more useful as a customization, and the UI option is good
enough for that.


Some text suggestions: "favorite icon" is not clear. I think the phrase
"shortcut icon" is better (that's the standard name) if we accompany it
with more text. Something like:


"Check this box to use the default shortcut icon or 'favicon'. This
icon is displayed in the address bar and bookmarks in most browsers."


"The path to the icon file you would like to use as your custom
shortcut icon."
(note: i replaced "image file" with "icon file" because for IE,
favicons need to be .ico files)


'Shortcut icon settings'




------------------------------------------------------------------------

April 23, 2005 - 20:07 : nysus

> I was more thinking of simply checking for the presence of a
favicon.ico in the theme's own directory.


This wouldn't work very well for sites sharing the same code base. 
Using your suggestion, you would have to create a custom theme just to
have a customized favorite icon.


> But I'm not sure if we really need this...


Can you please clarify to what "this" refers to?  It's unclear to me
what your suggestion is.


I'll incorporate your other suggestions and corrections into the next
revision.




------------------------------------------------------------------------

April 23, 2005 - 20:23 : kbahey

The patch as it stands is only for xtemplate. What about phptemplate?
Will it work with phptemplate?


+1 on the concept though. I like a more generic solution, like what you
proposed, that allows the favico to be added from an admin interface
just like the logo.


As for the comment:


>> I was more thinking of simply checking for the presence of a
>> favicon.ico in the theme's own directory.
>
> This wouldn't work very well for sites sharing the same code base.
> Using your suggestion, you would have to create a custom theme
> just to have a customized favorite icon.


If the theme is installed under sites/sitename/themes then its favico
will only show for the given site.




------------------------------------------------------------------------

April 23, 2005 - 20:41 : Steven

What I meant was to have the favicon default to
themes/themename/favicon.ico if it exists, and use favicon.ico
otherwise. I would still allow you to override the favicon regardless
over the admin interface. But I'm not sure if default per-theme icons
are useful anymore (that's what I meant with 'this').




------------------------------------------------------------------------

April 23, 2005 - 20:52 : nysus

> The patch as it stands is only for xtemplate. What about phptemplate?
Will it work with phptemplate?


xtemplate is the only templating engine in the Drupal core.  This patch
will not work with phptemplate until the maintainer of phptemplate
engine incorporates favicon functionality into the phptemplate engine.


> If the theme is installed under sites/sitename/themes then its favico
will only show for the given site.


Yes, I understand this, but it's a real hassle to have to create a new
theme directory just to facilitate the creation of a favicon.  I feel
strongly that a customized favicon is a common enough task that it
should be built into the Drupal user interface and should be made as
easy as possible.




------------------------------------------------------------------------

April 24, 2005 - 21:47 : nysus

Attachment: http://drupal.org/files/issues/favicon3.patch (6.59 KB)

Here is a revision to the patch that incorporates some suggestions that
have been made here.  It also fixes a bug in the previous version of
the patch.




------------------------------------------------------------------------

April 26, 2005 - 00:04 : Junyor

Why does this have to be part of the theme at all?  I think others
mentioned on the mailing list discussion that favicons are
site-specific, not theme-specific.  Thus, they are candidates for
inclusion in drupal/sites/ rather than as part of a theme.  Then, the
LINK element can be included with drupal_set_head (or whatever it's
called).


I like having the setting in the admin settings, similar to the site
logo setting.  And don't forget: favicons can be any image type these
days.  They won't necessarily by .ico files.


So, +1 for better customization of favicons and -1 for the
implementation.




------------------------------------------------------------------------

April 26, 2005 - 00:19 : nysus

> Why does this have to be part of the theme at all? I think others
mentioned on the mailing list discussion that favicons are
site-specific, not theme-specific.


I was actually the one who brought this point up.  However, upon
further reflection about the suggestion, some users may want to change
the icon to match the color scheme of the different themes.  Plus,
since this is what was requested, this is what I delivered.


> And don't forget: favicons can be any image type these days. They
won't necessarily by .ico files.


This is true, at least for FF and maybe for recent verision of IE, too.
 However, users may still try to upload .ico files.  And, right now, the
PHP function called to process images for the logo file will not work
for .ico files.  But, I'm not sure what your point is, here.  This
patch will work with any type of image file, including .ico.







More information about the drupal-devel mailing list