Make WordPress Core

Opened 10 months ago

Closed 7 months ago

Last modified 7 months ago

#58962 closed enhancement (fixed)

Provide a way to load multiple specific options with a single database request

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 dev-reviewed
Focuses: performance Cc:

Description

WordPress's get_option() function generally relies on making individual database requests for each option, however with the majority of options (in most cases) being autoloaded, i.e. fetched once with a single database request and then stored in (memory) cache.

The option autoloading approach has been helpful overall in minimizing database requests, however for many sites "excessive autoloading" (i.e. autoloading too many options) has led to problems, where lots of options are loaded in the frontend that are never used during these pageloads, which slows down load time performance. Sometimes the issue is even worse to the degree that certain object cache solutions cannot store the large amount of data anymore, which can lead to extremely slow performance due to the bug.

Part of the problem is lack of documentation around option autoloading, its implications and best practices. For example, options that are only used in very specific contexts, e.g. a specific WP Admin screen, should preferably not be autoloaded. Though as of today, while preferable from an overall performance perspective, there is a lack of alternatives for use-cases where multiple options are still needed: For example, if a plugin needs to load 5-10 options on their own WP Admin screen, simply recommending to not autoload them would lead to 5-10 database requests on that WP Admin screen, i.e. unnecessarily slowing down performance of that screen.

Therefore this ticket aims to provide a way to load (or prime in cache) multiple specific options with a single database request. This can be used as a reasonable alternative for use-cases like the above, encouraging to not use general option autoloading as a bandaid for the lack of another efficient way to load multiple options.

Change History (64)

#1 @flixos90
10 months ago

  • Keywords needs-patch needs-unit-tests added

To address this problem, we have holistically two options: Either we provide a way to directly retrieve multiple options at once, e.g. a get_options( $options ) function. Or we stick to focusing on the existing get_option() for actually retrieving options, but provide a function to prime specific options, i.e. load them into the cache with a single database request - similarly to the autoloaded options ("alloptions") query, but for a specific set of options.

I would propose the following solution to the problem:

  • Implement a function prime_options( $options ) to which developers can pass an array of option names. The function will load all options into the cache with a single database query, except for those options that are already in cache (if any).
  • With this function, a plugin for example could call it when its WP Admin screen is loaded, priming all its options in a single database query. It could then potentially update these options to use $autoload = 'no'. No changes would need to be made to how options are actually fetched, as get_option() would function the same way as before - the values would simply be already in cache if primed before.

The above function would be the central foundation, however we could consider implementing additional "wrapper" functions to facilitate specific use-cases, for example:

  • A prime_options_by_group( $option_group ) may be useful as a wrapper for prime_options(), allowing e.g. a plugin to prime all options registered with a specific option group. This would give the benefit of not having to remember/hard-code all option names in the priming function call, and it would tie in nicely with the existing Settings API, where option names have been a required parameter since its origin. The function would effectively get all option names in the given group and then call prime_options( $options ) with them.
  • A get_options( $options ) function may be useful to actually retrieve multiple options in one go. The function would effectively call prime_options( $options ) and then get_option( $option ) for each of them, returning an array of key value pairs.

While we could also implement a standalone get_options( $options ) function as an alternative (i.e. not using another function to prime options), I consider that approach overly limiting, as it would require to basically duplicate lots of the get_option() function's code, and it would be easy to miss some nuances. So focusing the new functionality on priming options while keeping the foundation for retrieving options untouched makes more sense IMO.

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


10 months ago
#2

  • Keywords has-patch added; needs-patch removed

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

This pull request adds new functions as described in the issue description. Once we agree with the functions' code and signature, I will add unit tests.

  • [ ] Add tests to cover the change.

#3 @mukesh27
10 months ago

Thanks @flixos90 for the details. I have raise PR that add requested functions.

@mukesh27 commented on PR #4970:


10 months ago
#4

@felixarntz PR is ready for review.

#5 @mukesh27
10 months ago

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

@mukesh27 commented on PR #4970:


10 months ago
#6

Thank you, @felixarntz, for the detailed code review and unit tests assessment. I have addressed those in my recent commit.

@costdev, I appreciate your explanation on Slack regarding the points I had misunderstood. 🦸‍♂️

@mukesh27 commented on PR #4970:


9 months ago
#7

Thank you, @felixarntz, for addressing the questions posed by @joemcgill. I concur with your responses. Joe, could you kindly review them when you have a moment today, so that we may proceed with the commit. Many thanks!

@mukesh27 commented on PR #4970:


9 months ago
#8

Thanks @joemcgill and @felixarntz 👏

#9 @flixos90
9 months ago

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

In 56445:

