Remove wp_favicon_request

[jonoaldersonwp]

wp_favicon_request(), which lives in https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/load.php, should be removed.

Theme/plugin authors need the ability to manage the behaviour of /favicon.ico requests, but the current behaviour prevents the programmatic modification/filtering of requests to /favicon.ico.

This is particularly timely+relevant, because:

• Google recently began to display favicons in its search results, but, many themes don't have / properly support favicons.
• The Customizer offer support for defining a 'Site Icon', but there's currently no mechanism to expose this at /favicon.ico.
• The current behaviour is inconsistent with other parts of WP

[mukesh27]

Patch that remove wp_favicon_request function from wp-includes/load.php

[SergeyBiryukov]

Introduced in [13205] for #3426 to avoid a performance hit of serving a full 404 page on every favicon request.

Could we maybe make it filterable instead to avoid that performance hit while allowing themes and plugins to modify the default behavior?

[jonoaldersonwp]

Making it filterable would be a good step, though I'd still be uncomfortable that this is still handled so differently from other endpoints, etc. It sets/maintains a bad precedent.

[joostdevalk]

Patch

[joostdevalk]

Just added another patch that also removes the call to the function instead of just the function.

I'm sorry but the performance argument is moot, that goes for literally _every_ other 404 request as well. People should just have a favicon. The problem with the current implementation is that we can't make WordPress serve a favicon because it outright sends a blank one.

[ocean90]

Can you please point me to a document where Google explicitly says that it's only indexing a (legacy) favicon.ico in the root directory of a site?

• wp_favicon_request() handles only requests to /favicon.ico and only, if a real file doesn't exist at the same place.
• "many themes don't have / properly support favicons." – Thankfully! Just like logos, favicons shouldn't be handled by the theme, otherwise every site would have the same icon. That's why we have the site icon since WordPress 4.3.
• "I'm sorry but the performance argument is moot" – I'm sorry, it's not. As mentioned in the commit, we'd load WordPress twice for a single request, one for the regular page and one for the (missing) icon. The existence of a favicon.ico is not given because it usually requires access to the file system to place it right in the root of the directory. And that's not even possible for sites which have installed their WordPress in a sub directory.
• In the ​Help Center Google explains how a favicon should be implemented. And it's not saying by placing a favicon.ico in the root directory, but rather by adding the <link> tag to the header, pointing the a favicon at any place on the same domain. For the rel attribute Google supports shortcut icon,icon,apple-touch-icon, and apple-touch-icon-precomposed.
• Additionally, the ​guidelines include what formats are supported. And it's basically "Any ​valid favicon format is supported." (ico, png, jpg, svg, and others).

The Customizer offer support for defining a 'Site Icon', but there's currently no mechanism to expose this at /favicon.ico.

Based on my comments above, it doesn't have to:

• The site icon allows the user to define "a visual representation of your website's brand" ✅
• The site icon is crawlable by Google ✅
• The site icon is a multiple of 48px square ✅
• The site icon is a supported file format (png, jpeg, gif) ✅
• The site icon is exposed in the header with the relevant <link> tag, using the icon and apple-touch-icon-precomposed relationship. ✅

So, I'm tending to close this as wontfix since it doesn't seem to be necessary.

PS: Adding a filter isn't possible since the plugin API isn't loaded at this point, neither are plugins.

[joostdevalk]

@ocean90 I'm fine debating with you over this, I'm not fine with you just adding a close keyword: that's just your opinion and you have no more and no less authority over this than I do. So adding that like this is RUDE and I don't appreciate it.

By removing this function, we'd allow plugins to hook into the URL with the rewrite API, currently they can't because we serve a stupid empty favicon. Our current solution is ridiculously poor. Even shipping a default WordPress favicon with every install would be better than this, or even better: outputting the site icon in the favicon.ico.

I'm happy to discuss other paths to a better solution of this problem.

[birgire]

In general (whether that's the outcome here or not, that's to be decided), I would think it would be better to deprecate a function, instead of removing the function's definition directly as in wp-favicon-request.diff​.

Specifically if the function is not marked as private, i.e. not prefixed with underscore _.

So one could look into the file /wp-includes/deprecated.php for storing the deprecated function's definition, use the _deprecated_function() function and the @deprecated tag for the inline docs.

