WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18422 closed defect (bug) (fixed)

Oddities in add(_site)_option

Reported by: duck_ Owned by: duck_
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

[13139] made a number of changes to standardise the the values returned by the options API. However add_option() will still return null when the option already exists. This should be changed to return false and the @return docs updated.

I then also spotted a difference in the behaviour of add_option() vs. add_site_option(). add_option() will just stop if the option already exists, see above, but add_site_option() will call update_site_option().

On singlesite installs add_site_option() is just a wrapper for add_option() so calling add_site_option() has different behaviour on multisite vs. singlesite.

Attachments (3)

18422.diff (988 bytes) - added by duck_ 4 years ago.
return false in add_option
18422.add-shouldnt-update.diff (581 bytes) - added by duck_ 4 years ago.
18422.2.diff (943 bytes) - added by ryan 4 years ago.
1 =>true

Download all attachments as: .zip

Change History (17)

@duck_4 years ago

return false in add_option

comment:1 @duck_4 years ago

Patch attached for always returning a boolean from add_option().

comment:2 @nacin4 years ago

  • Milestone changed from Awaiting Review to 3.3

add_site_option() will call update_site_option().

That is terrible.

I'm good on 18422.diff.

comment:3 @edwardw4 years ago

  • Keywords has-patch added

comment:4 @duck_4 years ago

In [18567]:

add_option() should always return a boolean, see #18422

comment:5 follow-up: @duck_4 years ago

Patch attached for the 2nd part of the ticket. add_site_option() shouldn't update_site_option() if it already exists. Thoughts please, aye or nay?

comment:6 in reply to: ↑ 5 ; follow-up: @edwardw4 years ago

Replying to duck_:

Patch attached for the 2nd part of the ticket. add_site_option() shouldn't update_site_option() if it already exists. Thoughts please, aye or nay?

According to the Codex it says it should overwrite the existing value.

comment:7 in reply to: ↑ 6 @duck_4 years ago

Replying to edwardw:

According to the Codex it says it should overwrite the existing value.

Thanks. Though it doesn't say it should rather that it will, this is specifically noted as a warning which also mentions that this behaviour is contrary to that of add_option().

comment:9 @ryan4 years ago

I'm fine with 18422.add-shouldnt-update.diff

comment:10 @ryan4 years ago

Also, add_site_option() returns 1 instead of true when in multisite mode.

comment:11 @nacin4 years ago

Likewise. A note in the phpdoc, codex, and wpdevel would be prudent, as would doing this now rather than later.

Would also be okay with 1 => true.

@ryan4 years ago

1 =>true

comment:12 @ryan4 years ago

That returns true instead of 1 and incorporates 18422.add-shouldnt-update.diff. With that patch the unit tests run clean against multisite.

comment:13 @duck_4 years ago

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

In [18587]:

add_site_option should not update existing options, should return a boolean and should only run actions on success. fixes #18422

comment:14 @duck_4 years ago

Also made a quick modification to the codex to indicate that this change was made.

Note: See TracTickets for help on using tickets.