Options, Meta APIs: Introduce prime_options() to load multiple options with a single database request.

WordPress's get_option() function generally relies on making individual database requests for each option, however with the majority of options (in most cases) being autoloaded, i.e. fetched once with a single database request and then stored in (memory) cache.

As part of a greater effort to reduce the amount of options that are unnecessarily autoloaded, this changeset introduces an alternative way to retrieve multiple options in a performant manner, with a single database request. This provides a reasonable alternative for e.g. plugins that use several options which only need to be loaded in a few specific screens.

Specifically, this changeset introduces the following functions:

  • prime_options( $options ) is the foundation to load multiple specific options with a single database request. Only options that aren't already cached (in alloptions or an individual cache) are retrieved from the database.
  • prime_options_by_group( $option_group ) is a convenience wrapper function for the above which allows to prime all options of a specific option group (as configured via register_setting()).
  • get_options( $options ) is another wrapper function which first primes the requested options and then returns them in an associative array, calling get_option() for each of them.

Props mukesh27, joemcgill, costdev, olliejones.
Fixes #58962.

#11 @flixos90
9 months ago

  • Keywords needs-dev-note added

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 months ago

#13 @flixos90
8 months ago

  • Keywords has-dev-note added; needs-dev-note removed

#14 @peterwilsoncc
8 months ago

  • Keywords changed from has-patch, has-unit-tests, has-dev-note to has-patch has-unit-tests has-dev-note

@flixos90 Sorry to do that thing I do but I didn't see the PR at the time.

All the other cache priming functions following the naming convention of _prime_thing_caches:

  • _prime_post_caches
  • _prime_post_parent_id_caches (new in 6.4)
  • _prime_site_caches
  • _prime_term_caches
  • _prime_comment_caches
  • _prime_network_caches

The other various functions that modify a cache in some way end in _caches too, update_post_caches, update_post_author_caches, etc.

While it's possible, I think it best if the new option priming functions follow the same convention.

#15 @flixos90
8 months ago

Thanks @peterwilsoncc. While I don't think we should use an underscore prefix for this function (since it's clearly not for core's private use), I would be open about renaming prime_options() to prime_option_caches(). On the one hand, I find the first name easier to remember, and the notion of caching in the name may make it more complicated to understand, as this function would be encouraged even for developers that may not be very familiar with caching - as it's about options more than it is about caching. On the other hand, the latter name would be more precise.

I would like to get other people's thoughts on the potential renaming.

#16 @ReneHermi
8 months ago

I like the purpose of prime_options but find the name prime_options kind of too fancy, complex, and confusing as prime has multiple meanings, especially when not used in caching context and when the verb prime does not stand beside cache.

By just reading the name and not the code I would never expect this function has the purpose of priming a cache.

A few more suggestions:

save_options_cache,
update_options_cache
cache_options
set_options_cache

But to be consistent with the other internal prime* methods and to shift prime into the caching context, I vote for prime_options_cache and go with @peterwilsoncc

Although, the other suggestions could be slightly more self-explanatory and even shorter but there is no need to get too deep into naming discussions.

even for developers that may not be very familiar with caching

I claim every *developer* is aware at least of the basic concept of caching.

Btw. Even chatGPT does not understand the meaning of prime_options:

There is a function named prime_options. What do you think this function does?

Based on the name "prime_options," I can make an educated guess about its functionality, but without a concrete context or the actual function definition, it's speculative. Here's what I infer:

Prime Numbers: The function might deal with prime numbers in some capacity. For example:

It might return a list of prime numbers up to a given limit.
It might check if a given number is prime.
It might generate the next prime number after a given number.
Options or Choices: The word "options" suggests some choices or configurations.

The function might return prime-related configurations or parameters.
It could possibly take a number as an argument and return its prime factors.
It might provide multiple methods or algorithms for determining primes and allow a user to select one.
Interactivity: If it's meant for a user interface or a command-line tool, "options" might refer to various choices a user has related to prime numbers. For instance:

"Would you like to find all primes up to X or just check if X is prime?"
Combinations: Maybe it deals with combinations of prime numbers, or ways in which primes can be combined or used in particular scenarios.

To provide a definitive answer, I'd need the actual code or at least a more detailed description of the function. If you have the function's definition or its documentation, please share, and I'll be happy to help further!
Last edited 8 months ago by ReneHermi (previous) (diff)

#17 follow-up: @peterwilsoncc
7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

While I don't think we should use an underscore prefix for this function (since it's clearly not for core's private use), I would be open about renaming prime_options() to prime_option_caches().

@flixos90 Re the underscore: I'm not sure what to do there now the cache priming functions have been made public. Go with consistency or try to move on from the errors of the past.

