`wp_prime_option_caches()` makes DB queries for known non-existent options (notoptions).

[peterwilsoncc]

The function wp_prime_option_caches() is intended to reduce the number of database queries by allowing the caching of options in bulk.

When determining which options to query for, it doesn't check the notoptions cache so re-queries the database for options that are known not to exist.

To reproduce in wp shell

wp> get_option( 'nonexistent_option' );
=> bool(false)
$initial_num_queries = get_num_queries();
=> int(10) ### Will differ depending on set up
wp> wp_prime_option_caches( array( 'nonexistent_option' ) );
=> NULL
wp> get_num_queries() - $initial_num_queries;
=> int(1)

A diff is incoming that resolves the issue, to avoid unnecessary requests to the cache, it checks the options cache and bails early if the requested options are all primed.

If items appear to need priming after that, the notoptions cache is checked and known invalid options are removed. If no options need to be primed after that then the function returns early.

As the likelihood of multiple requests to prime non-existent options is fairly low, I don't think this needs to be fixed in a rush and can be handled in 6.4.1 or 6.5.

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

Forked from PR ​https://github.com/WordPress/wordpress-develop/pull/5548 as it's likely to go in.

Unlike the patch I put on the on the ticket, this does get notoptions in the original request to the cache. As notoptions is in the options group it can be done without an additional request to the caching server. For persistent caches supporting multiple gets, this should be slightly more performant.

[flixos90]

Thanks @peterwilsoncc, great catch! I just reviewed the PR and the solution looks great to me.

I would be happy for this to be merged before the 6.4 release since I am confident about the fix. That said, it's not a critical bug so it could also be fixed in a 6.4.1, if there was concern about making the change this late in the 6.4 cycle. Will defer to other people's thoughts.

I'll milestone at 6.4 now, but it could be punted to 6.4.1 if preferable.

@felixarntz

• The falsey options issue occurred to me overnight too, I've pushed the change and added the test test_wp_prime_option_caches_handles_falsey_option_value() which tests for '', '0' and array()
• I've removed the second get of notoptions and moved to the clearer approach you suggested

While looking at the tests, I noticed a bunch of tests for options in the notoptions group rather than using the cache key. I think this is incorrect so I will work on changing those too. Case in point

​https://github.com/WordPress/wordpress-develop/blob/7c4be263cdb6e093a490b19e652f7ee707440012/tests/phpunit/tests/option/wpPrimeOptionCaches.php#L212-L216

It's currently in the new test so I'll fix that while about it.

[peterwilsoncc]

@flixos90 and I picked up a few things while looking over this, in addition to the items above:

• primed falsey option values '', '0' and array() result in a second query as the option's cache value wasn't using a strict type check
• a few tests for incorrect items in notoptions used the notoptions group rather than cache key in the options group.
• the tests confirming that wp_prime_option_caches_by_group() was correctly priming the cache used the expected value get_option( 'option_name' ) which will prime the cache, potentially causing a false pass

The ​linked pull request fixes each of the above along with the original issue. It's now ready for review.

@hellofromTonya and @mikeschroder:
As the option value '0' is quite common (there are 17 in a clean WP install) I think it's probably worth trying to get this in to 6.4.0 but I'll let you make the call as tech leads.

[hellofromTonya]

Replying to peterwilsoncc:

As the option value '0' is quite common (there are 17 in a clean WP install) I think it's probably worth trying to get this in to 6.4.0 but I'll let you make the call as tech leads.

What is the result and severity / impact for users for bug(s) identified in this ticket (including '0')?

Knowing more concretely how the bug(s) will impact users will help to determine when if it should go into 6.4.0.

[peterwilsoncc]

Replying to hellofromTonya:

What is the result and severity / impact for users for bug(s) identified in this ticket (including '0')?

When priming autoloaded falsey options there will be no impact as these are covered by the alloptions cache.

When priming non-autoloaded falsey options/non existent options the impact will be an additional database query each time such an option is attempted to be primed. This will occur for users both with and without a persistent object cache.

Ideally an option will only ever be attempted to be primed once per page load but for systems with a plugin architecture of their own (WooCommerce, various Forms plugins) there may be a higher impact if the plugin's plugin attempts to prime such options.

Overall severity is probably pretty low as the functions are new so unlikely to be widely adopted.

[kirasong]

Since I was pinged on this, checked into it for an opinion.

I agree with the assessment that either 6.4 or 6.4.1 would be fine, and don't have a strong opinion here.

Personally, I am okay with it landing for 6.4, so long as it lands before RC3 and there remains high confidence from y'all / maintainers.

