Make WordPress Core

Opened 8 months ago

Closed 7 months ago

Last modified 6 months ago

#58964 closed enhancement (fixed)

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

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch, has-unit-tests, has-dev-note
Focuses: performance Cc:

Description

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.

Change History (29)

#1 @SergeyBiryukov
8 months ago

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.

#2 follow-up: @flixos90
8 months ago

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?

#3 in reply to: ↑ 2 @SergeyBiryukov
8 months ago

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 :)

#4 @mukesh27
8 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.4

This ticket was discussed during the Performance bug scrub.

Move to 6.4 consideration.

Props to @joemcgill

This ticket was mentioned in PR #5064 on WordPress/wordpress-develop by @tabrisrp.


8 months ago
#5

  • Keywords has-patch has-unit-tests added; needs-patch removed

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

#6 @tabrisrp
8 months ago

  • Keywords dev-feedback added

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!

This ticket was mentioned in PR #5069 on WordPress/wordpress-develop by @flixos90.


8 months ago
#7

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).

#8 @flixos90
8 months ago

  • Keywords dev-feedback removed

@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.

#9 @flixos90
8 months ago

@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 :)

#10 follow-up: @tabrisrp
8 months ago

@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?

#11 in reply to: ↑ 10 @flixos90
8 months ago

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.

#12 @boonebgorges
8 months ago

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.

(accidental ticket reference)


8 months ago
#13

#14 @flixos90
8 months ago

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.

@flixos90 commented on PR #5069:


8 months ago
#15

@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.

@flixos90 commented on PR #5069:


8 months ago
#16

@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.

#17 @flixos90
8 months ago

  • Keywords needs-dev-note added
  • Owner set to flixos90
  • Status changed from new to assigned

@flixos90 commented on PR #5104:


8 months ago
#18

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.

@youknowriad commented on PR #5104:


8 months ago
#19

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

@flixos90 commented on PR #5069:


8 months ago
#20

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

@gziolo commented on PR #5104:


8 months ago
#21

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

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/da014395-dc3f-4221-85e5-d723fc090eff

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

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/956ef1e8-a549-470b-954f-be514db08295

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

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/409a0656-9e5f-4ec5-b003-ffc6bdf8c3dd

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

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/10667ace-56b3-4ade-8fdf-fab23fc44a2f

Everything works as expected in the editor though.

@flixos90 commented on PR #5104:


8 months ago
#22

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.

@flixos90 commented on PR #5104:


8 months ago
#23

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.

joemcgill commented on PR #5104:


7 months ago
#24

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?

@flixos90 commented on PR #5104:


7 months ago
#25

@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.

#27 @flixos90
7 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#29 @flixos90
6 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.