I'll reopen this so the discussion is more visible and it can always be closed again if it's decided not to rename. For the functions I suggest:

  • (_)prime_options_cache()
  • (_)prime_options_cache_by_group()

#18 in reply to: ↑ 17 @joemcgill
7 months ago

Replying to peterwilsoncc:

I'm not sure what to do there now the cache priming functions have been made public. Go with consistency or try to move on from the errors of the past.

These new function seem to be to serve the exact same purpose of other cache priming functions, so I would vote we go with consistent naming, including the use of the underscore prefix, as @peterwilsoncc suggests.

#19 follow-up: @flixos90
7 months ago

I'm onboard with the prime_options_cache() name (or prime_option_caches()), but I strongly object to prefixing the function with an underscore, which is a clear indicator of private functions, which this function is not and was never intended to be.

The existing functions to prime caches started as private functions, and I would argue even today they are primarily used by WordPress core. That is different with the functions here which are primarily introduced for plugins to make use of them.

Prefixing a public function just for consistency sake with existing private functions which may now be considered public is taking the point of consistency too far IMO.

Last edited 7 months ago by flixos90 (previous) (diff)

#20 @hellofromTonya
7 months ago

Hello all,

@peterwilsoncc pinged me for another opinion.

I'm all for consistency +1

However, @flixos90 identifies a difference with the functions in this PR versus the other internal (Core-only) priming functions:

That is different with the functions here which are primarily introduced for plugins to make use of them.

The underscore _ prefix is a common technique to further identify internal only usage, i.e. not for public usage.

As these functions are (or also) for plugins to use, they are public, not private Core-only priming functions. IMO they should not be prefixed with _.

Another concern I'd like to voice - the DevNote has been published. Could extenders have already modified their code to use the functions? If yes, consideration is needed for how to alert them to the rename changes to avoid potential fatal errors in their usages.

cc @flixos90 @joemcgill @peterwilsoncc

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 months ago

#22 follow-ups: @joemcgill
7 months ago

I understand @flixos90 objection to using the underscore here, given the intent of these functions. For that reason, even though they serve a very similar purpose to the other _prime_{thing}_caches() functions, I think avoiding the confusion altogether would be better. I'd even avoid adding _caches the end of the function names. To make the distinction and intent even more clear, I would consider renaming these to

  • wp_load_options()
  • wp_load_options_group()

While they still serve the purpose of loading and caching a set of options, these names would avoid the confusion with internal prime_ functions and would more closely align with other option loading functions, wp_load_alloptions() and wp_load_core_site_options().

Given that these are still unreleased functions, I'd prefer the minor inconvenience of needing to update the dev note over being stuck with inconsistent function names.

#23 in reply to: ↑ 22 @flixos90
7 months ago

Thanks @hellofromTonya @joemcgill.

Replying to joemcgill:

To make the distinction and intent even more clear, I would consider renaming these to

  • wp_load_options()
  • wp_load_options_group()

I like the idea of separating the names more obviously. Only one suggested change: The second should remain ...options_by_group() IMO because it's loading options, not an option group. The two functions are conceptually doing the same thing, and the latter is a convenience wrapper. So IMO we shouldn't change the noun/object of what is loaded. If there is a concern about the "by", maybe we could use "for" instead?

The other potential concern is that existing option functions don't use the wp_ prefix, but I'm not opposed to introducing it here - also since the option autoload functions from #58964 already went with wp_ anyway. Just sharing that as a further naming consideration, but I'm fine with having the wp_ prefix on them.

Last edited 7 months ago by flixos90 (previous) (diff)

#24 @joemcgill
7 months ago

wp_load_options_by_group() sounds good to me. I think adding the wp_ prefix is best practice for any new public functions at this point, even if some of the most basic original functions to get/update/delete objects omit the prefix. It also makes it very obvious that these are meant to be used by extenders and not primarily as core internals.

#25 in reply to: ↑ 22 @hellofromTonya
7 months ago

Replying to joemcgill:

To make the distinction and intent even more clear, I would consider renaming these to

  • wp_load_options()
  • wp_load_options_group()

While they still serve the purpose of loading and caching a set of options, these names would avoid the confusion with internal prime_ functions and would more closely align with other option loading functions, wp_load_alloptions() and wp_load_core_site_options().

Yes, good idea Joe. The term "prime" is an internal term. And you're right, these new functions do align well to wp_load_alloptions() and wp_load_core_site_options(), which also load and prime caches.

I think adding the wp_ prefix is best practice for any new public functions at this point, even if some of the most basic original functions to get/update/delete objects omit the prefix. It also makes it very obvious that these are meant to be used by extenders and not primarily as core internals.

