Unexpected behavior when switching autoload between update_option

[pentatonicfunk]

Autoload update, gives unexpected behavior. Replicated with this flow :

1. Update x option to value "A" with autoload = yes, all good here. DB updated, and alloptions cache updated.
2. Update x option to value = "B" with autoload = no. Function in effect update_option
   • Expected behavior :
     • Its should $wpdb->update
     • Its should delete it from alloptions cache,
     • Its should update "x" cache options
   • Actual behavior :
     • Its succesfully execute $wpdb->update ( correct )
     • Its updating alloptions cache, even the "autoload" is set as no
     • Its not updating "x" cache options
3. delete x option. Function in effect delete_option
   • Expected behavior :
     • Its should $wpdb->delete
     • Its should delete "x" cache options ( since autoload = no )
   • Actual behavior :
     • Its succesfully execute $wpdb->delete -- correct
     • Its deleting "x" cache options ( since autoload = no ) -- correct
     • alloptions cache still has x option, because of (2) behavior
4. get x option. Function in effect get_option
   • Expected behavior :
     • It should returns FALSE since its already deleted on (3)
   • Actual behavior :
     • It returns "B", because get_option prioritze wp_load_alloptions, and alloptions cache is still exists for x, see (2) and (3)
5. update x option to value "C" with autoload = no
   • Expected behavior :
     • Its should calls add_option, because its already deleted on (3)
     • Its should delete it from alloptions cache,
     • Its should update "x" cache options
   • Actual behavior :
     • Its not executing add_option, since update_option check old value by using get_option which returning "B" at the moment, so it will attempting to $wpdb->update instead, which eventually will fail, since its DB entry alreeady deleted on (3)
     • Its not deleting alloptions cache, because DB update fails
     • Its not updating "x" cache options, because DB update fails

Test code :

<?php
// just to make sure we use unique option name for each variant, so it won't interfering
$option_name = "x_" . wp_generate_uuid4();

error_log( "###VARIANT #1 : switch autoload from yes to no---" );//phpcs:ignore
/**
 * #1
 */
