WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#33326 closed defect (bug) (fixed)

Site Icon: Check return of wp_get_attachment_image_src in get_site_icon_url

Reported by: MikeHansenMe Owned by: obenland
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

In get_site_icon_url it uses wp_get_attachment_image_src which can return false. So we can check for that before assigning $url = $url_data[0];

Also in has_site_icon we are using !! which we have never used in core. We can just use (bool) instead.

Attachments (3)

site-icon-check-bool.diff​ (729 bytes) - added by MikeHansenMe 6 years ago.
33326.diff​ (658 bytes) - added by MikeHansenMe 6 years ago.
33326.2.diff​ (1.1 KB) - added by obenland 6 years ago.

Download all attachments as: .zip

Change History (16)

@MikeHansenMe
6 years ago

#1 @obenland
6 years ago

  • Component changed from General to Customize
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.3
  • Owner set to obenland
  • Status changed from new to accepted

Looks good.

This ticket was mentioned in ​Slack in #core by obenland. ​View the logs.
6 years ago

#3 @swissspidy
6 years ago

  • Keywords needs-refresh added

It's considered bad practice to have assignments in an if condition. Prohibited by the WP coding standards.

@MikeHansenMe
6 years ago

#4 @obenland
6 years ago

  • Keywords needs-refresh removed

#5 @helen
6 years ago

  • Keywords commit added

Looks good.

#6 follow-up: @azaozz
6 years ago

33326.diff looks good. However, why do we have if ( function_exists( 'get_blog_option' ) ) at the top of get_site_icon_url()? Shouldn't that be is_multisite()?

Generally function_exists() is slow, we don't want it running on every front-end page load if possible.

Last edited 6 years ago by azaozz (previous) (diff)

#7 in reply to: ↑ 6 @obenland
6 years ago

Replying to azaozz:

why do we have if ( function_exists( 'get_blog_option' ) ) at the top of get_site_icon_url()? Shouldn't that be is_multisite()?

I was wondering about that too. I think it should. 33326.2.diff​ simplifies that a bit, checking for $blog_id first and switching to is_multisite().

@obenland
6 years ago

#8 follow-up: @azaozz
6 years ago

33326.2.diff looks good. It changes the behavior a little. On multisite when $blog_id is not set, get_site_icon_url() will use get_option() instead of getting the current blog_id and using get_blog_option().

#9 in reply to: ↑ 8 ; follow-up: @obenland
6 years ago

Replying to azaozz:

It changes the behavior a little. On multisite when $blog_id is not set, get_site_icon_url() will use get_option() instead of getting the current blog_id and using get_blog_option().

Correct, see ​Slack conversation.

#10 in reply to: ↑ 9 ; follow-up: @mrahmadawais
6 years ago

  • Keywords ux-feedback added

Replying to obenland:

By the way, the site icon can be more optimized to support more platforms, let me know if I can create a patch.

Here is what my optimized fav icon's code looks like in a web project.

<link rel="apple-touch-icon" sizes="57x57" href="/images/apple-touch-icon-57x57.png">
<link rel="apple-touch-icon" sizes="60x60" href="/images/apple-touch-icon-60x60.png">
<link rel="apple-touch-icon" sizes="72x72" href="/images/apple-touch-icon-72x72.png">
<link rel="apple-touch-icon" sizes="76x76" href="/images/apple-touch-icon-76x76.png">
<link rel="apple-touch-icon" sizes="114x114" href="/images/apple-touch-icon-114x114.png">
<link rel="apple-touch-icon" sizes="120x120" href="/images/apple-touch-icon-120x120.png">
<link rel="apple-touch-icon" sizes="144x144" href="/images/apple-touch-icon-144x144.png">
<link rel="apple-touch-icon" sizes="152x152" href="/images/apple-touch-icon-152x152.png">
<link rel="apple-touch-icon" sizes="180x180" href="/images/apple-touch-icon-180x180.png">
<link rel="icon" type="image/png" href="/images/favicon-32x32.png" sizes="32x32">
<link rel="icon" type="image/png" href="/images/favicon-194x194.png" sizes="194x194">
<link rel="icon" type="image/png" href="/images/favicon-96x96.png" sizes="96x96">
<link rel="icon" type="image/png" href="/images/android-chrome-192x192.png" sizes="192x192">
<link rel="icon" type="image/png" href="/images/favicon-16x16.png" sizes="16x16">
<link rel="manifest" href="/images/manifest.json">
<meta name="msapplication-TileColor" content="#2c2c2c">
<meta name="msapplication-TileImage" content="/images/mstile-144x144.png">
<meta name="theme-color" content="#ffffff">

This supports iOS and Android Bookmarks as well as Win 8 Tiles.

Compatible with:

  • Windows (IE, Chrome, Firefox, Opera, Safari)
  • Mac (Safari, Chrome, Firefox, Opera, Camino)
  • iOS (Safari, Chrome, Coast)
  • Android (Chrome, Firefox)
  • Surface (IE)

Looking forward.

#11 @MikeHansenMe
6 years ago

  • Keywords ux-feedback removed

@mrahmadawais thanks for being willing to write a patch. Can you create a new ticket with this request since it is not directly relate to this ticket?

#12 in reply to: ↑ 10 @obenland
6 years ago

Replying to mrahmadawais:

the site icon can be more optimized to support more platforms

Limiting the output to the most commonly used sizes was intentional and I doubt we'll be adding more any time soon. There are hooks that let plugin authors add sizes that they deem necessary, as outlined in ​this make/core post.

#13 @obenland
6 years ago

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

In 33606:

Site Icon: Improvements to Site Icon API.

  • Only call get_blog_option() when there is a blog id and we're in Mulitsite. If there is no blog id the request is for the current blog.
  • Check return value of wp_get_attachment_image_src() before getting the URL since it could be false.
  • Use {bool} rather than !! to return a boolean value.

Props MikeHansenMe, obenland.
Fixes #33326.

Note: See TracTickets for help on using tickets.