I agree. Also the prefixing with wp_ avoids potential naming conflicts in the wild.

Overall

  • wp_load_options()
  • wp_load_options_by_group()

I agree with adding the wp_ prefix and the renaming to better align with existing functions that load and prime caches. LGTM

#26 @poran766
7 months ago

After reading all the comments, I agree with the latest proposal.
wp_load_options
wp_load_options_by_group
As these functions can be used by plugin developers, using _ might be confusing.

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


7 months ago
#27

Per the latest discussion on the ticket, this PR renames the functions:

  • prime_options() to wp_load_options()
  • prime_options_by_group() to wp_load_options_by_group()

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

#28 @flixos90
7 months ago

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

In 56990:

Options, Meta APIs: Rename prime_options() to wp_load_options().

This clearly separates these functions which are intended to be used by external developers from the existing _prime_*_caches() functions which are primarily intended for internal usage. The term "load" is additionally more accessible than "prime".

This changeset renames the above function, as well as the wrapper function prime_options_by_group() to wp_load_options_by_group().

Props peterwilsoncc, joemcgill, hellofromTonya, poran766, flixos90.
Fixes #58962.

#30 @flixos90
7 months ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to the 6.4 branch.

@flixos90 commented on PR #5548:


7 months ago
#31

@peterwilsoncc I think this is a duplicate of #5554, which I just committed?

#33 @hellofromTonya
7 months ago

In talking with @peterwilsoncc, he clarified that in working with @spacedmonkey that ALL prime cache functions are intended to be publicly used by plugins and themes.

the prime functions are no longer internal. Jonny and I have been making a concerted effort to encourage their use by extenders to imrpove the performance of sites using plugins.

My agreement in comment:25 was predicated on the idea that only these 2 new functions were to be public for plugins to use and that all the other prime functions, using the _prime_{thing}_caches() structure, were for Core-only internal usage.

If indeed all priming functions are for public usage, then the renaming of the 2 functions in this ticket should be reconsidered, minus the wp_ prefix.

Let's continue the discussion before backporting [56990] to 6.4. cc @flixos90 @joemcgill @peterwilsoncc

#34 in reply to: ↑ 19 ; follow-up: @peterwilsoncc
7 months ago

I can be convinced to use wp_ prefix and follow up with deprecation and renaming of other functions in the next release. In which case I'd rename the new function for priming post parent IDs prior to release too.

A lot of plugins are using the existing priming functions, the biggest being Yoast SEO and WooCommerce with 5M+ installs each.

As the purpose of the new functions is to prime caches, I think we need those keywords to be included as part of the effort to encourage plugins to prime caches.

#35 @flixos90
7 months ago

Regardless of whether the existing _prime_*_caches() function are now meant to be used by others or not, I don't think it makes sense to use an underscore prefixed name for a public function, for the reasons outlined above.

The existing functions were already introduced before, and given they are intended to be public now, their names are technically not appropriate. Of course I understand why, it is because the use-case for the function would have changed after their introduction. But that is not the case for new functions we're introducing now. Therefore, I don't think it makes sense to knowingly make a naming "mistake" on a new function when we don't have to. I would support deprecation and renaming of the existing _prime_*_caches() functions for that reason.

Last but not least, using the term "load" as @joemcgill suggested is easier to understand than "prime", and it is aligned with the closely related wp_load_alloptions() and wp_load_core_site_options() functions and their names. In fact, the functions are introduced as a replacement for relying on autoloaded options ("alloptions").

Last edited 7 months ago by flixos90 (previous) (diff)

#36 @joemcgill
7 months ago

  • Keywords dev-reviewed added; dev-feedback removed

My experience is that many of the current priming functions were introduced as internal functions, but (like all WP functions) end up getting used by savvy 3rd party developers regardless, since they’re useful.

These new functions were not created with the intent to make something in core more performant, but as a helper for extenders to more efficiently load a set of options.

I like the latest commit in trunk and for this purpose and think it would be useful for us to consider making more of these types of priming functions public and more well documented as best practices.

#37 in reply to: ↑ 34 @hellofromTonya
7 months ago

Seems there's a difference of opinion as the intent for / purpose of these new functions:

These new functions were not created with the intent to make something in core more performant, but as a helper for extenders to more efficiently load a set of options.

As the purpose of the new functions is to prime caches

Two different views on why these 2 public functions are being introduced into 6.4: loading vs priming.

How to move forward:

Put consistency aside for now as renaming functions to fit a naming pattern should only apply if the function aligns to the group of functions for that naming pattern.

Instead, think about:

  • What the primary purpose is for each function? Name it to tell what it does when invoked.
  • What is the distinct difference being "loading" and "priming"?
    • Can loading functions also prime caches?
    • Can priming functions also load?
    • Which is the primary purpose / behavior?