[SergeyBiryukov]

I think a request to favicon.ico should be handled the same way as we handle robots.txt with do_robots():

• If a physical file exists, do nothing, let the server handle the request.
• Otherwise, serve a fallback icon.

For the latter:

• If there's a Site Icon set in Customizer, we can use that.
• Otherwise, there is a precedent of using wp-admin/images/w-logo-blue.png in the_embed_site_title()​.

In 47398.2.diff​:

• Introduce is_favicon() conditional tag to complement is_robots().
• Introduce do_favicon() function to complement do_robots() and use it in template loader.
• Introduce do_faviconico action to complement do_robotstxt, for plugins to override the default behavior.
• Mark wp_favicon_request() as deprecated.

Thoughts?

[SergeyBiryukov]

Note: Testing the patch requires going to Permalink Settings to flush the rewrite rules.

[SergeyBiryukov]

Replying to SergeyBiryukov:

Note: Testing the patch requires going to Permalink Settings to flush the rewrite rules.

47398.3.diff​ bumps $wp_db_version to flush rewrite rules automatically.

[jonoaldersonwp]

Sounds good!

I'm wondering if there might be an advantage to serving the default favicon by echoing it out in base64 - that might work around issues or rewrite support, installations not in the root, etc?

[SergeyBiryukov]

Replying to jonoaldersonwp:

I'm wondering if there might be an advantage to serving the default favicon by echoing it out in base64 - that might work around issues or rewrite support, installations not in the root, etc?

• If rewrites/permalinks are disabled, I don't think there's a way to catch the request, so serving the icon in base64 probably wouldn't make a difference.
• do_robots() only handles the robots.txt request if installed at the root, see #13115. Unless I'm missing something, the same applies to favicon.ico as well, it shouldn't be relevant for installations not in the root.

[SergeyBiryukov]

Replying to SergeyBiryukov:

In 47398.2.diff​:

• Introduce is_favicon() conditional tag to complement is_robots().
• Introduce do_favicon() function to complement do_robots() and use it in template loader.
• Introduce do_faviconico action to complement do_robotstxt, for plugins to override the default behavior.
• Mark wp_favicon_request() as deprecated.

Checked the Plugin Directory for potential naming conflicts, no matches found. Should be good to go.

[SergeyBiryukov]

#46135 was marked as a duplicate.

[birgire]

47398.3.diff​ could be simplified with the get_site_icon_url() that supports a fallback image.

Users that use Photon from JetPack will have images served from e.g. i2.wp.com.

So the output from wp_get_attachment_image_src( $site_icon_id, array( 32, 32 ) ) would contain a source like:

https://i2.wp.com/example.com/wp-content/uploads/2019/06/image.jpg?fit=32%2C32&ssl=1

but it seems wp_check_filetype() doesn't support query arguments like ?fit=32%2C32&ssl=1

Another thing to consider is that some prefer to set allow_url_fopen=0 in php.ini, for security reasons. I don't recall if WordPress requires allow_url_fopen=1, but it would be informative to check it out.

In the case of allow_url_fopen=0, neither the readfile() or wp_check_filetype() will work for sites with the site icon set (i.e. ​https:// or ​http:// file wrappers), external or not.

I wonder if it would be better to use the file path instead of file url of the site icon. But in that case the fallback favicon would also kick in for external site icons.

[desrosj]

It looks like this one needs a little more work. At the least, @birgire's points above need to be discussed.

With beta 1 in a few hours, I'm going to punt this one.

[jonoaldersonwp]

Can we pick this back up? How do we move forward? :)

[SergeyBiryukov]

Refreshed

[SergeyBiryukov]

47398.5.diff​ simplifies things a bit to address the concerns raised in comment:17:

• Use get_site_icon_url() with a fallback icon, same as in the_embed_site_title()​.
• Perform a 302 redirect to the icon URL, instead of trying to serve it in PHP. That way we don't have to deal with allow_url_fopen limitations, differentiating between local vs. external icons, etc.

[SergeyBiryukov]

In 47018:

Bootstrap/Load: Make handling the /favicon.ico requests more flexible.

Previously, wp_favicon_request() was introduced in [13205] to avoid a performance hit of serving a full 404 page on every favicon request.

