WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34339 closed defect (bug) (fixed)

Default option for site_icon

Reported by: sebastian.pisula Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Options, Meta APIs Keywords: has-patch commit
Focuses: Cc:

Description

If Default Icon is not defined then insert default option.

Attachments (7)

34339.patch (890 bytes) - added by sebastian.pisula 4 years ago.
34339.2.patch (707 bytes) - added by swissspidy 4 years ago.
34339.3.patch (469 bytes) - added by sebastian.pisula 4 years ago.
34339.4.patch (473 bytes) - added by sebastian.pisula 4 years ago.
34339.5.patch (465 bytes) - added by sebastian.pisula 4 years ago.
34339_6.patch (450 bytes) - added by sebastian.pisula 4 years ago.
34339.7.diff (322 bytes) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (16)

#1 @sebastian.pisula
4 years ago

if option not exists then wordpress send request to mysql all the time:

SELECT option_value FROM wp_options WHERE option_name = 'site_icon' LIMIT 1

in my opinion unnecessary query

#2 @swissspidy
4 years ago

  • Keywords reporter-feedback added

Can you explain why this should be needed? update_blog_option would result in an additional, unneeded database query just so that there's a 0.

If you want a different default value, we could pass that as the second argument to get_option.

Also, why is SELECT option_value FROM wp_options WHERE option_name = 'site_icon' LIMIT 1 unnecessary? I mean, somehow we need to get that option from the database…

#3 @johnbillion
4 years ago

  • Keywords has-patch needs-testing added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release

This is valid. Same problem as #26876.

@swissspidy
4 years ago

#4 @swissspidy
4 years ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 4.4

34339.2.patch adds little coding standards improvements.

Works great so far.

#5 follow-up: @ocean90
4 years ago

  • Type changed from enhancement to defect (bug)
  • Version set to 4.3

Shouldn't site_icon be added to populate_options()?

#6 in reply to: ↑ 5 @swissspidy
4 years ago

  • Keywords needs-refresh added

Replying to ocean90:

Shouldn't site_icon be added to populate_options()?

D'oh, that makes more sense :)

@sebastian.pisula: Thanks for the updated patch! Note that we use 4.4.0 instead of 4.4 for docs. Also, you mixed up the indentation in your patch (spaces vs. tabs).

Ps. Trac doesn't send notifications for new attachments. I usually write something afterwards to notify people.

#7 @obenland
4 years ago

Since it holds a post id, it should probably be an int. We can also just add it to the 4.3 part, it was introduced back then.

@swissspidy
4 years ago

#8 @swissspidy
4 years ago

  • Keywords commit added; needs-refresh removed

34339.7.diff makes 0 the default.

#9 @wonderboymusic
4 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 35312:

Site Icon: make the default value for the site_icon option 0 in schema.php.

Props swissspidy, sebastian.pisula.
Fixes #34339.

Note: See TracTickets for help on using tickets.