For example:

  • wp_load_options() loads, primes the cache (with missing options), but does not return any data.
  • wp_load_alloptions() loads, caches, and returns an array of all options.
  • _prime_post_caches() primes the post, postmeta, and object term caches, but does not return any data. Does it load anything?

If there's consensus that its primary job is to:

  • Load, then the new names fit.
  • Prime caches, then the new names need to change.
Last edited 7 months ago by hellofromTonya (previous) (diff)

#38 @flixos90
7 months ago

Following up on @hellofromTonya's questions above:

  • The two new functions are being introduced purely for external (plugin or theme, most likely plugin) developers to load their options in bulk before accessing them, to improve performance over having to load them individually only once they're accessed.
  • One may also argue that these functions are priming the caches for these options, though this is of secondary importance, and really not important (as in, a low level detail) to know for the developer using the function.
  • All they need to know is: Call this function early during the request, with the options from your plugin that you're going to access throughout the request, in order to improve performance.

Given that this has been dev-reviewed by another committer, and in order to prevent inconsistent trunk and 6.4 branches, I will move forward with the backport, and update the dev note accordingly. I think we have exhausted the conversation here and everybody involved shared their perspectives, and they were considered. If there remains any strong objections against the current names that I may have missed, we can always reopen again.

#39 @flixos90
7 months ago

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

In 57004:

Options, Meta APIs: Rename prime_options() to wp_load_options().

This clearly separates these functions which are intended to be used by external developers from the existing _prime_*_caches() functions which are primarily intended for internal usage. The term "load" is additionally more accessible than "prime".

This changeset renames the above function, as well as the wrapper function prime_options_by_group() to wp_load_options_by_group().

Props peterwilsoncc, joemcgill, hellofromTonya, poran766, flixos90.
Merges [56990] to the 6.4 branch.
Fixes #58962.

This ticket was mentioned in Slack in #core-performance by flixos90. View the logs.


7 months ago

#42 @peterwilsoncc
7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I really don't understand the reluctance to use the words prime and cache in a function designed to prime a cache.

As I've said repeatedly, the other cache priming functions are public and have been since 6.1. There is no may about it. Both Yoast SEO and WooCommerce are using them, each of which have over 5M installs (the highest number listed on the plugins directory). It's disingenuous to suggest they are not used by plugin authors. They are.

Re the functions with a similar name:

  • wp_load_alloptions() does not accept arguments regarding which options should be cached
  • wp_load_core_site_options() does not accept arguments regarding which options should be cached
  • _prime_thing_cache(), like these new functions, accepts an array of arguments to define which items should be cached.

All they need to know is: Call this function early during the request, with the options from your plugin that you're going to access throughout the request, in order to improve performance.

I think this is really dismissive of extenders skills. Extenders DO and should know about caching. This is something I and other members of the team have been working on. There may be an opportunity to improve documentation but there have been dev notes about this for many reelases now.

Given that this has been dev-reviewed by another committer, and in order to prevent inconsistent trunk and 6.4 branches, I will move forward with the backport, and update the dev note accordingly. I think we have exhausted the conversation here and everybody involved shared their perspectives, and they were considered. If there remains any strong objections against the current names that I may have missed, we can always reopen again.

I am really disappointed in this action and think it goes well beyond the spirit and intent of double sign off.

Tonya and I are both very experienced committers and were continuing the discussion. Tonya is the tech lead of the current release.

I had chosen not to commit anything while the discussion was ongoing and find it quite disrespectful that you did not observe the same courtesy.

#43 @flixos90
7 months ago

@peterwilsoncc

I am really disappointed in this action and think it goes well beyond the spirit and intent of double sign off.

Tonya and I are both very experienced committers and were continuing the discussion. Tonya is the tech lead of the current release.

I had chosen not to commit anything while the discussion was ongoing and find it quite disrespectful that you did not observe the same courtesy.

I had committed the initial name change before you commented yesterday, so there was no way for me to anticipate you pushing back again. I committed the backport because the other name change was already committed, as we must certainly avoid having differently named functions in trunk vs the current release branch. Until 6.4 is released, none of this is definite. That said, the backport had the double sign off so while I understand that you still do not agree with the current naming, what I did did not go against the principle of double signoff.

In any case: You disagree with the current naming and have presented your rationale, but I disagree with that rationale. We can continue arguing back and forth, but unless any of us can make additional arguments, I don't see the point. Both @joemcgill and I agree with the current naming, and as far as I understand @hellofromTonya was pointing out additional considerations for the naming without strongly leaning in one or the other direction.

