Introduce dedicated function to set autoload value of an option already in the database

[flixos90]

As outlined with more context in #42441 and #58962, "excessive autoloading" (i.e. autoloading too many options) can lead to performance problems, in some cases huge ones that even cause bugs.

Part of the problem comes from options which are autoloaded unnecessarily, often from stale plugins that were once used on a site but then deactivated or deleted. While plugins are encouraged to provide uninstaller routines, deleting all plugin data in itself is problematic in the way that WordPress handles it (see e.g. #20578 and #31136), and even then it's not always desired by the end user. However, there is an argument to make that, as soon as a plugin gets deactivated, the options that it has added make no sense to be autoloaded anymore, as they will not be in use at all.

A limitation with that is today WordPress only allows to modify the autoload field of an option by also updating (or adding) the option value, which is not exactly useful in this context. Therefore this ticket proposes adding a new function (or potentially set of functions) specifically to set the autoload value of an option - for example something like wp_set_option_autoload( $option, $autoload ). This provides a single-purpose low-level function that fits directly with other WordPress option functions.

A somewhat related proposal is found in #55584, however there is an argument to make that the Settings API is not the right approach for that (see #55942), since not every plugin using options is using or has to use the Settings API.

[SergeyBiryukov]

Related/duplicate: #48393.

@herregroen in comment:2:ticket:48393:

I think changing update_option so that autoload is updated even if the value is unchanged would be by far the most intuitive and elegant way of doing things. Of course if neither the value nor the autoload property have been changed then nothing should happen.

[flixos90]

Thanks @SergeyBiryukov, great pointer!

I like the proposal on that ticket, I would actually propose that they should coexist though, and #48393 should use what's proposed here: I think implementing a single-purpose function to update the autoload value provides most flexibility, and then update_option() could use that function internally if the value of the option matches but the autoload value does not.

The function here would still offer support for additional use-cases, for example: A plugin could implement logic in their deactivation hook that sets all its options to autoload=no, and then logic in the activation hook to reconfigure the autoload value to be as intended.

WDYT?

[SergeyBiryukov]

Replying to flixos90:

I like the proposal on that ticket, I would actually propose that they should coexist though, and #48393 should use what's proposed here: I think implementing a single-purpose function to update the autoload value provides most flexibility, and then update_option() could use that function internally if the value of the option matches but the autoload value does not.

Sounds good :)

[mukesh27]

This ticket was discussed during the Performance bug scrub.

Move to 6.4 consideration.

Props to @joemcgill

Add a new function wp_set_option_autoload( $option, $autoload ) to modify the autoload value of an option without having to modify its value.

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

[tabrisrp]

I started a PR to add the new function, with unit tests associated.

I did not yet do changes to update_option() because I wanted to discuss how best to implement that, as currently the function uses get_option() to get the old value, but there is nothing to get the old autoload value (and I don't think there is anything existing to get a specific option autoload value?).

We could add an additional SQL request to get the option autoload value, but this has performance implications. I could also see the possiblity of changing the code to get the old value & autoload value at the same time, so feedback on the different possibilities would be appreciated!

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

This PR introduces wp_set_option_autoload() and wp_set_options_autoload() functions, which allow updating the autoload value for an option, or multiple using a single DB request.

Both functions also handle updating the respective caches accordingly (alloptions vs individual option cache).

[flixos90]

@tabrisrp Thank you for the PR. Too bad we both worked on this apparently at the same time 🙃

I also opened one in ​https://github.com/WordPress/wordpress-develop/pull/5069, which additionally implements a function to do the same for multiple options at the same time in a single DB request. I consider that useful particularly in plugin activation or deactivation hooks to bulk-modify the autoload value for options.

I'm happy for either of us to continue the work, there's probably some bits in both PRs that should be used and combined. For example, my PR is missing the wp_protect_special_option() calls, while your PR is missing the logic to only update the DB if necessary, and to update the caches.

[flixos90]

@boonebgorges I know you're not really active in Core at this point, but just in case you see this and are interested, I'd love to get your thoughts on this :)

[tabrisrp]

@flixos90 Unlucky! Feel free to take what you need from my PR and add it to yours, as yours is more complete I think it's the best way to go.

Do you have any opinion on the update_option() part?

[flixos90]

Replying to tabrisrp:

Do you have any opinion on the update_option() part?

I agree we can leave that change out of here and separately consider/implement it later as part of #48393.

[boonebgorges]

Thanks for the ping @flixos90 ! It was definitely an oversight in [31640] that autoload would not be updated if the option value was unchanged, so I'm very much in favor of correcting that behavior.

Introducing separate functions for this purpose seems OK to me.

Your PR doesn't make the necessary changes to update_option(), as you note in https://core.trac.wordpress.org/ticket/58964#comment:11. I'm not certain about the best way to implement this without messing with the return semantics of update_option(). Currently we return false if the new value is the same as the old. But on the proposal, update_option() would update the value if only autoload changed, which means we either have to (a) return true, which is a sort of change in behavior, or (b) return false, which is semantically incorrect.

[flixos90]

Thank you for the prompt feedback @boonebgorges! I've shared it in https://core.trac.wordpress.org/ticket/48393#comment:11, which is where that discussion is mainly being held. I've also replied to your feedback on the PR.

@joemcgill In ​https://github.com/WordPress/wordpress-develop/pull/5069/commits/40714a48d9780f435ba6bd947343b84f176c3024, I have updated wp_set_option_autoload() to become a wrapper function for wp_set_options_autoload(). That makes a lot of sense and reduces the amount of complex code added quite a bit.

