Disable autoload for large options

[markjaquith]

A frequent issue I encounter with WordPress sites is extreme slowdown due to a huge option that is being autoloaded. Sometimes this is some sort of cache option that a careless plugin has let be autoloaded, and has grown to tens of megabytes.

Having an option this large be autoloaded not only slows downs the options loading query (when that option may not be required on every load), but on sites with object caching, it causes a lot of wear and tear on the alloptions cache. Combine a large option with a frequently updated option and you have a recipe for a very sluggish site.

We should consider preventing options from being autoloaded if they grow to a certain size. Off the top of my head, 100kb sounds like a reasonable upper bounds. We could monitor option settings/updates and peek at the size of the option going in. If it's above a certain (filterable) bounds, we can force autoload to be off.

Add a hard limit of 150kb of option size. Also add a new default value of autoload to default.

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

[spacedmonkey]

I have a start of a PR for this at ​#4881.

[flixos90]

Per my feedback on the PR, I think the approach it takes is too complicated and puts too many responsibilities into a single function. So I think this needs an iteration.

Something to consider is if we should change these values to default-

​https://github.com/WordPress/wordpress-develop/blob/60d2d8ac75c4350a45e4c6970c7d0295b757f1f0/src/wp-admin/includes/schema.php#L593-L597

​https://github.com/WordPress/wordpress-develop/blob/60d2d8ac75c4350a45e4c6970c7d0295b757f1f0/src/wp-includes/option.php#L1283-L1285

[flixos90]

@markjaquith @boonebgorges Any chance you can take a look at the PR ​https://github.com/WordPress/wordpress-develop/pull/4881? Would be great to get your thoughts on the direction.

[spacedmonkey]

This ticket was discussed in the ​WordPress core performance chat today. It was agree this ticket needs more time to bake and for feedback from the community. Punting to WP 6.5

[spacedmonkey]

I am unable to work on this ticket in WP 6.5, removing myself as owner.

Code refresh

Code refresh

[pbearne]

I have refreshed the code

I found an issue with the tests failing
The update_option() function was clearing the cache correctly

This is ready to merge

Closing in favor of #5671, which iterates on this.

[flixos90]

I just reviewed the new PR by @pbearne, and it looks very good to me functionally, despite needing a few more changes.

Separately, I would like to bring the conversation about the new database values for the autoload column back to this ticket, to decouple from the specific PR implementation.

Including the new values (regardless of whether they're called default-yes/default-no or something else) in the database is not only a "side effect" of this change. It is a requirement to make the thresholds for the maximum size of an option work reliably. Here's why:

• Options don't always maintain the same size. An option may be added with some very small starter/placeholder value, but later grow much larger depending on how the option is used and how the end user interacts with the relevant functionality.
• Because of that, it is crucial that we not only perform the maximum size check when adding the option, but also when updating it.
• For that, unless the developer explicitly passes a value to update_option() (which is not exactly intuitive to do when you just want to update the option value), we need to see whether the autoload value previously stored is an explicit one chosen by the developer (yes|no) or not (default-yes|default-no). In the latter case, we must re-check the option size. But in the first case, we must not do that, as we need to respect the developer's explicit choice.
• In other words, being able to determine between explicitly set autoload values and "chosen by WordPress" autoload values requires this differentiation to be made in the database.

The only alternative to that which I can think of is completely doing away with the $autoload parameter of all those functions and instead handle it as part of either setting registration or a separate way to register an option's autoload value independently. That however would have much wider implications and affect far more plugins than the currently proposed implementation that adds support for two more database values. Only plugins that implement their own functionality around autoloading options (e.g. certain database optimization plugins) will require an update with the current proposal, while a broader shift towards autoloading registration would be far more disruptive and affect plugins of all kinds.

[pbearne]

Or we can just block all large options from autoloading

Even this can worked round by adding a late filter to set the limit high enough

Thinking this is a good start but needs a bit more consideration/refinements especially in the way it would work. Adding support for two new values for the DB column seems good, but changing function signatures does not. Added few code comments and suggestions.

Also thinking this patch should introduce a filter to allow individual decisions to turn autoload on or off to be overridden by plugins. Trying to pass null (meaning "undecided") when adding new options is not a good way to accomplish this imho.

In addition the idea for this patch would need to be thoroughly tested for performance. Autoloading options that are always needed is imperative for improved performance, even if their values are "huge". The cost of an additional trip to the DB is usually much higher.

Was also thinking, the default-yes and default-no don't really sound that good, and may be misleading. Seems possible to be mistaken with the default when adding options, which would be wrong.

How about auto-yes and auto-no? Having auto in there makes it really clear this setting is automated :)

[azaozz]

Replying to pbearne:

Or we can just block all large options from autoloading

Ummm, that would be a huge performance loss for options that are used often. The additional trip to the DB may be hundreds of times slower.

Already mentioned this on the PR, thinking this would need a good amount of performance testing with different scenarios. Most importantly what happens when autoload is turned off for big options that are used on every page load, or on every second page load, etc.