While what I'm saying may come across harsh, I am trying to be clear. I am not dismissing your opinion. But sometimes, it can happen that there is no consensus between everyone, and a decision has to be made regardless in order to move forward, either based on a committer majority or a decision by a lead. To map out a worst-case scenario here, it's certainly not worth for instance reverting this entire enhancement over a naming discussion.

#44 follow-up: @joemcgill
7 months ago

@peterwilsoncc I'm really sorry that the double sign-off on this didn't allow you the opportunity to follow up. To add some additional context, I think the rush was mostly due to the pending deadline for the RC2 package that was released earlier today. Speaking personally, I tend to think of the spirit of double sign off as mostly a precaution against introducing late bugs, rather than a way of expressing that an ongoing discussion should be ended. Even so, I'll try to be more cautious about that in the future.

Since you raised the question about appropriate naming again, I went back to research the ticket where all of the existing priming functions were made public, #56386, and admit that I wasn't aware of the whole discussion that happened then. It does seem like similar concerns about the use of underscores in intentionally public functions was raised in that ticket as well and both @desrosj and @SergeyBiryukov agreed that there wasn't a strong need to rename these functions at that time. Perhaps we should reconsider the naming strategy in a future release? In the meantime, I think it would be good to avoid as much churn as possible with the new function names.

These new functions do seem to have the same functional purpose as existing _prime_{thing}_cache functions, as I mentioned in a previous comment. Assuming we can come to agreement on that point, it seems to me like we have 3 options:

  1. Leave things as they currently stand and punt the naming conversation to a future release.
  2. Update these new function names to _prime_options_cache() and _prime_options_cache_by_group(), to be consistent with other cache priming functions in the meantime.
  3. Update the new function names to what we anticipate potentially renaming the other functions.

Given that all of us have been pretty close to this conversation, I think it would be useful to get some outside opinions, if possible.

#45 @flixos90
7 months ago

@joemcgill I like the plan you've outlined. I agree part of this being more in a rush than usual is because we're so close to the stable release. Given that, I think it makes sense to not tie ourselves to that rush and instead address this holistically in 6.5, for all of the functions being discussed. Any such late release cycle changes put additional stress on contributors and potentially confusion on external developers alike for those that may have already read the dev notes.

I am all for consistency and I would love for us to revisit the naming of all of these functions in a separate ticket. Whether we rename all of them to wp_load_*() or wp_prime_*_caches() or something completely different, any such decisions could be made there. Even the functions introduced here could be deprecated again if needed due to a renaming. Granted, it wouldn't feel great deprecating a function introduced 1-2 cycles before, but it's certainly not a blocker and has happened before in core.

A separate ticket milestoned for 6.5 will give us the opportunity to continue this discussion without the 6.4 pressure, while allowing time for further opinions to come in, and it will allow us to holistically address it, which is not possible in the scope of this ticket anyway, as it's only about the new functions.

Last edited 7 months ago by flixos90 (previous) (diff)

#46 in reply to: ↑ 44 @SergeyBiryukov
7 months ago

Replying to joemcgill:

These new functions do seem to have the same functional purpose as existing _prime_{thing}_cache functions, as I mentioned in a previous comment. Assuming we can come to agreement on that point, it seems to me like we have 3 options:

  1. Leave things as they currently stand and punt the naming conversation to a future release.
  2. Update these new function names to _prime_options_cache() and _prime_options_cache_by_group(), to be consistent with other cache priming functions in the meantime.
  3. Update the new function names to what we anticipate potentially renaming the other functions.

I would prefer option 2 here for consistency with the existing functions, as per comment:14:

All the other cache priming functions following the naming convention of _prime_thing_caches:

  • _prime_post_caches
  • _prime_post_parent_id_caches (new in 6.4)
  • _prime_site_caches
  • _prime_term_caches
  • _prime_comment_caches
  • _prime_network_caches

The other various functions that modify a cache in some way end in _caches too, update_post_caches, update_post_author_caches, etc.

For the naming, I would suggest:

  • _prime_option_caches()
  • _prime_option_caches_by_group()

The underscore here, while being a vestige of the older naming convention, would give us more flexibility in deprecating and renaming these functions following a holistic approach in 6.5.

Last edited 7 months ago by SergeyBiryukov (previous) (diff)

#47 @desrosj
7 months ago

Having read through this, I wanted to give some outside thoughts and opinions on the discussion so far in order to try and move us forward with the short amount of time available.

While I do generally agree we should make things as easy to understand as we can for developers/extenders/etc., I disagree with hiding technical terms to accomplish that. Avoiding potentially transitional jargon is one thing, but avoiding adopted and established nomenclature of things is another. Whenever clarification is required, we can better communicate purpose and context through expanded inline documentation.

