WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#33325 closed task (blessed) (fixed)

Rearrange function params in Site Icon API

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

Description

Having to pass null as the first parameter if a site icon for the current blog is requested is not very elegant. Let's put it at the end, as it's even more likely that a fallback URL will be passed that a blog id.

Attachments (2)

33325.diff (3.6 KB) - added by obenland 5 years ago.
33325.2.diff (2.7 KB) - added by obenland 5 years ago.

Download all attachments as: .zip

Change History (12)

@obenland
5 years ago

#1 @obenland
5 years ago

  • Owner set to obenland
  • Status changed from new to accepted

#2 @DrewAPicture
5 years ago

I agree that having to pass null as a first parameter is not awesome.

+1 for 33325.diff.

This ticket was mentioned in Slack in #core by obenland. View the logs.


5 years ago

#4 @azaozz
5 years ago

  • Keywords commit added

33325.diff looks good.

#5 @helen
5 years ago

  • Keywords commit removed

We don't use !! anywhere else in PHP in core - we cast to bool, which is more readable anyway. Since we're in here, let's change that as well.

#6 @MikeHansenMe
5 years ago

#33326 takes care of the !! and is dependent on this ticket.

This ticket was mentioned in Slack in #core by obenland. View the logs.


5 years ago

#8 @helen
5 years ago

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

In 33605:

Site icon: Rearrange function parameters to avoid frequently passing empty values.

props obenland.
fixes #33325.

#9 @jorbin
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

https://travis-ci.org/aaronjorbin/develop.wordpress/builds/74978783

Either the tests also need to be updated, or this had an unintended change.

@obenland
5 years ago

#10 @obenland
5 years ago

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

In 33607:

Tests: Update Site Icon tests to account for changes in [33605].

H/t jorbin.

Fixes #33325.

Note: See TracTickets for help on using tickets.