While working as intended, that implementation did not provide a way for theme or plugin authors to manage the behavior of favicon requests.

This changeset implements the following logic (only applied if WordPress is installed in the root directory):

• If there is a Site Icon set in Customizer, redirect /favicon.ico requests to that icon.
• Otherwise, use the WordPress logo as a default icon.
• If a physical /favicon.ico file exists, do nothing, let the server handle the request.

Handling /favicon.ico is now more consistent with handling /robots.txt requests.

New functions and hooks:

• Introduce is_favicon() conditional tag to complement is_robots().
• Introduce do_favicon action to complement do_robots and use it in template loader.
• Introduce do_favicon() function, hooked to the above action by default, to complement do_robots().
• Introduce do_faviconico action to complement do_robotstxt, for plugins to override the default behavior.
• Mark wp_favicon_request() as deprecated in favor of do_favicon().

Props jonoaldersonwp, birgire, joostdevalk, mukesh27, SergeyBiryukov.
Fixes #47398.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2020/02/19/enhancements-to-favicon-handling-in-wordpress-5-4/

[keepmovingdk]

I just read the dev notes on make.wordpress.org, and I wonder why it was decided to use the WordPress icon as a default fallback icon?

I think site owners will be surprised to suddenly see the WordPress logo show up for their website. Also, it might be confusing for visitors to such sites to see the WordPress' logo show up (especially since they might also show up in Google search results, as described in the introduction to the issue).

Is it considered a good idea to so publicly display to visitors that a site is running WordPress?

I know that I can use the new filters to reset the favicon to a blank file for any of my client sites that don't have a favicon. But I wanted to bring this up to make sure that the consequences of this change has been thoroughly considered for all the site owners out there without favicons who might not have the technical skills or knowledge to do so.

Thanks.

[SergeyBiryukov]

Replying to keepmovingdk:

I just read the dev notes on make.wordpress.org, and I wonder why it was decided to use the WordPress icon as a default fallback icon?

Thanks for bringing this up! As noted in comment:10, this was simply following an existing pattern used in the_embed_site_title(), admittedly taking it further.

Is it considered a good idea to so publicly display to visitors that a site is running WordPress?

The fact of running WordPress is generally obvious by looking at the page source, unless it's heavily customized, in which case the new hooks added in [47018] can be used to override the favicon, or a physical file can be created.

Personally, I like the "Ferrari analogy" from ​Don’t Hide the Fact That You’re Using WordPress by @kovshenin:

Sometimes people try hide the fact that they’re running WordPress because they’re afraid other humans will spot that and think they’re “unprofessional” or cheap. Well WordPress is the most professional content management system known to human kind, trusted by ​some of the largest companies worldwide and although free and open source, ​certainly not cheap.

When you buy yourself a new Ferrari, do you remove the Ferrari logos before showing it to your friends? No. Although if you did, it would still be obvious.

[keepmovingdk]

Replying to @SergeyBiryukov:

Thank you for elaborating and for sharing your thoughts.

From my own perspective I'm proud to use WordPress as a platform, and I do realize that it's easy for people with even a tiny bit of technical insight to see that a site has been implemented using WordPress.

But I think that exposing the WP logo as favicon is taking it too far. Because it becomes *very* visible.

I would be less concerned if it only affected new installations. Because people then would have a chance to notice the favicon while building the website - and then finding out how to replace it if they don't like it. But this change will affect all sites upgraded to WP 5.4 (and in such situations, the developer and/or site owner might not immediately be aware).

And yes, I do realize that only a very small number of company websites don't have a favicon.

BTW, what is the recommended, minimal amount of code to add a hook for outputting the current blank favicon if there is no site icon? My guess is that it would be to hook into 'do_faviconico' and redirect to the blank favicon. Does that sound correct to you?

Thanks again.

[pipdig]

Replying to keepmovingdk:

Replying to @SergeyBiryukov:

BTW, what is the recommended, minimal amount of code to add a hook for outputting the current blank favicon if there is no site icon? My guess is that it would be to hook into 'do_faviconico' and redirect to the blank favicon. Does that sound correct to you?

if (function_exists('do_favicon')) {
	add_action('do_faviconico', function() {
		if (has_site_icon()) {
			wp_redirect(get_site_icon_url(32));
		}
		die;
	});
}