There were a few mentions above that "prime" was only meant to be an internal term. I do also disagree with that. In the context of caching, prime has specific meaning and is widely used throughout the industry in association with caching. It may be that it wasn't intended to expose the term to the broader developer community, but we shouldn't shy away from it if it's the right term to use.

  • wp_load_alloptions() does not accept arguments regarding which options should be cached
  • wp_load_core_site_options() does not accept arguments regarding which options should be cached
  • _prime_thing_cache(), like these new functions, accepts an array of arguments to define which items should be cached.

As for the issue of which naming convention to use for 6.4, I see the pros and cons of both sides and personally don't have a strongly held opinion. But I definitely prefer using prime for the bullet points mentioned above.

And finally how to move forward, I also prefer option 2 mentioned above. While we intend to follow up in 6.5 to rename things more holistically, it's always possible that it could take 2, 3 or more releases to accomplish this (we all know what it's like to find new unknown problems when attempting something). So for the time being, I think it makes sense to have everything named similarly. Even if with an older naming convention.

#48 follow-up: @flixos90
7 months ago

Trying to decouple the two discussions we're having here. While in the long term we'll probably open another ticket to revisit the naming holistically, for this ticket and the 6.4 release I think we're trying to decide on two questions:

  1. Should we prefix the functions with an underscore?
  2. Should we use the term "prime" or the term "load"?

Both questions partly have to do with consistency with some existing functions.

Let me try to summarize my thoughts on question 2 first ("prime" vs "load"):

  • I think "load" is a more accessible (as in easier to understand) term than "prime".
  • FWIW, I am not a native English speaker, and a lot of WordPress developers aren't, but I certainly know what "load" means as it is a very common verb that even non developers are exposed to. You see it all the time even when using software.
  • For "prime", that is not the case. I honestly do not know even now what the verb "prime" means, as in I wouldn't be able to tell you the German translation of that word. The only reason that I know what the "prime cache" functions do because I am familiar with their code. The only meaning I know of the word "prime" is for the mathematical number meaning.
  • Whether we hide the word "caches" from the function name or not isn't that important to me. But to me something like wp_load_x_caches() is clearly more meaningful than wp_prime_x_caches().

Summarizing my thoughts for question 1 (the underscore prefix):

  • While the discussion on wording to use for the function has no "right or wrong", prefixing a public function with an underscore is simply wrong as it violates a common software engineering expectation that a underscore-prefixed function is not intended for public usage, as @hellofromTonya outlined in 20.
  • That said, I am quite certain all of us are aware of that, so the only argument for prefixing with an underscore is consistency.
  • To me however, this is consistency going too far. Why introduce poorly named functions just because there are related existing poorly named functions? (I realize that for the existing ones the implications are different since they only became public over time.)
  • I also question that consistency is a strong argument here, given there is another set of functions such as wp_load_alloptions() that also is closely related to the new functions introduced here, and the current naming would be consistent with those existing functions. So IMO we're also discussing with which set of existing functions to achieve naming consistency.
  • Specifically to the existing _prime_*_caches() functions, there is one distinct difference to the functions introduced here which is a counterargument against these functions doing the same thing:
    • The existing functions are all about priming database entities, which can be created by the end user, also sometimes referred to as "object types" in WordPress. Options are different, as the option names that exist are entirely defined by code.
    • This becomes particularly relevant when looking at the wp_load_options_by_group() function: This function would not make any sense in the context of the existing _prime_*_caches() functions. Sure, we could in theory have a function like _prime_post_caches_by_post_type(), but that would not be scalable as users can create an infinite number of posts, and the same applies to any other entities covered by the _prime_*_caches() functions. For options, that concern doesn't apply. They are controlled by code, and a single option group is extremely unlikely to have let's say more than 100 options (I think that's already a very high number for a single option group).

As a summary, I think the conversation about "prime" vs "load" is probably best held in the follow up ticket where we revisit the naming holistically, and while I have shared my preference and rationale I'm not going to get hung up on that for the short term solution, as in this ticket.

For the underscore though, I strongly object as this is wrong from an engineering perspective, and sends a confusing message for a new function that is purely intended for external usage. The argument of consistency is IMO not strong enough to go against that pattern based on what I outlined above.

#49 in reply to: ↑ 48 @peterwilsoncc
7 months ago

Replying to flixos90:

  1. Should we prefix the functions with an underscore?
  2. Should we use the term "prime" or the term "load"?
  1. You've convinced me, it's probably best not to follow the convention and prefix with wp_.
  2. I remain convinced prime is the best term. Both because of how the functions differ from existing wp_load functions and, as Jon mentions above, priming a cache is an established industry phrase. I'm guessing that's why the original function names used the word prime.

My linked pull request #5548 follows these steps and uses the names wp_prime_options_cache() and wp_prime_options_cache_by_group(). A modified version of @joemcgill's option two to address @flixos90's concerns about continuing with an underscore convention.

I'd like to adopt these in 6.4 and continue the original discussion from #56386 about either deprecating or aliasing the existing functions in a follow up ticket. Joe's option three.

#51 @flixos90
7 months ago

@peterwilsoncc I'm glad we could find a consensus. While I am leaning towards "load", I think what you're proposing is a reasonable compromise I'd be happy with.

Only one additional note on your PR: In accordance with the existing functions, the functions should probably start with wp_prime_option_caches instead of wp_prime_options_cache. :)

