Null values for Update Options does not reset in $all_options

[MikeNGarrett]

This is a fun, extremely niche case.

We were using the Settings API to save out options for a plugin when we came across a really frustrating scenario. Half the time our setting was returning values that were not accurate to the setting that was stored in the database and in cache.

Come to find, the value being returned from wp_load_alloptions was incorrect while every other instance was correct. In our case, these values were in the database, memcache, and the single value in wp-object-cache.

On line 319 of option.php, we're using isset to check the value in the array. This returns false if the value is NULL. This leads to all kinds of sadness.

I would propose we instead use array_key_exists here to ensure we're getting true if the key is present in the array at all.

[MikeNGarrett]

Patch #1

[MikeNGarrett]

Co-contributor: @tndavide, @jipmoors

[MikeNGarrett]

I ran back through and realized there were quite a few places here that are using isset for assessing array keys existence. Here's a patch to switch all isset to array_key_exists. Next step, let's create some unit tests.

[MikeNGarrett]

Patch 2 - replace all isset in option.php

[MikeNGarrett]

This is still present, but it's a very narrow and uncommon case. I would be ok closing this.

https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/option.php#L391

[pbearne]

isset is faster than array_key_exist ​https://stackoverflow.com/questions/3210935/whats-the-difference-between-isset-and-array-key-exists

we could do both which is only a little slower

isset()

array_key_exists()

Thoughts

[MikeNGarrett]

It's been a minute, so let me wrap my head around this.

The original issue is that in the case of $options = array('test' => null); that isset($options['test']) returns false, leading to differences in the structure of the options array.

I just checked out 6.5.2 (https://core.trac.wordpress.org/browser/tags/6.5.2/src/wp-includes/option.php) and it looks like this issue still exists.

I think adding a secondary check for array_key_exists() after isset() would work wonderfully without slowing down execution in most circumstances.

The existing condition logic for option checks only uses the 'isset' function. This commit adds additional checks using 'array_key_exists' alongside 'isset'. This helps to handle edge cases more accurately where the array key exists but its value is null.

[pbearne]

I have refreshed the patch

[pbearne]

TODO: Add test for null value

[pbearne]

I looked at the code, and I could not find a way to check null in the functions, but I was able to confirm that the code change worked with this test.

<?php
/**
 * @ticket 32848
 *
 * Test that $options = array('test' => null); works.
 */
public function test_for_null() {
        $option = 'foo';
        $this->assertTrue( add_option( $option, null, '',true  ) );

        $alloptions = wp_load_alloptions();
        $this->assertFalse( isset( $alloptions[ $option ] ) );
        $this->assertTrue( isset( $alloptions[ $option ] ) || array_key_exists( $option, $alloptions ) );
  }

I can level this test in but it doesn't test function rather a fragment of code so it doesn't feel right

Paul

[peterwilsoncc]

Related #22192.

This appears to be due to how add_option() caches data compared to how it is stored in the database. When the data is originally cached it uses the data type as passed to the function. When stored in the database, the data is type cast to a string.

Without a persistent cache, subsequent requests to load alloptions will result in isset() === true as the value will be returned as an empty string. To demonstrate via WP CLI:

vagrant@wp-dev:/vagrant$ wp shell
wp> add_option( '6766-testing', null );
=> 
bool(true)
wp> wp_load_alloptions()['6766-testing']
=> 
NULL
wp> isset( wp_load_alloptions()['6766-testing'] )
=> 
bool(false)
wp> exit


vagrant@wp-dev:/vagrant$ wp shell
wp> wp_load_alloptions()['6766-testing']
=> 
string(0) ""
wp> isset( wp_load_alloptions()['6766-testing'] )
=> 
bool(true)
wp> exit

I suspect the best option might be to ensure that options are cached in the same form that they are stored (ie, as a string). This would ensure that the behavior of get_option() doesn't randomly change depending on whether the cache is populated or unpopulated.

Although some work along these lines in #22192 proved to be risky and was ultimately reverted.

(Mike and Paul: I know you're probably aware of the cause but best to spell it out.)

[MikeNGarrett]

I've opened and closed a business, changed cities, bought a house, started a family, and gone through a pandemic since I opened this ticket.

...but for some reason I still remember the details of this ticket.

Yes, the issue is that null values in an array are cast to empty strings when being stored in the database. In cache, they are correctly stored as null.

This casts the cached value of options to a string in update_option(), update_network_option(), add_option() and add_network_option().

Currently the cached value of an option can be in either of the original type or a string, depending on whether the cache was primed after a database query in get_(network)_option() (always a string) or primed while updating or adding an option (type as passed to the function).

The effect of this is that the result of get_(network)_option() can be inconsistent depending on whether the cache was primed or required a database query.

This change ensures that the options always use the same type (a string) regardless of how the cache is primed.

Risks

Yes.

On hosting providers that prevent the options cache/all options cache from being flushed the type would consistently be as set by the code while adding or updating the value. Following this change, that will no longer be the case.

Trac ticket: https://core.trac.wordpress.org/ticket/32848

[flixos90]

@peterwilsoncc I left some feedback on your draft PR ​https://github.com/WordPress/wordpress-develop/pull/7133.

While the ideal solution is possibly along the lines of what you're saying, I don't think this is realistic to implement given all the BC breakage side effects it would have (IMO even more severe than the problems we found in #22192).

I would prefer if we narrowed the solution to this ticket to address the more specific problem with null values and wp_load_alloptions() - which doesn't address the underlying problem, but I think is a more reasonable way to fix what this ticket is about.

Aside, I'll remove the performance focus, since this ticket is not about performance, but about a general bug in Core.

I am concerned about this change, mostly because it changes the return type of the option retrieval functions, which I think is notable backward compatibility breakage. The few get_option() instances you have to update in this PR alone indicate that.

I've pushed some tests to demonstrate the issue I'm trying to resolve. The return type for the options getters is currently inconsistent depending on whether the cache is warm or cold. See the ​action run after pushing the test changes only.

Anything that uses get_option() === for non-string values without typecasting is probably buggy unless there is type casting on the option_{$option} filter.

The tests are showing that that's still the case in some places so I will look at those and come back to the rest of your feedback and suggestions.

[peterwilsoncc]

I'm moving this off the release as it's unlikely a fix will be committed prior to the final 6.7 beta in just under 24 hours.

[audrasjb]

@peterwilsoncc do you think this is going to be ready for 6.8 beta 1? If not, it's probably better to move it to Future Release (or 6.9).

[audrasjb]

Moving to Future Release