[wp-trac] [WordPress Trac] #31245: Replace alloptions with a key cache

[WordPress Trac]

#31245: Replace alloptions with a key cache
-------------------------------------+-----------------------------
 Reporter:  rmccue                   |       Owner:  rmccue
     Type:  enhancement              |      Status:  assigned
 Priority:  normal                   |   Milestone:  Future Release
Component:  Options, Meta APIs       |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch needs-testing  |     Focuses:  performance
-------------------------------------+-----------------------------

Comment (by flixos90):

 I really like this proposed change. I'm not sure about the usage of
 metadata for options, but this can be discussed separately. Having
 distinct classes for options and a better way to query and cache them is a
 major improvement, not only for the obvious issues, but also for allowing
 developers to query options more flexibly for all the edge-cases where
 they currently make direct DB requests. :)

 I did a first review on [attachment:31245.2.diff] and have the following
 comments:

 * Why does line 236 of `class-wp-option-query.php` check whether multisite
 is disabled? Not saying this is wrong, but I'd like to know what the
 background for doing that is.
 * The query vars for querying by option ID should either be called simply
 `option`, `option__in` and `option__not_in` (or alternatively `id`,
 `id__in` and `id__not_in`). This ensures they align with similar names
 from other Core query classes. I think this change is especially important
 since the `option_id` column is not the "actual" property in `WP_Option`
 (it is just `id` which is good!), so I don't think that name should be
 used anywhere.
 * I'd prefer for `WP_Option::get_instance()` to have a different name.
 Other Core object classes already contain a `get_instance()` method that
 accepts an ID and returns the instance for that ID.
 `WP_Option::get_instance()` is more flexible though, as it behaves more
 like functions such as `get_post()` or `get_site()`. Therefore I think we
 should give it a different name, so that we would theoretically be able to
 add a method with similar flexibility to the other Core object classes
 that has the same name. I'm thinking about simply using
 `WP_Option::get()`, just to be future-proof (think about a possible
 `WP_Post::get()` as replacement for `get_post()`, or `WP_Site::get()` as
 replacement for `get_site()`).
 * I don't think we need to support an array being passed to the
 `WP_Option` constructor. No other Core object class supports this, and
 supporting that only adds a bit of unnecessary complexity to the logic.
 * I like the idea of having properties with more sane names in
 `WP_Option`, however it appears there are some issues with setting them.
 The constructor simply sets the `get_object_vars()` keys from the database
 result, which would not be the properties that the class declares. We need
 to make sure the correct properties are set by mapping `$option_id` to
 `$id`, `$option_name` to `$name` and `$option_value` to `$value`.
 * I'm not sure we should even support the original database column names
 through magic properties. There are no backward-compatibility concerns as
 this is new. :) Especially the magic `ID` property should not be
 supported. It has never been associated with options, and at some point in
 the Core chat it was decided to go with `id` as property name for an ID
 wherever possible. Due to those reasons, I think no magic methods should
 be present in the class.
 * Currently full `WP_Option` objects are stored in cache. I think it's
 fine to store the full objects, but it should be plain `stdClass` objects,
 as storing the entire class instances can cause unexpected problems. Some
 logic could be added `wp_update_option_cache()` to ensure that.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/31245#comment:48>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform