WordPress.org

Make WordPress Core

Opened 8 weeks ago

Last modified 6 weeks ago

#47398 reviewing enhancement

Remove wp_favicon_request

Reported by: jonoaldersonwp Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch seo commit
Focuses: Cc:

Description

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

Attachments (4)

47398.diff (616 bytes) - added by mukesh27 8 weeks ago.
Patch that remove wp_favicon_request function from wp-includes/load.php
wp-favicon-request.diff (1.1 KB) - added by joostdevalk 8 weeks ago.
Patch
47398.2.diff (10.7 KB) - added by SergeyBiryukov 8 weeks ago.
47398.3.diff (11.0 KB) - added by SergeyBiryukov 8 weeks ago.

Download all attachments as: .zip

Change History (21)

@mukesh27
8 weeks ago

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

#1 @mukesh27
8 weeks ago

  • Keywords has-patch added

#2 @SergeyBiryukov
8 weeks ago

  • Milestone changed from Awaiting Review to 5.3

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?

#3 @SergeyBiryukov
8 weeks ago

  • Component changed from Gallery to Bootstrap/Load

#4 @jonoaldersonwp
8 weeks ago

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.

#5 @joostdevalk
8 weeks ago

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.

#6 @SergeyBiryukov
8 weeks ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @ocean90
8 weeks ago

  • Keywords close added
  • Milestone changed from 5.3 to Awaiting Review

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.

#8 @joostdevalk
8 weeks ago

  • Keywords close removed

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

#9 @birgire
8 weeks ago

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.

#10 follow-up: @SergeyBiryukov
8 weeks ago

  • Milestone changed from Awaiting Review to 5.3

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 precedence 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?

#11 follow-up: @SergeyBiryukov
8 weeks ago

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

#12 in reply to: ↑ 11 @SergeyBiryukov
8 weeks ago

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.

#13 follow-up: @jonoaldersonwp
8 weeks ago

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?

#14 in reply to: ↑ 13 @SergeyBiryukov
8 weeks ago

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.

#15 in reply to: ↑ 10 @SergeyBiryukov
7 weeks ago

  • Keywords commit added

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.

#16 @SergeyBiryukov
6 weeks ago

#46135 was marked as a duplicate.

#17 @birgire
6 weeks ago

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.

Note: See TracTickets for help on using tickets.