error_log( "---UPDATING TO A, AUTOLOAD = YES---" );//phpcs:ignore
$success = update_option( $option_name, "A", "yes" );
error_log( "Should updated successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
error_log( "Should return : 'A', Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

/**
 * #2
 */
error_log( "\n---UPDATING TO B, AUTOLOAD = NO---" );//phpcs:ignore
$success = update_option( $option_name, "B", "no" );
error_log( "Should updated successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
error_log( "Should return : 'B', Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

/**
 * #3
 */
error_log( "\n---DELETING---" );//phpcs:ignore
$success = delete_option( $option_name );
error_log( "Should deleted successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
/**
 * #4
 */
error_log( "Should return : false, Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

/**
 * #5
 */
error_log( "\n---UPDATING TO C, AUTOLOAD = NO---" );//phpcs:ignore
$success = update_option( $option_name, "C" );
error_log( "Should updated successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
/**
 * #4
 */
error_log( "Should return : 'C', Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore


// just to make sure we use unique option name for each test variant, so it won't interfering
$option_name = "x_" . wp_generate_uuid4();
error_log( "\n-----------------------" );//phpcs:ignore
error_log( "\n###VARIANT #2: switch autoload from no to yes---" );//phpcs:ignore
/**
 * #1
 */
error_log( "---UPDATING TO A, AUTOLOAD = NO---" );//phpcs:ignore
$success = update_option( $option_name, "A", "no" );
error_log( "Should updated successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
error_log( "Should return : 'A', Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

/**
 * #2
 */
error_log( "\n---UPDATING TO B, AUTOLOAD = YES---" );//phpcs:ignore
$success = update_option( $option_name, "B", "yes" );
error_log( "Should updated successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
error_log( "Should return : 'B', Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

/**
 * #3
 */
error_log( "\n---DELETING---" );//phpcs:ignore
$success = delete_option( $option_name );
error_log( "Should deleted successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
/**
 * #4
 */
error_log( "Should return : false, Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

/**
 * #5
 */
error_log( "\n---UPDATING TO C, AUTOLOAD = YES---" );//phpcs:ignore
$success = update_option( $option_name, "C", "yes" );
error_log( "Should updated successfully : YES, Actual result : " . ( $success ? "YES" : "NO" ) );// phpcs:ignore
/**
 * #4
 */
error_log( "Should return : 'C', Actual Return : " . var_export( get_option( $option_name ), true ) );// phpcs:ignore

Changes :

• Use autoload flag from function argument, or fallback to previous autoload value.
• Invalidates old cache and update cache based on current autoload.

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

[SergeyBiryukov]

Related: #51372

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

[kkmuffme]

Just to get this rolling again, since many related issues have actually already been fixed.

The issue above is in 2) only (which is what the existing PR fixes): the issue is basically that "autoload" value is not updated in cache, which then causes non-autoloaded options to be incorrectly cached and vice versa.

However the PR has some performance issues, so I created a new PR on trunk on the latest version.

The PR fixes the current issue + probably #51372 (this was 99% fixed already though in the current WP version) + probably #38931.

This really is a major issue that should get merged with WP 6.3. Could you @SergeyBiryukov be so kind and let me know what is needed for my PR to get merged?

[SergeyBiryukov]

Nice, thanks for the PR! Moving to 6.3 for review.

[azaozz]

Actually it is not a good idea to be able to change autoload when updating an option. Not sure why it was made possible. The intent in #26394 where this was added was only to use autoload when adding new options, not when updating existing.

As far as I see autoload is a low level DB setting that was intended to be set once when adding a new option and never changed. Can't see a point in changing it on every update of the option.

Seems a good fix for this ticket would be to deprecate and remove the possibility to change autoload when updating options. As update_option() can also be used to add new options, the $autoload = null parameter would need to stay, but it would be used only when redirecting to add_option().

If plugins really needs to change autoload, they can delete the old option and then add it again. This should handle caching, etc. properly.

[oglekler]

Due to comment above, I am moving this ticket to 6.4 for further work.

Let's also consider if this will be an enhancement and not a bug fix.

[kkmuffme]

Autoload can definitely be removed in "update", but that would be an enhancement as @oglekler says.

However the current issue itself is a bug and should definitely land in 6.3 with the PR above - since this is a major issue.
And the enhancement can be taken care of in a separate PR for 6.4 then. I suggest to actually put it in a separate ticket then though.

[oglekler]

This ticket was discussed during bug scrub. It is not ready for testing because it is not clear what we are fixing or improving and what the solution should look like.

@spacedmonkey I see that you are watching this ticket. What do you think?

[kkmuffme]

@oglekler the OP clearly outlines how it can be tested/reproduced and what the current and expected behavior is?

The PR https://core.trac.wordpress.org/ticket/51352#comment:3 fixes this exact issue and can be tested with what is described in the OP of the ticket.

Could you please clarify what exactly is not clear? As I'm happy to clear it up and provide what is necessary to get this merged.

[spacedmonkey]

No clear path forward on this one. I am working on other ticket related to autoloaded options. This maybe fixed elsewhere.

[kkmuffme]

@spacedmonkey In how far?

There is a patch https://core.trac.wordpress.org/ticket/51352#comment:3 that fixes this issue.
Merge it and mark the ticket as done.

Or is there a problem with the PR?

[nicolefurlan]

This ticket was discussed during the 6.4 bug scrub today.

Per @spacedmonkey, new core functions will help with this issue: ​https://github.com/WordPress/wordpress-develop/commit/a22fee0e5a6c01d4b8dc32e1763783d4a6b3df2b

Removing the version as this ticket is 3 years old, so 6.3 is not valid. We can update that once the version is discovered.

Also changing the severity to normal.

Dropping this in #core-performance to get feedback on @azaozz's #comment:8. @johnjamesjacoby we'd love your input on that comment well.

Thank you for weighing in on this @hellofromTonya!

[joemcgill]

Thanks @nicolefurlan. Reading through this conversation, this looks like a well described bug with initial unit tests from the first PR that was added, but with a second PR that only includes a fix (not the tests). The PR needs a refresh that includes tests and someone to review/test.

Given that we're explicitly adding functionality in this release via #58964 to allow autoload values to be changed, it seems sensible to also make sure caches are being properly cleared to avoid returning stale values when calling get_option(), which is my understanding of this bug.

@kkmuffme if you (or someone else) has time to update the PR we can try to get this in, but really needs to land before Oct 10 so it's in the final beta of this release. Otherwise, this can continue in a future release.

[flixos90]

Regardless of whether we want to keep the functionality of updating the autoload value of an option during an update (related is #48393, which is another problem with doing that), what is described in this ticket is clearly a bug.

I just left a review on ​https://github.com/WordPress/wordpress-develop/pull/4494, the approach looks solid to me and as @joemcgill said the only thing missing there are tests.

If we can get this completed in time, I'd be happy to commit before 6.4 Beta 3.

[kkmuffme]

@joemcgill @flixos90 my PR is done

For tests, we could cherry-pick those of the first PR ​https://github.com/WordPress/wordpress-develop/pull/541/commits/f81720085010a8c87a8e5116c190bbd7c467cfb2 (since I don't want to copy/steal other people's work; if cherry-picking doesn't work for whatever reason, then I can just copy and commit it to my PR, just let me know what attribution is needed)

[joemcgill]

@kkmuffme cherry-picking is fine. We just need a single PR that we can run in CI and review that included the entire change that will be committed.

[flixos90]

@kkmuffme Can you copy the changes to your PR? No need to worry about the attribution on your end: Once this gets committed, both you and @pentatonicfunk will receive props for it. :)

[flixos90]

In 56796:

Options, Meta APIs: Fix bug with update_option() updating the wrong cache, leading to potentially stale values being returned.

When using the $autoload parameter of update_option() alongside an option value update, prior to this changeset the function would update the incorrect cache, not respecting the new autoload value. This could have severe implications such as returning a stale option value when the option in fact had already been deleted.

This changeset fixes the bug alongside test coverage that failed with trunk but now passes.

Props kkmuffme, pentatonicfunk, SergeyBiryukov, oglekler, azaozz, spacedmonkey, nicolefurlan, joemcgill, flixos90.
Fixes #51352.

[flixos90]

This doesn't need a dev note per se, but since I am already working on a dev note covering #58962 and #58964, both of which are about autoloading options, it makes sense to briefly mention this fix there as well.

Committed in https://core.trac.wordpress.org/changeset/56796

[flixos90]

Dev note published: ​https://make.wordpress.org/core/2023/10/17/new-option-functions-in-6-4/