@peterwilsoncc commented on PR #5548:


7 months ago
#52

@felixarntz Yep, you're right: singular thing, plural caches it is. Pushed in a04715a21b3e3eeb544d327f76206e6378735dc7

I've made sure the terminology is consistent elsewhere in 7c036e98e4ddbd1f4cda121a6844fe209375e5b6.

#53 @joemcgill
7 months ago

I like the approach naming approach agreed to by @peterwilsoncc and @flixos90, as demonstrated in PR #5548.

There is consistency with the existing _prime_{thing}_caches functions with the added clarity that these are intended for public use by introducing them with the wp_ prefix. @peterwilsoncc do you plan to open a follow-up ticket to consider updating existing priming function names with a wp_ prefix as a way of encouraging additional use of those functions by third-party projects?

#54 @peterwilsoncc
7 months ago

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

In 57013:

Options, Meta APIs: Rename option cache priming functions.

Rename the option cache priming functions to more closely follow the naming convention used by other cache priming functions.

  • wp_load_options() becomes wp_prime_option_caches()
  • wp_load_options_by_group() becomes wp_prime_option_caches_by_group()

The unit test files and classes are renamed accordingly.

Unlike the existing cache priming functions, these functions were introduced with the intention of being public so use the wp_ prefix rather than the _ prefix used by the functions initially introduced as private functions but since made public.

Follow up to [56445],[56990].

Props flixos90, peterwilsoncc, joemcgill, SergeyBiryukov, desrosj.
Fixes #58962.

#56 @peterwilsoncc
7 months ago

  • Keywords dev-feedback added; dev-reviewed removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to 6.4 branch upon another committers sign-off.


@peterwilsoncc do you plan to open a follow-up ticket to consider updating existing priming function names with a wp_ prefix as a way of encouraging additional use of those functions by third-party projects?

@joemcgill Yes, I will open a ticket.

#57 @flixos90
7 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Thanks @peterwilsoncc, signing off for backport.

I'll update the dev note now.

#58 @hellofromTonya
7 months ago

Good compromise everyone. Thank you for coming to a consensus to resolve the renaming for 6.4.

#59 @peterwilsoncc
7 months ago

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

In 57016:

Options, Meta APIs: Rename option cache priming functions.

Rename the option cache priming functions to more closely follow the naming convention used by other cache priming functions.

  • wp_load_options() becomes wp_prime_option_caches()
  • wp_load_options_by_group() becomes wp_prime_option_caches_by_group()

The unit test files and classes are renamed accordingly.

Unlike the existing cache priming functions, these functions were introduced with the intention of being public so use the wp_ prefix rather than the _ prefix used by the functions initially introduced as private functions but since made public.

Follow up to [56445],[56990].

Props flixos90, peterwilsoncc, joemcgill, SergeyBiryukov, desrosj.
Reviewed by flixos90.
Merges [57013] to the 6.4 branch.
Fixes #58962.

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


7 months ago
#60

See https://core.trac.wordpress.org/ticket/58962

Some additional tests that already pass.

@peterwilsoncc commented on PR #5588:


7 months ago
#61

All of these are in the other pr. I agree, it’s best to put the other pr in but wanted to pull out these so I had passing tests I could yolo commit during the rc if the src changes weren’t approved. --Sent from my phoneOn 31 Oct 2023, at 5:01 am, Felix Arntz *@*.*> wrote:
@felixarntz commented on this pull request.

@peterwilsoncc Is there anything in here that's not in #5576? I'm asking since I think we should rather commit that PR with the fixes and backport it, per my response on the ticket.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: *@*.*>

@peterwilsoncc commented on PR #5588:


7 months ago
#62

Closing without merge, PR https://github.com/WordPress/wordpress-develop/pull/5576 to be committed to include src fixes.

#63 @peterwilsoncc
7 months ago

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.

#64 @peterwilsoncc
7 months ago

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.

Note: See TracTickets for help on using tickets.