Thanks for your feedback @azaozz, I left replies on the individual points.

Regarding the name, I agree default-yes and default-no don't sound great. I like your proposed auto-yes and auto-no.

Going to reply here so this shows on the trac ticket too. From ​https://github.com/WordPress/wordpress-develop/pull/5671#discussion_r1419653637:

@azaozz I don't think it really is a BC break. While the value on the function is no longer yes by default, the effective outcome...

It is a change in behaviour in a commonly used function. Until now all extenders that wanted to add an autoload option would leave out the last param as it defaults to yes. However if this patch is committed, that will not be the case any more. Options whose values are over a certain threshold will be set to not autoload.

This way of doing things also has another disadvantage. If a plugin wants to override the automated decision, it will have to do it in different ways: once pass yes when adding the option, and then use the wp_max_autoloaded_option_size (or the new filter that could be added there to make it more intuitive) to make sure this is not auto-changed in the future. Pretty confusing, why not just use the filter? :)

The reason to change the default is to properly differentiate between whether a developer passed yes or whether it's just default-yes / auto-yes.

Why should this matter? I think this is more of a "how should this actually work" discussion. The question is: how should developers override the automated decision to turn autoload off?

Imho a filter to do that per individual option would be good, commonly used way in WP in many different cases. Another advantage in using such filter is that when the size of an option value goes over the threshold, the plugin that has added the option would be able to override the decision to set autoload to off. (The current code doesn't cover this case as far as I see).

Generally the choices are:

1. Ask extenders to change their code and pass yes for autoload when adding new options. This method does not solve the question what happens when an option has grown larger when it is being updated. The current behaviour is to turn autiload off for it when it is an pre-existing option, or to leave autoload on for options that will be added after this patch is applied (see the code here that returns early: ​https://github.com/WordPress/wordpress-develop/pull/5671/files#diff-ca21e48c46c9dd91f3e8b8837f147ae3d99457233f4f8a042ef458205d4d81e3R1173-R1180).
2. Add a new filter that will let extenders to always override the autoload/no-autoload decision made by the new determine_option_autoload_value() function.

From ​https://github.com/WordPress/wordpress-develop/pull/5671#discussion_r1419698845:

Related to my reply in ​https://github.com/WordPress/wordpress-develop/pull/5671#discussion_r1419694609, we need to differentiate between "default/auto" vs "manual" behavior
...
this code checks whether the current value is a default/auto value, or an explicitly provided value
...
If we forced the use of the new automated approach, I'd consider that a BC break.

Yes, I understand. The problem is that this way of managing autoloading seems unintuitive and pretty much the opposite of the existing code. I.e. currently all calls to add_option() that do not pass explicitly the third param may be relying on having the option autoload. From now on that will not be the case. Changing the default from yes to undecided also changes the behaviour of the function.

To be able to deal with this I'm also thinking we could potentially move away from yes and no as the values, and introduce new value to force autoloading (or values to force both autoload and not-autoload?). Seems that would resolve this in a simple and intuitive way and will not introduce any changes to the existing behaviour.

@azaozz

It is a change in behaviour in a commonly used function. Until now all extenders that wanted to add an autoload option would leave out the last param as it defaults to yes. However if this patch is committed, that will not be the case any more. Options whose values are over a certain threshold will be set to not autoload.

Right, this is a behavioral change, but it will only affect a very small subset of options - 150 KB is a lot. On another note, this optimistically assumes that extenders _intentionally_ don't pass the $autoload parameter because they want the option to autoload. Since autoloading options is on its own a very low level concept, my hunch is the majority of extenders don't give much thought on whether or not to autoload an option - in other words, I assume more often than not it's not a conscious decision.

This way of doing things also has another disadvantage. If a plugin wants to override the automated decision, it will have to do it in different ways: once pass yes when adding the option, and then use the wp_max_autoloaded_option_size (or the new filter that could be added there to make it more intuitive) to make sure this is not auto-changed in the future. Pretty confusing, why not just use the filter? :)

That's not what is being suggested here: The suggested approach is that the $autoload parameter continues to provide control over whether or not the option is autoloaded. As in: If an extender passes yes or no, WP core will _never_ change that.

If I understand you correctly, what you're describing here (requiring use of a new filter to override the core decision) would be a breaking change with larger implications than the above: If WordPress core no longer respects an _explicitly_ passed yes or no, that's more severe than changing what is the default, as it would mean the $autoload parameter becomes effectively pointless.

Why should this matter? I think this is more of a "how should this actually work" discussion. The question is: how should developers override the automated decision to turn autoload off?

See above. If the automated decision needs to be overwritten by a filter, that's a breaking change, as that would mean WP core no longer respects when an extender passes yes or no to the function.

Changing the default from yes to undecided also changes the behaviour of the function.

That's correct. Though per the above, the surface area for where it actually changes is very small due to the intentionally high threshold. More importantly though, the key problem here is that the current default is yes. IMO there should have never been a default in the first place, or it should have always been a default of "we choose it for you".

While this default change is indeed a breaking change, I think it is a very low-severity breaking change that won't affect many plugins/options, and it won't affect existing sites much either, as the options will already be present in the database with yes or no. This is a scenario where I believe the benefits of this small breaking change are greater than its cost.

To be able to deal with this I'm also thinking we could potentially move away from yes and no as the values, and introduce new value to force autoloading (or values to force both autoload and not-autoload?). Seems that would resolve this in a simple and intuitive way and will not introduce any changes to the existing behaviour.

That's effectively what the null value means in the PR as it currently is. However, as long as yes remains the default, this won't help much, since yes is simply not a good default (for the above reasons).

@felixarntz thanks for the response and clarifications.

Right, this is a behavioral change, but it will only affect a very small subset of options - 150 KB for a single option is a lot.

Yep, I agree the chance is very slim. But if this happens in a more popular plugin the chances are that many thousands of sites will be affected. Ideally there should be no such chance :)