Reasoning: This fixes bugs for a new feature shipping in 6.4 (re: RC guidelines), and the fixes are focused and supported by automated tests.

As @hellofromTonya has been more involved in this feature, happy to defer if there is a difference of opinion, but thought I'd leave some feedback regardless.

[hellofromTonya]

@peterwilsoncc and I chatted (in my late last night) to better understand what users could expect from these bugs (i.e. without a fix shipping in 6.4.0):

• Severity is low.
• Worse case: would cause extra DB queries.

Should it be included in 6.4.0?

As @mikeschroder noted, ​the RC process guidelines state:

Regressions: bugs that have been introduced during the WordPress 6.4 development cycle, either to existing or new features.

The bugs were introduced during this cycle. Given RC3 is in a few days and final in a little over a week, risks need to be mitigated at this late stage in the release. Thus, I'd be okay with it going into 6.4.0 if:

• there's high confidence in the fix (i.e. code reviews, automated tests, and manual test reports).
• code changes are minimal.

More significant, risky, and/or non-high confidence changes could be targeted for 6.4.1, i.e. to give more time for discussion, consideration, and testing.

[flixos90]

Replying to hellofromTonya:

The bugs were introduced during this cycle. Given RC3 is in a few days and final in a little over a week, risks need to be mitigated at this late stage in the release. Thus, I'd be okay with it going into 6.4.0 if:

• there's high confidence in the fix (i.e. code reviews, automated tests, and manual test reports).
• code changes are minimal.

I think both of these are the case here, so I would support this being committed to the 6.4 branch, which we could probably do today. They are indeed not critical bug fixes (because they only result in unnecessary database queries, not actually incorrect data), but I don't have any concerns about introducing other problems with this code. So I think it's worth fixing it now instead of shipping with the bug and fixing after.

[peterwilsoncc]

I discovered another issue while writing tests, the cache may be primed differently depending on whether an option is primed via the priming function vs get_option.

$ wp shell
wp> update_option( 'pwcc-falsey', false, false )
=> bool(true)
wp> wp_cache_delete( 'pwcc-falsey', 'options' );
=> bool(true)
wp> wp_cache_get( 'pwcc-falsey', 'options' );
=> bool(false)
wp> get_option( 'pwcc-falsey' );
=> string(0) ""
wp> wp_cache_get( 'pwcc-falsey', 'options' );
=> string(0) "" ######### via get_option
wp> wp_cache_delete( 'pwcc-falsey', 'options' );
=> bool(true)
wp> wp_prime_option_caches( 'pwcc-falsey' );
=> NULL
wp> wp_cache_get( 'pwcc-falsey', 'options' );
=> bool(false) ######### via cache priming

@flixos90 I think it's probably best to hold off until 6.4.1 so we can get all the fast-follows in as needed. I'm pushing tests to the PR as I find them, I'm not worried if you decide to push to my branch.

@hellofromTonya There are a couple of very minor test fixes that I think it would be good to get in so I'll set up a PR that doesn't touch src for those. But not today as it's the weekend.

[peterwilsoncc]

Here's a full test -- let me know if I am doing something wrong. ​https://github.com/WordPress/wordpress-develop/actions/runs/6673810783/job/18139891250?pr=5576#step:14:328

I've forced pushed to get a run of the failing tests only as I added some after the initial source changes.

The failing test run can be seen in ​this run of the GitHub actions.

Here's a summary of the bugs I've found:

1. repriming non-existent options triggered a database call
2. repriming falsey options triggered a database call (props Felix)
3. the shape of the caches differed when primed via the priming function vs get_option() (they are cached in their serialized form in get_option()
4. the tests for ensuring priming by group did in fact prime could result in a false pass

I've refactored my initial tests to ensure falsey values didn't trigger a repeat database call to test that no type of value triggered a repeat database call. This better represents the heart of the issue: the type doesn't matter, the additional DB call does.

cc @hellofromtonya

[peterwilsoncc]

This has become a bit of a general fast-follow ticket rather than limited to the initial bug.

Since my last update detailing each of the known bugs, there are now four:

1. repriming non-existent options triggered a database call
2. repriming falsey options triggered a database call (props Felix)
3. the tests for ensuring priming by group did in fact prime could result in a false pass
4. (new) the shape of the caches differed when primed via the priming function vs get_option() (they are cached in their serialized form in get_option()

The ​linked pull request handles each of these bugs.

Regarding severity: I think the new item (4) bumps the severity up a little as the shape of the cache should be consistent regardless of how it is primed. Furthermore, if used in the wild with a persistent cache, the difference will stick around for a period of time after the fix.

[hellofromTonya]

Replying to peterwilsoncc:

This has become a bit of a general fast-follow ticket rather than limited to the initial bug.

Since my last update detailing each of the known bugs, there are now four:

1. repriming non-existent options triggered a database call
2. repriming falsey options triggered a database call (props Felix)
3. the tests for ensuring priming by group did in fact prime could result in a false pass
4. (new) the shape of the caches differed when primed via the priming function vs get_option() (they are cached in their serialized form in get_option()

The ​linked pull request handles each of these bugs.

Regarding severity: I think the new item (4) bumps the severity up a little as the shape of the cache should be consistent regardless of how it is primed. Furthermore, if used in the wild with a persistent cache, the difference will stick around for a period of time after the fix.

6.4 RC3 is delayed 2 days to Nov 1.

Are there src fixes that should be considered for 6.4.0? For example, item 4. cc @peterwilsoncc @flixos90

[flixos90]

Thank you @peterwilsoncc for the additional work here, and great catch on the additional maybe_unserialize() call leading to inconsistent caches.

I agree that this is actually the most severe of the 3 problems your PR for this ticket addresses (I'm not counting point 3 from your list since it's a test fix, i.e. doesn't affect the WordPress core production code).

@hellofromTonya I think we should commit this in its current form and backport to 6.4. The new point indeed increases the severity of this, and the other 2 more minor fixes are reasonable to backport as part of this as well. The risk of backporting this is extremely small, since they are exclusively about a new function that nobody uses as of today. All that these fixes do is make this function more reliable once it is out there, so I don't think we should be splitting these fixes out into separate commits or anything.

Marking this as ready for commit, and ready for backport, to expedite that process. Please let me know if you have any concerns about that. If not, feel free to commit and backport, or I can help as needed.

[peterwilsoncc]

@hellofromTonya @flixos90 Yes, I think it should be included in 6.4.0 given the difference in data storage.

[joemcgill]

Just reviewed the PR as well. This looks like a nice improvement and catches a couple of other edge cases. Thanks, @peterwilsoncc!

[hellofromTonya]

Replying to peterwilsoncc:

@hellofromTonya @flixos90 Yes, I think it should be included in 6.4.0 given the difference in data storage.

Thanks :)

​The patch:

• Has been well reviewed and considered ✅
• Has 2 approvals ✅
• Has high confidence from the 3 committers who are and have been deeply involved ✅

Big 👍 from me for its commit and backport to 6.4.

Thank you all for identifying and resolving these issues in time for 6.4 RC3 ⭐

[peterwilsoncc]

In 57029:

Options, Meta APIs: Fast follow fixes for option cache priming functions.

A collection of fixes for wp_prime_option_caches():

• cache arrays and objects in their serialized form for consistency with get_option() and wp_load_alloptions()
• prevent repeat database queries for falsey and known non-existent options (notoptions)

Additional tests for wp_prime_option_caches() to ensure:

• additional database queries are not made repriming options (known, known-unknown and alloptions)
• cache is primed consistently
• get_option() returns a consistent value regardless of how it is primed
• database queries do not contain earlier primed options
• get_option does not prime the cache when testing the cache has been successfully primed

Fixes a test for wp_prime_option_caches_by_group() to ensure get_option does not prime the cache when testing the cache has been successfully primed.

Follow up to [56445],[56990],[57013].

Props peterwilsoncc, costdev, flixos90, hellofromTonya, mikeschroder, joemcgill.
Fixes #59738. See #58962.

Committed in r57029 / ​https://github.com/WordPress/wordpress-develop/commit/17a01ed185f38f87362eb7588908dc95ec6cbe23

[peterwilsoncc]

In 57030:

Options, Meta APIs: Fast follow fixes for option cache priming functions.

A collection of fixes for wp_prime_option_caches():

• cache arrays and objects in their serialized form for consistency with get_option() and wp_load_alloptions()
• prevent repeat database queries for falsey and known non-existent options (notoptions)

Additional tests for wp_prime_option_caches() to ensure:

• additional database queries are not made repriming options (known, known-unknown and alloptions)
• cache is primed consistently
• get_option() returns a consistent value regardless of how it is primed
• database queries do not contain earlier primed options
• get_option does not prime the cache when testing the cache has been successfully primed

Fixes a test for wp_prime_option_caches_by_group() to ensure get_option does not prime the cache when testing the cache has been successfully primed.

Follow up to [56445],[56990],[57013].

Reviewed by flixos90, hellofromTonya, joemcgill.
Merges [57029] to the 6.4 branch.

Props peterwilsoncc, costdev, flixos90, hellofromTonya, mikeschroder, joemcgill.
Fixes #59738. See #58962.