@Tabrisrp In ​https://github.com/WordPress/wordpress-develop/pull/5069/commits/8ba3b09286bbe3fe9ecab2209539b96a0a8b98eb, I've added the call to wp_protect_special_option() as you had it in your alternative PR #5064.

Thanks @youknowriad!

This looks good to me. Is this dependent on the Gutenberg fix? Should we be waiting for the package update first before committing?

No, in fact it would be better to first commit this one before backporting the other Gutenberg fix, see https://core.trac.wordpress.org/ticket/58154#comment:15.

This looks good to me. Is this dependent on the Gutenberg fix? Should we be waiting for the package update first before committing?

@boonebgorges @joemcgill This is now ready for another full review.

I did quite extensive testing using the Gutenberg plugin version downloaded from the linked PR:

​https://github.com/WordPress/gutenberg/actions/runs/6003053680

​https://github.com/WordPress/gutenberg/suites/15571281198/artifacts/888604674

I identified some edge cases in the block-based theme that I believe we saw together with @felixarntz last week during WC US:

Homepage rendered on the front

Everything works here as expected – "Third post with loop" renders the same loop as the one on the homepage and it correctly skips rendering the post content, which would lead to infinite loop.

Blog Home template in the editor

For some reasons, the nested query loop doesn't get displayed, but that might be good in this context.

Third post with loop on the frontend

The debugging message is only visible when WP is in the debug mode. More importantly, the query loop isn't correctly rendered.

Third post with loop in the editor

Everything works as expected in the editor though.

Thank you for the detailed testing @gziolo! Some follow up feedback on your notes:

• I think your observations on the "third post with loop" aren't quite right: on the frontend, when viewed as the singular post in TT3 theme, its query loop only displays itself, which is correct, since the global query loop on that URL is only that post. I don't think there's any real use-case for doing that anyway, but it's working as it should. If anything, that part is off in the editor, where it should also display just that post, not the list of all posts. It should only display all the posts when looking at the post in an archive. Basically what is displayed in that post will differ depending on the current main query loop.
• Regarding your observation in the TT1 theme, as you've already suspected, in the archive view the loop inside "third post with loop" is not displayed as that template is only using the excerpt where any "rich content" is cut.

Thanks @gziolo. Based on the findings, I also left an update on ​https://github.com/WordPress/gutenberg/issues/50790: I don't think it makes sense that we even allow inheriting the main query inside a specific post.

A new concern I have after thinking about this some more, is that it essentially forces all singular block templates to include the entire template in "the loop", which can cause inconsistent behavior for things like headers or other template parts that are traditionally outside the loop. For example, if we have some logic that attempts to apply some image optimizations only when the image is inside the loop, with this change, a header image would be included on singular pages but not on list pages like archives. This is very edge case, but could be confusing. Would it be safer to only apply this logic to the full template if we are able to first do a has_block() check to see if a theme has omitted one of the blocks that would start the loop as expected?

@joemcgill Your concern is a fair point, and I've considered that initially when I started this PR.

Would it be safer to only apply this logic to the full template if we are able to first do a has_block() check to see if a theme has omitted one of the blocks that would start the loop as expected?

I did have that in here at the start, but there are several limitations with that:

• There is no existing core block that would realistically be used on a singular template to trigger the loop. I thought originally that the core/query and core/post-template loop would also be usable for that purpose, but they unfortunately include additional markup _forcing_ posts to be in a semantic list, which makes no sense for singular content and can be problematic for accessibility.
  • Figuring this out would require notably more discussion and changes. For example, would we introduce a new block for singular loops? Or somehow adjust the core/post-template block to conditionally not have wrapping markup? Those things may end up quite confusing from an end user perspective too.
• Even if there were singular-friendly variant of these blocks, we would need something more complex than has_block(), because we would only _not_ need to trigger the main query if the core/post-template block was wrapped in a core/query block that had the inherit: true attribute set - something more costly to check for which there's no dedicated function available.

While what you're raising is indeed a potential concern, I see it more as a theoretical concern. While typically themes indeed only have the loop around the main content of a singular URL, I can't think of any actual problem that wrapping the whole content of such a URL would cause.

In other words: I think this is something worth revisiting in the future, but I don't consider it a blocker, since the current issue is certainly more severe, and properly fixing the additional concern would require more discussion and effort than what we could realistically tackle for 6.4.

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

[flixos90]

In 56508:

Options, Meta APIs: Introduce wp_set_option_autoload_values().

This function accepts an associative array of option names and their autoload values to set, and it will update those values in the database in bulk, only for those options where the autoload field is not already set to the given value.

Two wrapper functions for ease of use accompany the new main function:

• wp_set_options_autoload( $options, $autoload ) can be used to set multiple options to the same autoload value.
• wp_set_option_autoload( $option, $autoload ) can be used to set the autoload value for a single option.

All of these functions allow changing the autoload value of an option, which previously has only been possible in combination with updating the value. This limitation prevented some relevant use-cases: For example, a plugin deactivation hook could set all of its options to not autoload, as a cleanup routine, while not actually deleting any data. The plugin's activation hook could then implement the reverse, resetting those options' autoload values to the originally intended ones for when using the plugin.

Props boonebgorges, joemcgill, costdev, mukesh27, SergeyBiryukov, tabrisrp, flixos90.
Fixes #58964.

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

[flixos90]

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