If an extender passes yes or no, WP core will never change that.
...
See above. If the automated decision needs to be overwritten by a filter, that's a breaking change

Yep, makes sense. So changing the autoload should never happen automatically for existing plugins as the assumption until now was that not using the third argument for add_option() means the option will be autoloaded. This has been the default for many years. Then the only way for WP to change autoloading would be if the plugins opt-in, right?

More importantly though, the key problem here is that the current default is yes. IMO there should have never been a default in the first place, or it should have always been a default of "we choose it for you".

Heh, yea, it would have been perfect if we could go back in time and set it that way! :)
(If I remember correctly there was no autoloading in early versions of WP. It was added in something like 2.3 or 2.5. That's the reason the default became yes as it was a huge performance improvement.)

That's effectively what the null value means in the PR

Yea, to some extend. However it has the drawback of not being particularly clear, changes the current behaviour od the function, and also I think passing null there can be used (or was used) to turn off autoloading. So using null there is not suitable.

Frankly I think that yes and no should be "soft-deprecated" and there should be brand new values that would control autoloading better. Thinking perhaps enabled, disabled, auto-enabled and auto-disabled` would make most sense.

Then yes would stay as the default parameter value to support the huge number of existing plugins, and will be saved to the DB as enabled. At some point, maybe even now, WP can start throwing "deprecated value" warnings so plugins will be gradually updated to decide if autoloading can be set automatically or may been to be turned off and to use the new values.

Another thing I'm still a bit unsure about is what happens when a "big option" is needed on every page load. If such option is not autoloaded there will be a separate trip to the DB to fetch it. Seems this will be a sizeable performance degradation that would need to be addressed before adding this patch?

Was also thinking, the default-yes and default-no don't really sound that good, and may be misleading. Seems possible to be mistaken with the default when adding options, which would be wrong.

How about auto-yea and auto-no? Having auto in there makes it really clear this setting is automated :)

changed

oloaded there will be a separate trip to the DB to fetch it. Seems this will be a sizeable performance degradation that would need to be addressed before

with the filters, a dev can allow/force a big option to be loaded

[pbearne]

I have tried to clear the issued raised
Please recheck

I fix the tests not working (main due to filter name changing
But also a PHP 8+ issue not liking null in strlen()

As I've thought about this, I think the thing that I keep getting stuck on is that there is currently no way for core to improve the autoload situation without being able to distinguish between options that have been added to the DB with and autoload value of 'yes' because of the default or because of intention, which is what makes these decisions difficult.

I think a solution to disabling some auto-loaded options without considering a path to change the way default values are recorded in the database will keep us at a disadvantage when considering not only this, but also future enhancements.

Looking at what @azaozz proposed:

Frankly I think that yes and no should be "soft-deprecated" and there should be brand new values that would control autoloading better. Thinking perhaps enabled, disabled, auto-enabled and auto-disabled would make most sense.

Then yes would stay as the default parameter value to support the huge number of existing plugins, and will be saved to the DB as enabled. At some point, maybe even now, WP can start throwing "deprecated value" warnings so plugins will be gradually updated to decide if autoloading can be set automatically or may been to be turned off and to use the new values.

I think a soft deprecation of "yes" is one solution, but I think "no" can be trusted, since this has to be explicitly passed to an option. Another option would be to run an update routine on existing options marked as "yes" to and change them to "enabled". We could combine this with a change in behavior to the way the default param is saved to the DB and from that point on have the following values for auto-loaded options:

• yes – The value was added with an explicit value of "yes"
• enabled – The option was added with an unknown explicit value, but was previously set to "yes"
• default – The an explicit value was not passed, WP can decide to change default behavior in the future (initially will be autoloaded)
• disabled – The option was added with no explicit yes or no autoload value, and WP has chosen to disable it.
• no – The option was added with an explicit autoload value of "no".

This would put us in a future state where we can change the future autoloading default and distinguish in the DB between options that are relying on default behavior, ones that have been explicitly set, ones that were programmatically enabled (e.g., because they were existing values) or disabled (e.g., because they were too large).

Thank you for the additional feedback @azaozz @joemcgill. I think a deprecation strategy could work. However, I would want to make sure the benefit of that is not only naming. Based on your proposals, how about the following:

1. yes and no will be deprecated in favor of enabled and disabled.
2. There will be deprecation warnings to replace those usages, but initially no functional change: Initially enabled will behave the same way as yes and disabled will behave the same way as no.
3. In a future WordPress core release (after the above has been rolled out already), the new behavior will be put in place:
   • yes will be treated as a non-explicit value. Any yess passed to the function or stored in the database may be overwritten by WordPress's own "decision process" (i.e. auto-enabled unless the option value is too large, in which case it becomes auto-disabled).
   • no can still be treated as an explicit value, i.e. it has the same meaning as disabled.
   • The functions will get a new default that allows WordPress to make the decision. It could be null or auto-enabled for example, whichever we prefer.
   • There could be a database cleanup routine to migrate all no values to disabled and all yes values to auto-enabled (potentially the relevant ones even to auto-disabled). That's not critical for the functionality though, it would be mostly for cleanup and to give the benefits to existing options right away too.

I think this would give sites and plugins a good migration path, and no migration routine would be required on the core side (it would just be a nice-to-have, i.e. things still work fine if it's not run e.g. on a very large site).

The only tricky part is: If we deprecate yes and no but still keep yes the default, we'll give a ton of deprecation notices to people that don't even pass a deprecated parameter. It might make sense if we want to go with the assumption that some developers _intentionally_ leave out the parameter to get yes. But it's still a bit odd I guess to trigger deprecation notices in a situation where the deprecated value isn't actually passed to the function. 🤔

Going back to what I proposed, I don't think we need to trigger any deprecation notices at all if we do the following:

1. Auto-update all options in the DB with autoload set to yes to enabled.
2. Change the default $autoload value of add_option() from yes to null (which matches update_option() already).
3. Save any new option created with $autoload = null value as default.
4. Update this PR so that any new large options that are added with an autoload value of null are instead set to disabled.
5. Update the autoload functionality so that any option set to yes, enabled, or default are auto-loaded (keeping the current behavior).
6. Encourage developers to update their code to explicitly pass an $autoload value of yes or true if the option is needed for the majority of requests.
7. Consider a future change where we stop auto loading options that are set to default.

Ideally, we would be able to distinguish from the stored DB values whether the option was explicitly set to autoload or not autoload, whether it was enabled/disabled by core, or if it was set as the default value and left unaffected by core.

@joemcgill Thanks for clarifying, I think now I understand what you mean.

I believe what you're proposing is very much along the lines of what this PR proposes so far, except for naming. The one thing I don't understand in your proposal is: What's the purpose of enabled? It looks like it would only become a replacement for yes? In that case, what's the benefit of that over just sticking with the existing yes value?

FWIW I would be on board with what you're proposing as far as I understand. But I believe it doesn't address the concerns that @azaozz voiced in that it would still involve changing the default behavior of the function (from "always autoloading" to "autoloading only if option value is not too large").

What's the purpose of enabled? It looks like it would only become a replacement for yes? In that case, what's the benefit of that over just sticking with the existing yes value?

Currently, sites have options in the DB with the autoload value set to 'yes', but there is no way to know if those were explicitly set or if they were set by default. Changing those to enabled allows us to respect their current behavior for backwards compatibility reasons, and have a future state where we know the difference between options that were explicitly set (i.e., yes) or are are relying on default behavior (i.e., default). I think being able to make these determinations are critical to us being able to adapt to future needs (though I don't feel strongly about the exact values used).

To address @azaozz's concern that we could end up inadvertently stop autoloading a large option that is actually used in the majority of requests, having an enabled value also gives us the opportunity to programmatically mark an option for autoloading, even if it wasn't explicitly set. As a hypothetical example, we could begin monitoring the usage of large options that are set to default over the next several requests and set them to either enabled or disabled based on usage, rather than just size.

Without some value to distinguish between explicit (yes) and default auto-loading, I'm not sure how we can safely change behaviors going forward.

@joemcgill

Currently, sites have options in the DB with the autoload value set to 'yes', but there is no way to know if those were explicitly set or if they were set by default. Changing those to enabled allows us to respect their current behavior for backwards compatibility reasons, and have a future state where we know the difference between options that were explicitly set (i.e., yes) or are are relying on default behavior (i.e., default). I think being able to make these determinations are critical to us being able to adapt to future needs (though I don't feel strongly about the exact values used).

I see. So in your thinking yes is the explicit value and enabled the non-explicit / potentially WordPress-determined?

I think we may just be confused by each other's naming. 100% we need to be able to differentiate between an explicitly set yes vs a yes by default. So far in the discussion this was going to be accomplished by something like yes vs auto-yes, or in my recent proposal in ​https://github.com/WordPress/wordpress-develop/pull/5671#issuecomment-1852837985 by enabled vs auto-enabled.

Naming is of course of secondary nature as we try to determine the solution. So I _think_ we're basically saying the same thing with slightly different approaches (e.g. whether or not have a deprecation/transition period to prepare for the change in behavior).

Yes, I do think we're pretty close. The main difference that I see is that I'm also wanting to be able to distinguish between options that should have the default behavior applied and ones that have been effected by core (e.g., default vs. enabled/auto-yes or disabled/auto-no). I also don't remember seeing an proposal to change currently set values from 'yes' to something else, in order to make 'yes' going forward an explicitly set value—though I may have just missed it.

@joemcgill

The main difference that I see is that I'm also wanting to be able to distinguish between options that should have the default behavior applied and ones that have been effected by core (e.g., default vs. enabled/auto-yes or disabled/auto-no).

I definitely want that too. The PR currently accomplishes it via yes and auto-yes.

I also don't remember seeing an proposal to change currently set values from 'yes' to something else, in order to make 'yes' going forward an explicitly set value—though I may have just missed it.

Right, that's indeed not something present in the PR and not something I'm proposing. In my thinking, all existing yes values in the database would simply be treated as explicit, even though many of them likely aren't. In other words, the new behavior would only start kicking in at the point where this is launched in WordPress. Old values stay in place, i.e. this will just gradually over time lead to an improvement.

While I'm not opposed to a migration routine as you're proposing, the benefits I see in leaving the old values untouched and assuming them to be explicit (for safety reasons) are:

• No risk of the migration failing (e.g. on large sites those things can be of concern).
• Lower risk of large-scale adverse effects in the ecosystem: Even plugins that don't migrate (or don't migrate right away) to the newly recommended approach will have limited negative impact on end users. Since existing options in the DB will continue to work as before, it would only impact new sites or sites that newly install the plugin. That certainly doesn't mitigate the risk, but it considerably lowers the chance of "code red" problems.

@felixarntz after coming back to this after a few weeks, I think it may be helpful to clarify the end state I think we should be in, rather than being focused on some of the naming/implementation details. Setting aside any options that are already stored in databases before we make any changes for now, in the future we want to be in a situation where we can distinguish between options with explicitly set autoload values, autoload values that have been set based on heuristics like this one (i.e. option size), and ones that are simply relying on WordPress' default behavior for options. To avoid confusion with the current yes/no values, let's assume that at minimum we had three values in the DB:

• on – The option was added with an explicit truthy autoload value (i.e., not relying on the default param) and will be autoloaded
• auto – The option was added without passing an explicit autoload value. These would still be autoloaded, based on current behavior.
• off – The option was added with an explicit falsey autoload value and would NOT be autoloaded.

This would allow us to make informed decisions about whether an option was added with the explicit intent of being autoloaded rather than the current state, where we don't know if an option's autoload value is 'yes' because of developer intent, or because they didn't specify any autoload value. In my view, this distinction is critical if we want to be able to make informed decisions about whether an option that is already in the DB can stop being autoloaded in the future, or even—at the extreme—decide that WP should no longer autoload options by default.

That said, this PR introduces a new concept where an option that was added without an explicit autoload value would be added with an autoload value that would allow us to NOT autoload it by default. To do so, we would need to introduce a fourth value to the three above, e.g., auto-off, meaning that the option was added without an explicit autoload value, but will not be autoloaded based on some criteria (option size, in this case). We could stop with these four states for now, but in my mind auto-off implies the need for an auto-on value, meaning that the option SHOULD be autoloaded, even if in the future we decided to stop autoloading by default. A totally theoretical but valid use case for such a value would be a large option that we've determined should be safe to autoload, because it is called on every page load. So I would propose we end up with 5 new possible autoload values in the DB for options (final value names to be determined):

• on – The option was added with an explicit truthy autoload value (i.e., not relying on the default param). This option MUST be autoloaded.
• auto-on – The option was added without an explicit autoload value, but SHOULD be autoloaded.
• auto – The option was added without an explicit autoload value. WordPress can determine whether this option should be autoloaded (To be consistent with past behavior, we will initially still autoload these values, but that may change in the future).
• auto-off – The option was added without an explicit autoload value, but SHOULD NOT be autoloaded.
• off – The option was added with an explicit falsey autoload value and MUST NOT be autoloaded.

With the above being the future state that I would propose we aim for, we need to consider what to do with options that are already in the database, where we are not able to determine whether an option is autoloaded based on explicit or implicit (default) intent. I had previously suggested that we add a DB update routine that would set all options previously set to yes to the new auto-on value as a way of being confident in the intent of the new on value. However, I concede that this would come with some risk, so I'm happy with the previous suggestions to simply deprecate the old yes and no values and replace them with a new set of values as both solve the same problem.

The final consideration where there seems to be some disagreement in this thread is what (if any) changes should be made to the default parameter of the add_option() function in order to implement this new autoloading strategy. Here, I see three options:

1. Leave it as is, $autoload = 'yes', but use something like func_get_args() to determine if an explicit value was passed or not. If not, then we can determine whether the option should be auto-on, auto, or auto-off. However, if we deprecate yes it's a bit odd to keep that as the default value, IMO.
2. Change the default value to a new value that is neither truthy or falsey, to indicate that the option might not get autoloaded. I think $autoload = null makes sense, or something like $autoload = 'default' or $autoload = 'auto' to better communicate the intent of this default to developers. One of these would be my preference.
3. Update the default param value to a different truthy value that aligns with the new strategy as long as we're still autoloading most options by default. Personally, I would just update the code to make the boolean true the default, which is already supported and deprecate uses of yes or no, but we could also make the default on or whatever the new explicit DB value ends up.

Hopefully this very long (sorry 😬 ) explanation is helpful in clarifying what I think we should aim for and lay out some possible next steps.

@joemcgill Thank you for the detailed writeup, this is pretty much in line with what I'm thinking. A few additional thoughts/notes:

• I actually like on and off as the new terms. IMO they fit a bit better together with the auto- prefix than enabled and disabled, and I also think semantically they are closer to a "yes" and "no" for autoloading. In any case, I don't feel strongly about the names chosen (other than that they should differ from yes and no), especially since I think from an external developer API perspective it makes most sense to encourage usage of the booleans true and false. Most plugin developers will not need to worry about which strings are actually stored in the database - that only matters for a tiny fraction of plugins that explicitly focus on autoloading options.
• I think having auto-on, auto and auto-off makes sense, especially as it allows for future refinements to the logic to decide whether to autoload in either core or plugins. I was initially thinking to only have auto-on and auto-off, but it's a good point that the only decision core would be making as of this ticket is a "no" decision. While anything that isn't "no" will as of today still be autoloaded, I agree it's worth differentiating between "core decided yes" and "core simply doesn't know". Both will be autoloaded as of today, but it allows a differentiation that is critical for further potential enhancements to autoloading optimization.
• There should probably exist some kind of filter to allow for additional logic to decide the "auto" value in case no explicit value was provided when calling add_option(). For example, it could be a wp_default_autoload_value filter which can return either true, false, or null, which would then be interpreted as auto-on, auto-off, and auto respectively.
• Regarding the add_option() default, I think it should be changed to null, for the following reasons:
  • It is a neutral value that doesn't signify anything.
  • It is the same default that is already used in update_option().
  • It is backward compatible as today explicitly passing null (which doesn't make much sense but is possible) will have the same outcome as passing yes.
• Per my first point, I would suggest that we encourage developers to pass either true, false, or nothing (null) to add_option(). yes and no will still need to be supported for BC, and we could even support on and off, but I think this is pointless as from a plugin developer perspective all mean the same. Example code / documentation should always use true and false, and the parameter doc could probably say something like "The values 'yes' and 'no' are supported for backward compatibility.".

Thanks @felixarntz. Given your first 2 points above, it sounds like we're in agreement on having 5 values and can probably just go with that naming for now. I'm happy for us to move forward with deprecation of 'yes' and 'no' values in the DB (as was @azaozz above), so it seems like a good next path forward.

I totally agree with you that adding a filter to allow third party code to override/enhance on the decisions WP might be making makes sense and agree with your proposed requirements.

I also agree that changing the default autoload param value from "yes" to null makes the most sense. While null can in some cases evaluate to false, I agree that it is best interpreted here as a neutral value and better aligns with the expectation that not passing an explicit value _could_ result in the option NOT being autoloaded.

I've got nothing left to add for now 😄

[pbearne]

I have updated the pull request to match the new values

I belive I have made all the changes

A few outstanding questions regarding the current approach:

• The PR currently passes the serialized option value to the new function to determine the autoload value, and from there it gets passed to the new filters as well. Is that a good decision? I realize the current use-case only requires the serialized data, but I find this a bit of an odd choice for an API. I think it would be cleaner to pass the regular option value. If we want to avoid having to call maybe_serialize() again, maybe we should simply pass $value _and_ $serialized_value to the function?
• There are now 4 values that need to be autoloaded, on (explicit), yes (legacy), auto-on ("on" but determined by default heuristics), and auto (no clear decision made by default heuristics, but still needs to be autoloaded for backward compatibility alone). IMO it's error-prone to have to remember those individual values everywhere (already happened a few times during code review), which is also not great from an extender point of view (thinking about the few niche plugins that do something specifically about autoloading options). Maybe we should have a function like wp_autoload_values_to_autoload() or something like that that returns the array of those values? Maybe there's a better function name, but I think this would be useful for both core and relevant plugins.

@joemcgill @azaozz @pbearne Please let me know what you think about these points. It would also be great to get another review from you on the approach (and the PR implementing it of course).

A few outstanding questions regarding the current approach:

• The PR currently passes the serialized option value to the new function to determine the autoload value, and from there it gets passed to the new filters as well. Is that a good decision? I realize the current use-case only requires the serialized data, but I find this a bit of an odd choice for an API. I think it would be cleaner to pass the regular option value. If we want to avoid having to call maybe_serialize() again, maybe we should simply pass $value _and_ $serialized_value to the function?
• There are now 4 values that need to be autoloaded, on (explicit), yes (legacy), auto-on ("on" but determined by default heuristics), and auto (no clear decision made by default heuristics, but still needs to be autoloaded for backward compatibility alone). IMO it's error-prone to have to remember those individual values everywhere (already happened a few times during code review), which is also not great from an extender point of view (thinking about the few niche plugins that do something specifically about autoloading options). Maybe we should have a function like wp_autoload_values_to_autoload() or something like that that returns the array of those values? Maybe there's a better function name, but I think this would be useful for both core and relevant plugins.

@joemcgill @azaozz @pbearne Please let me know what you think about these points. It would also be great to get another review from you on the approach (most comprehensively outlined in ​#5671 (comment)), and the PR implementing it of course.

I agree with both points

[pbearne]

made the changes

[pbearne]

I have fixed the tests / wp_load_alloptions() function

let's get this marked as early for 6.6

[flixos90]

The PR ​https://github.com/WordPress/wordpress-develop/pull/5671 looks almost good to go IMO. Marking this for 6.6 early to get some extra time for potential quirks to address.

@joemcgill Curious to get your feedback on ​https://github.com/WordPress/wordpress-develop/pull/5671#discussion_r1522049679. This is a bit ugly from an API perspective (we shouldn't "support" unsupported values), but simple enough and it'll be good from a backward compatibility perspective. For example, if a plugin passes something like 1 today, I'd argue it makes sense to continue considering this as an explicit 'yes' as it is evaluated today, purely for BC. Also see my comment ​https://github.com/WordPress/wordpress-develop/pull/5671#discussion_r1522051375 which was what originally pointed me to this potential problem.

Let me know what you think.

@felixarntz I left feedback on ​your comment in ​my PR review. But since that doesn't show up in Trac, I'll summarize:

Technically speaking, I agree but I think it would be better to leave things as is for now, rather than creating the opportunity for unexpected values to be supported as "true" and wait for real bug reports to determine if we need to handle other values.

@joemcgill Replying to your feedback:

Technically speaking, I do agree with @felixarntz's concern ​here, though I think it would be better to leave things as is for now, rather than creating the opportunity for unexpected values to be supported as "true" and wait for real bug reports to determine if we need to expand the checks in that switch statement.

I would generally agree with your assessment, but leaving this out actually breaks previously existing unit tests. That to me is a reasonable indicator that we need to have this condition to convert technically unsupported values to true for BC. It doesn't really change whether or not this is a theoretical concern, but we previously added tests ensuring those unsupported values evaluate as true, so I would feel uneasy about removing those tests.

LMKWYT.

Ah, I had missed that there were previous tests breaking. In that case, I’d prioritize ensuring previous tests still pass, though it’s probably worth reviewing why the tests were written the way they were to see if they are unnecessarily broad

@joemcgill Looking at the source of those tests, they are quite old, coming from https://core.trac.wordpress.org/ticket/31119 / https://core.trac.wordpress.org/changeset/31278.

Since at that time a boolean false ended up as 'yes', which certainly makes no sense, these tests were added which additionally assert that pretty much anything else does end up as 'yes'.

I don't feel strongly about the course of action here either way. I don't think there's realistically a great danger of changing those tests, because it's unlikely values like array() are passed to this function - why would someone do that? :) As always, I'm sure it happens but it's probably irrelevant talking at scale. More importantly, if we removed the BC handling, such an option would typically end up as auto, which would result in it still being autoloaded. So the only real "risk" from that change would be that the value is no longer considered explicit and thus could be overwritten by core at some point.

Concluding, I'd be okay removing the BC handling and adjusting the relevant tests to assert 'auto' instead of 'yes'. Let me know if you're still onboard with that given the additional context.

@felixarntz I think I'm missing something. Looking at this PR, the ​test cases you're referencing don't seem to be changed (besides changing yes/no to on/off).

@joemcgill That's my point. They aren't changed right now because the PR includes the extra BC handling. But if I remove that, they'll fail, so they'd need to be changed to expect 'auto' instead of 'on' ('yes' in legacy).

@felixarntz

That's my point. They aren't changed right now because the PR includes the extra BC handling.

That's what I missed 😄 — I thought you were asking whether we should _add_ some BC handling to this PR, not whether we should _remove_ the BC handling that was already put in place. Thanks for clarifying.

I actually think I prefer the changes you've suggested above to handle unsupported values as if they were null and have them processed using the default logic (which functionally will still cause them to be autoloaded, so it's not really a BC). It could also be helpful to add a doing_it_wrong() where you previously had the BC logic to encourage developers to update their code to supported values.

[flixos90]

With the PR looking great and having two approvals, I'm planning to commit this soon, so that it can get a good amount of testing prior to the 6.6 release. Flagging this here for potential additional feedback, though we could also make minor changes as a follow up.

[flixos90]

In 57920:

Options, Meta APIs: Use more sensible default for autoloading options which allows WordPress core to make a decision.

An excessive amount of autoloaded options is a common cause for slow database responses, sometimes caused by very large individual autoloaded options. As it is not mandatory to provide an autoload value when adding an option to the database, it tends to be ignored, which in combination with a default value of "yes" and lack of documentation can lead to the aforementioned problem.

This changeset enhances the option autoloading behavior in several ways:

• Update the function documentation to encourage the use of boolean true or false to explicitly provide an autoload value for an option.
• Use new string values on and off for explicitly provided values stored in the database, to distinguish them from yes and no, since yes does not allow determining whether it was set intentionally by the developer or only as a default.
• Effectively deprecate the values yes and no. They are still supported for backward compatibility, but now discouraged.
• Use null as new default autoload value for add_option(). If the developer does not provide an explicit value, this will now trigger WordPress logic to determine an autoload value to use:
  • If WordPress determines that the option should not be autoloaded, it is stored in the database as auto-off. As part of this changeset, the single heuristic introduced for that is to check whether the option size is larger than a threshold of 150k bytes. This threshold is filterable via a new wp_max_autoloaded_option_size filter.
  • If WordPress determines that the option should be autoloaded, it is stored in the database as auto-on. No logic to make such a decision is introduced as part of this changeset, but a new filter wp_default_autoload_value can be used to define such heuristics, e.g. by optimization plugins.
  • If WordPress cannot determine whether or not to autoload the option, it is stored in the database as auto.
  • This effectively means that any option without an explicit autoload value provided by the developer will be stored with an autoload value of auto, unless the option's size exceeds the aforementioned threshold. Options with a value of auto are still autoloaded as of today, most importantly for backward compatibility. A new function wp_autoload_values_to_autoload() returns the list of autolaod values that dictate for an option to be autoloaded, and a new filter wp_autoload_values_to_autoload can be used to alter that list.

These behavioral changes encourage developers to be more mindful of autoloading, while providing WordPress core and optimization plugins with additional control over heuristics for autoloading options where no explicit autoload value was provided.

At the same time, the changes are fully backward compatible from a functionality perspective, with the only exception being that very large options will now no longer be autoloaded if the developer did not explicitly request for them to be autoloaded. Neither WordPress core nor plugins are able to override an explicitly provided value, which is intentional to continue giving developers full control over their own options.

Props pbearne, flixos90, joemcgill, azaozz, spacedmonkey, swissspidy, mukesh27, markjaquith.
Fixes #42441.

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

[joemcgill]

In 58105:

Options: Update default autoload values used in core.

This updates the values used for the $autoload parameter in various functions to replace 'yes' and 'no' with 'on' and 'off', respectively.

Follow-up to [57920].

Props pbearne, mukesh27, joemcgill.
Fixes #61045. See #42441.

[peterwilsoncc]

Reopening for 42441-esc-sql.diff​ to add a little defensive coding to the autoload database query.

While it's safe now, let's not trust our future selves to catch it if the API is modified to allow developers to add values.

Trac ticket:

Lower the priority of when wp_filter_default_autoload_value_via_option_size() is hooked to the new wp_default_autoload_value filter so that third-party developers can filter the large option size value on the default priority.

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

[joemcgill]

@peterwilsoncc 42441-esc-sql.diff looks like a good suggestion to me. Converted it into the previously linked so CI could run. :thumbsup: to commit.

Unrelated, I like your suggestion in the dev-note draft that we should move the wp_filter_default_autoload_value_via_option_size filter on wp_default_autoload_value to a lower priority so it can be overridden using the default priority, so I opened an additional PR for that change.

[peterwilsoncc]

In 58380:

Options, Meta APIs: Add SQL escaping to query loading 'all options'.

Escapes the return value of wp_autoload_values_to_autoload() for use in the database query loading 'all options'. This is a hardening fix to protect against future changes to the options API which may allow developers to further customize the return value of the wp_autoload_values_to_autoload filter.

Follow up to [57920].

Props peterwilsoncc, joemcgill.
Fixes #42441.

Committed r58380 / ​https://github.com/WordPress/wordpress-develop/commit/9955b87bcef14b69f66fe18ff336aa5fbba76f25

[peterwilsoncc]

In 58381:

Options, Meta APIs: Lower priority of default autoload threshold filter.

Lowers the priority at which wp_filter_default_autoload_value_via_option_size() is registered to run on the wp_default_autoload_value() filter. The default filter now runs at priority 5.

This is to allow third party developers to modify whether an option is autoloaded using the default priority, 10, rather than require they register their code to run at a higher priority.

Follow up to [57920].

Props peterwilsoncc, joemcgill.
Fixes #42441.

Committed r58381 / ​https://github.com/WordPress/wordpress-develop/commit/48497199e15c024fadca0b7af93982335ae43f30

[joemcgill]

In 58416:

Options: Fix some default autoload values used in core.

This fixes some autoload values that were updated in [58105] that used the database values of "on" and "off" instead of the boolean values true and false when being passed to add|update_option().

Props joemcgill, desrosj, rajinsharwar.
Fixes #61045. See #42441.

[Cybr]

How much time is saved by unloading large options? How long do the extra database requests take for the unloaded options? Does the benefit of unloading outweigh the adverse effects of additional database requests?

Also, why was 150kB chosen? Why not 200kB or 50kB?