WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 5 years ago

#3426 closed feature request (fixed)

Should include default favicon

Reported by: robertaccettura Owned by: cpoteet
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8
Component: Performance Keywords: has-patch tested health-check commit
Focuses: Cc:

Description

By default there should be a favicon. Some browsers (won't mention names) try and retrieve whatever is at /favicon.ico (for those of us who install wp at the root of the hostname. If wordpress handles 404's as well, you'll be wasting cpu/memory to serve up a 404 for not good reason. I guess it could be a wordpress logo, or just something generic like a white square (blank in the url bar) 100% transparent (does that work in all browsers?) or something else rather neutral.

It's rather wasteful on behalf of the UA, but I guess that's the way the cookie crumbles.

Attachments (8)

favicon.ico (3.6 KB) - added by cpoteet 9 years ago.
Suggested Favicon
favicon.2.ico (198 bytes) - added by hakre 6 years ago.
favicon.3.ico (198 bytes) - added by hakre 6 years ago.
3426.diff (568 bytes) - added by Denis-de-Bernardy 6 years ago.
3426_favicon.diff (2.2 KB) - added by robertaccettura 6 years ago.
Alternate Patch
3426_favicon_v2.diff (984 bytes) - added by robertaccettura 6 years ago.
Another Alternate
3426-4.patch (728 bytes) - added by azaozz 6 years ago.
3426.5.diff (1.3 KB) - added by sivel 5 years ago.
refresh of 3426-4.diff to work with changes from #11881

Download all attachments as: .zip

Change History (66)

@cpoteet9 years ago

Suggested Favicon

comment:1 @cpoteet9 years ago

  • Component changed from Optimization to Template
  • Keywords favicon added
  • Milestone set to 2.0.7
  • Owner changed from anonymous to cpoteet
  • Priority changed from low to lowest
  • Severity changed from minor to trivial
  • Status changed from new to assigned

Here is a suggested favicon from the WP logo.

comment:2 @Martin-FR9 years ago

I don't agree this !

comment:3 @JeremyVisser9 years ago

The favicon looks crummy in its scaled state. If I have time, I think I'll to a retouch pixel-by-pixel.

comment:4 @robertaccettura9 years ago

I wonder if it should be located within the theme, or in the home directory...

comment:5 @markjaquith9 years ago

  • Milestone changed from 2.0.7 to 2.2

If located within the theme, it can only be used for people with mod_rewrite or 404 processing for their permalinks.

comment:6 @matt9 years ago

  • Resolution set to wontfix
  • Status changed from assigned to closed

I don't think we should include a default favicon. WP isn't always in the root. Even if it was, including one might overwrite someone's default.

comment:7 @foolswisdom9 years ago

  • Milestone 2.2 deleted

comment:8 in reply to: ↑ description @revmj7 years ago

  • Priority changed from lowest to normal
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Replying to robertaccettura:

By default there should be a favicon. Some browsers (won't mention names) try and retrieve whatever is at /favicon.ico (for those of us who install wp at the root of the hostname. If wordpress handles 404's as well, you'll be wasting cpu/memory to serve up a 404 for not good reason. I guess it could be a wordpress logo, or just something generic like a white square (blank in the url bar) 100% transparent (does that work in all browsers?) or something else rather neutral.

It's rather wasteful on behalf of the UA, but I guess that's the way the cookie crumbles.

I'm re-opening this as I work for a web host that provides one-click installations of word press. While including a default favicon.ico may not be ideal, I can say the lack of a favicon.ico causes the CPU usage of a site to jump by 400%. This is really pretty insane.

So here are 2 alternatives to including a favicon.ico:

  1. During the setup process, simply do a 'touch' of the favicon.ico file in the base of the install. If you are concerned about changing the mtime of the file, then check for it's existence first. You could even implement putting a WordPress branded favicon.ico in place if you wanted the additional free "advertising".
  1. Modify the default .htaccess file to not redirect to index.php on a 404 error if the requested files was favicon.ico.

Either of these solutions would resolve this issue. This bug was reported 2 years ago. Think of how many barrels of oil have been burned needlessly. Just think of the positive spin you could put on this simple fix in comparison to the negative spin that could easily be applied. Seems like a no-brainer to me.

comment:9 @revmj7 years ago

  • Component changed from Template to General

comment:10 @sivel7 years ago

Perhaps something like what was implemented on wp.com (http://en.blog.wordpress.com/2008/12/02/go-get-a-blavatar/) could be implemented for wp.org.

comment:11 follow-up: @mrmist7 years ago

Not a lot of negative spin really though is there, as this was closed 2 years ago and not a peep since.

Generally -1 on this, I think.

It's added complexity in the installation, because rather than just dropping a zip, you're having to have an installer that shifts .ico files around.

Absolutely -1 to just dropping the .ico file in regardless, potentially undoing people's work.

To be honest, if you're a web hosting provider worried about the cost of not serving up .ico files, you could implement your own host-wide policy on it.

Likewise if you are a site owner interested in having a favicon, then you'd already have one.

Lastly I think part of the point of a favicon is to distinguish one site from another by its icon. That is somewhat diluted if all the default WP installs have the same icon.

I think this is generally more plugin land.

comment:12 @westi7 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

We aren't going to include a default favicon.

comment:13 in reply to: ↑ 11 ; follow-up: @lloydbudd6 years ago

  • Component changed from General to Performance
  • Milestone set to 2.9
  • Resolution wontfix deleted
  • Severity changed from trivial to normal
  • Status changed from closed to reopened
  • Type changed from enhancement to defect (bug)
  • Version set to 2.8

Replying to mrmist:

Absolutely -1 to just dropping the .ico file in regardless, potentially undoing people's work.

I'm calling this a defect and think this should be revisited. Although, it is not common for a heavily trafficked site not to have a favicon it seems to be a significant problem for web hosts:

http://blog.nearlyfreespeech.net/2009/06/16/quick-wordpress-performance-tip-create-a-favicon/

http://www.dreamhoststatus.com/2009/06/10/faviconico-favicongif-and-robotstxt-update/

Possible Solution:

If the request gets to WordPress then a favicon does not exist, so WordPress can serve up WordPress's logo favicon from wp-includes/images/

Generally I want to stay away from adding / modifying the favicon information in the <header>

To Investigate:

Any solution would want to confirm that it works with popular caching solutions such as supercache and batcache.

comment:14 @demetris6 years ago

  • Cc dkikizas@… added

comment:15 in reply to: ↑ 13 ; follow-up: @westi6 years ago

  • Keywords needs-patch added

Replying to lloydbudd:

Replying to mrmist:

Absolutely -1 to just dropping the .ico file in regardless, potentially undoing people's work.

I'm calling this a defect and think this should be revisited. Although, it is not common for a heavily trafficked site not to have a favicon it seems to be a significant problem for web hosts:

http://blog.nearlyfreespeech.net/2009/06/16/quick-wordpress-performance-tip-create-a-favicon/

http://www.dreamhoststatus.com/2009/06/10/faviconico-favicongif-and-robotstxt-update/

Possible Solution:

If the request gets to WordPress then a favicon does not exist, so WordPress can serve up WordPress's logo favicon from wp-includes/images/

Generally I want to stay away from adding / modifying the favicon information in the <header>

To Investigate:

Any solution would want to confirm that it works with popular caching solutions such as supercache and batcache.

I think if the request gets to WordPress then the shared hosts have already lost anyway.

While we can improve on the performance of serving a 404 page by sending out the icon if it gets to WordPress. I am not sure that is going to make it as good for the shared hosts as the files being there.

Maybe we could add rewrite rules to the htaccess for these specific files instead?

comment:16 in reply to: ↑ 15 ; follow-up: @lloydbudd6 years ago

I think if the request gets to WordPress then the shared hosts have already lost anyway.

While we can improve on the performance of serving a 404 page by sending out the icon if it gets to WordPress. I am not sure that is going to make it as good for the shared hosts as the files being there.

I was thinking that it might help a large chunk of scenarios, where they would be smart enough to have a caching solution in place, but I'm know thinking that solving it there also is not the correct place.

Maybe we could add rewrite rules to the htaccess for these specific files instead?

Interesting idea, though haven't htaccess dependent solutions to other problems always been frowned upon.

Looks like the best solution is theme hook based? Insert default favicon info if user/theme does not override.

comment:17 in reply to: ↑ 16 @robertaccettura6 years ago

Replying to lloydbudd:

Looks like the best solution is theme hook based? Insert default favicon info if user/theme does not override.

Could also do a file_exists() to check for a favicon in the root, and put a default if none is there already. Should be done in the theme so that it can be replaced/disabled easily.

comment:18 @robertaccettura6 years ago

Actually... here's another option:

If a 404 is served for '/favicon.ico'... then set a wp_option which would flag in the wp-admin alerting any user with the ability to edit options to enable a default favicon checkbox (which can use standard wp header hook) or to create a favicon.

This a little more graceful.

comment:19 @mrmist6 years ago

Remember if there is some solution that involves checking for the existance of the favicon, then it also needs to check that WordPress itself is in the root folder - if not, then it might simply be inappropriate to add a WordPress type favicon to the site.

In general I think this sounds like a whole bunch of extra code and / or options that would add clutter to admin and make the average user go "eh?". But what do I know, I'm not average. :)

comment:20 @robertaccettura6 years ago

This isn't so much a user feature, but an ISP and performance feature.

Serving up a 404 is a waste of cpu, memory and database connections. It turns 1 pageview into 2. Hence if there is a reasonable way to fix this and prevent the bogus pageview... for people who don't know about using a favicon, they will improve performance, and their hosts will be able to shed some unnecessary load.

comment:21 follow-up: @lloydbudd6 years ago

An occurrence of this issue was Matt's 1st story in his Velocity 09 talk
http://velocityconference.blip.tv/file/2293079/

  1. shipping with a default favicon in wp-includes/images/ seems like a no brainer, and updating the bundled themes to use it.
  1. Triggering a warning in the admin if 404 is served for '/favicon.ico' sounds elegant to me. Though maybe should take a step back and look for generalized solutions.

comment:22 in reply to: ↑ 21 @hakre6 years ago

Replying to lloydbudd:

  1. shipping with a default favicon in wp-includes/images/ seems like a no brainer, and updating the bundled themes to use it.

wp-includes/images/ won't help for /favicon.ico requests.

  1. Triggering a warning in the admin if 404 is served for '/favicon.ico' sounds elegant to me. Though maybe should take a step back and look for generalized solutions.

So what for? Then just put a favicon.ico into the wordpress zip and go for it. I suggest to use a very reduced version (for example 1bit colors) as favicon.ico so that a site owner is aware of the possibilities.

@hakre6 years ago

comment:23 @hakre6 years ago

  • Keywords has-patch tested added; favicon needs-patch removed

comment:24 follow-up: @sivel6 years ago

  • Keywords needs-patch added; has-patch tested removed

The patch is going to need to be more extensive than that. The automatic upgrade process will also need to check and make sure there is not a custom favicon.ico before copying the default favicon over the users custom favicon.

comment:25 @filosofo6 years ago

Why not just make a rewrite rule for it, like the robots.txt one? Then if a real favicon file exists the rewrite rules are such that it will be served; otherwise, the default in /wp-includes/images/ can be used.

comment:26 @sivel6 years ago

I believe one of the reasons behind adding a default was to reduce server load for requests to favicon.ico. So that a request wouldn't have to go through PHP to deliver the favicon.

If we wanted to add a new Apache mod_rewrite rule that will go into .htaccess to redirect requests to the one in wp-includes/images when a favicon.ico doesn't exist I would be happy with that.

comment:27 @hakre6 years ago

rewrite rule to the resuce. those server who do not have rewrite rules won't have the problem with the 404s, right?

It would be possible to block the request then us sparing a file.

Additionally I add another "missing favicon"-favicon.

@hakre6 years ago

comment:28 in reply to: ↑ 24 @hakre6 years ago

Replying to sivel:

The patch is going to need to be more extensive than that. The automatic upgrade process will also need to check and make sure there is not a custom favicon.ico before copying the default favicon over the users custom favicon.

asccording to what filosofo suggest, this ain't any issues. no checks need to be made because the core file is linked only if not a users favicon actually exists.

comment:29 @janeforshort6 years ago

  • Type changed from defect (bug) to feature request

comment:30 @ryan6 years ago

We already intercept robots.txt requests if the file doesn't exist. I think we can do the same for favicon.ico.

And if we want to allow uploading favicons we can consider porting the blavatar stuff from wp.com.

comment:31 follow-up: @ryan6 years ago

Granted, if we do it like robots.txt we'll still load a lot of WP unless we add a short circuit.

comment:32 in reply to: ↑ 31 @azaozz6 years ago

Replying to ryan:

Granted, if we do it like robots.txt we'll still load a lot of WP unless we add a short circuit.

Yes, so it would still run WP twice on each page load. Seems revmj and westi have the right solution: add another rule to htaccess to redirect requests for /favicon.ico to wp-includes/images/favicon.ico when the file doesn't exist.

Of course this will have to go at the bottom of htaccess so if there's already a redirect for favicon, we don't override it.

comment:33 follow-up: @azaozz6 years ago

Another possible solution would be to intercept requests for favicon.ico very early, perhaps near the top of wp-settings.php and output something like:

if ( preg_match('/favicon.ico$/i', $_SERVER['REQUEST_URI']) ) {
    header( 'Content-Type: image/x-icon' );
    header( 'Content-Length: 0' );
    exit;
}

comment:34 in reply to: ↑ 33 @filosofo6 years ago

Replying to azaozz:

Another possible solution would be to intercept requests for favicon.ico very early, perhaps near the top of wp-settings.php and output something like:

I like this idea, especially if you add a no-cache header, because it will keep browsers from caching the default favicon even after a real one has been put in place.

comment:35 @westi6 years ago

Is the best solution here really to provide a duff/correct image or to highlight to the user that there site would perform better if they had one.

I think I the following is as far as we should go in the core:

  • Add a rule to the default .htaccess section which points at a favicon.ico in wp-includes/images if one does not exist in /

Other things which should/could be addressed by one or many canonical plugin:

  • Add a warning for admins - maybe we need a performance page or even better a performance canonical plugin - it's not something you need forever!
  • blavatar idea

comment:36 @hakre6 years ago

It just pops in my mind to think about having the favicon placed per theme as well. I think this makes sense for example if you make a nice theme and want that to ship with a favicon. But that's just an idea.

comment:37 @westi6 years ago

As we are now feature frozen it is too late to include this into 2.9.

Moving to Future Release as there is currently no patch.

I think this is best solved by a canonical plugin which checks for existence of the file and suggests adding one to improve performance.

comment:38 @westi6 years ago

  • Milestone changed from 2.9 to Future Release

comment:39 @westi6 years ago

  • Keywords health-check added

comment:41 @hakre6 years ago

  • Milestone changed from Future Release to 3.0

comment:42 @Denis-de-Bernardy6 years ago

the idea discussed in #11412 was to add one or two things:

  • a few lines in the htaccess file to catch favicon requests and bail accordingly.
  • a handler in WP that bails before the WP_Query

comment:43 @Denis-de-Bernardy6 years ago

I'm playing around with adding this rewrite rule atm:

RewriteRule \.(gif|png|jpe?g|css|js|ico)$ - [L]

@Denis-de-Bernardy6 years ago

comment:44 @Denis-de-Bernardy6 years ago

  • Keywords has-patch added; needs-patch removed

comment:45 @Denis-de-Bernardy6 years ago

  • Keywords tested added

Marking this as tested. it has been running on my server for a week without a hiccup.

comment:46 @hakre6 years ago

Just made a little review, I like the simplicity of this patch.

It might make sense to limit it to favicon.ico only in case someone actually wants to return dynamically generated images or so. Just as a note. Or: Offer a filter then for it, so a plugin can interact into that.

comment:47 @matt6 years ago

Rather than changing mod_rewrite rules, which people running on non-Apache platforms like nginx or IIS probably won't appreciate, we should special-case favicon.ico with an early exit someplace like wp-settings.php since this is a fairly common problem that causes performance issues, as azaozz suggested with his simple patch.

@robertaccettura6 years ago

Alternate Patch

comment:48 @robertaccettura6 years ago

That's what I originally had in mind. Other variations would be fpassthru() rather than base64_decode() or moving it higher up (index.php would be the most optimal) to prevent PHP from having to needlessly compile multiple files before aborting to a favicon.

comment:49 @Denis-de-Bernardy6 years ago

wouldn't an actual file and using readfile() fit as well?

comment:50 @robertaccettura6 years ago

I was under the impression fpassthru was faster than readfile() but either would work.

@robertaccettura6 years ago

Another Alternate

comment:51 @robertaccettura6 years ago

This one uses a default favicon in the wp-content directory. It also sets cache headers for 3hrs which should be adequate for an average session and avoid multiple requests. While this gets rid of DB connections and most overhead it's still php processing, anything that can be cut can be helpful.

comment:52 @azaozz6 years ago

Thinking more about this: best "fix" is when the site has an actual icon. Don't think WordPress should be substituting it if it doesn't exist (millions and millions of web pages with the same favicon).

Also the browsers seem to have "special" caching for these icons, don't think we need to try to refresh them every few hours (would probably affect only network caches).

3426-4.patch is based on 3426_favicon_v2.diff.

@azaozz6 years ago

comment:53 @robertaccettura6 years ago

  1. Not really sure a null image is really per-spec though it should work in all browsers without issue.
  2. Will proxies and reverse proxies cache a request with a response of 0 bytes? Or will it hit origin every time? If it's hitting the origin, that defeats the point. Pretty sure many don't cache that.
  3. The reason for setting cache is to ensure any proxy will keep it cached and offset load issues.

The point here isn't design but to reduce load due to this operator error.

If not the wordpress logo (which is good enough on wordpress.com's millions of blogs) then a transparent image would serve the purpose just as well.

comment:54 @robertaccettura6 years ago

Sorry about the formatting above :-/

comment:55 @nacin5 years ago

I'm +1 on 3426.4.patch (though it is stale due to #11881). Let's get this in.

@sivel5 years ago

refresh of 3426-4.diff to work with changes from #11881

comment:56 @nacin5 years ago

  • Keywords commit added

comment:57 @Viper007Bond5 years ago

Looks good to me.

comment:58 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [13205]) Bail early for favicon.ico requests so we don't load WP twice. Props azaozz, sivel. Fixes #3426

Note: See TracTickets for help on using tickets.