Add param for atomic updating of options and distinct wrapper function #6057
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
kkmuffme
force-pushed
the
update-option-atomic
branch
from
eb65036
to
d5ec205
Compare
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
kkmuffme
force-pushed
the
update-option-atomic
branch
from
d5ec205
to
443543c
Compare
| * @param string|bool $autoload Optional. @see update_option(). Default null. | ||
| * @return bool True if the value was updated, false otherwise. | ||
| */ | ||
| function update_option_atomic( $option, $value, $old_value, $autoload = null ) { |
There was a problem hiding this comment.
I'm -1 for adding such a new wrapper function, it's not consistent with other APIs like the metadata API for example.
I'd suggest focusing on the new param on update_option for now
There was a problem hiding this comment.
Fine for me 👍
The long term goal was to eventually deprecated update_option by replacing it with the new atomic one, since this was an architectural oversight (or non-issue when WP was mostly serving small sites).
| @@ -88,4 +88,40 @@ public function test_get_options_with_nonexistent_options() { | |||
|
|
|||
| $this->assertFalse( $options['nonexistent_option'], 'nonexistent_option is present in option.' ); | |||
| } | |||
|
|
|||
| /** | |||
| * @covers update_option_atomic | |||
There was a problem hiding this comment.
| * @covers update_option_atomic | |
| * @covers ::update_option | |
| * @ticket 59246 |
| $this->assertTrue( update_option_atomic( $option, 'fourth', $old_value ) ); | ||
| } | ||
|
|
||
| public function data_options_atomic() { |
There was a problem hiding this comment.
Tip: add array keys to describe the scenario
That said, is the autoload value really relevant for this test?
Seems like we don't really need the data provider.
There was a problem hiding this comment.
That said, is the autoload value really relevant for this test?
Yes, since options & autoload options behave differently in the way they're cached in WP current request ($alloptions)
(as well as in how it works with caches).
This issue came up when we found and fixed https://core.trac.wordpress.org/ticket/31245 (and related), so it should be kept just to be sure.
Trac ticket: https://core.trac.wordpress.org/ticket/59246