#23381 closed defect (bug) (fixed)
update_option() stores unserialized value in object cache
Reported by: | rmccue | Owned by: | nacin |
---|---|---|---|
Milestone: | 3.6 | Priority: | normal |
Severity: | normal | Version: | 3.0 |
Component: | Cache API | Keywords: | has-patch |
Focuses: | Cc: |
Description
update_option()
and add_option()
have inconsistent behaviour when storing items in the cache. add_option()
stores the serialized version, whereas update_option()
stores the raw data. When using APC for object caching, this can result in objects being loaded in wp_load_alloptions()
before the class for it is defined in a plugin.
In update_option()
(storing raw data):
$_newvalue = $newvalue; $newvalue = maybe_serialize( $newvalue ); // ... $alloptions[$option] = $_newvalue; wp_cache_set( 'alloptions', $alloptions, 'options' ); }
Whereas in add_option()
(storing serialized data):
$_value = $value; $value = maybe_serialize( $value ); // ... $alloptions[$option] = $value; wp_cache_set( 'alloptions', $alloptions, 'options' );
Example plugin included, breaks horribly when using the APC cache, as that doesn't enforce serialization internally. This only occurs when using update_option()
on an existing option.
Looks like this was introduced in r13673.
Steps to reproduce
- Install markjaquith's APC object caching plugin
- Activate my plugin. Each refresh will append an object to the array
- Note output is:
array(0) { }
- Refresh. Note new output is:
array(1) { [0]=> object(rmccue_Test_Object)#275 (0) { } }
- Refresh again. Output should be:
array(2) { [0]=> object(__PHP_Incomplete_Class)#5 (1) { ["__PHP_Incomplete_Class_Name"]=> string(18) "rmccue_Test_Object" } [1]=> object(__PHP_Incomplete_Class)#6 (1) { ["__PHP_Incomplete_Class_Name"]=> string(18) "rmccue_Test_Object" } }
Attachments (3)
Change History (15)
@
12 years ago
Alternate patch that changes add_option to match update_option (will need fixing in APC instead)
#3
follow-up:
↓ 4
@
12 years ago
As per rboren on IRC, changed the patch to switch add_option()
to match update_option()
. Will require fixing APC to handle the specific use-case.
#4
in reply to:
↑ 3
@
12 years ago
- Cc themacmaster@… added
Replying to rmccue:
As per rboren on IRC, changed the patch to switch
add_option()
to matchupdate_option()
. Will require fixing APC to handle the specific use-case.
rmccue, could you explain rboren's rationale for switching add_option() to match update_option(), rather that the other way around. This would leave an inconsistent pattern of 'alloptions' being either a serialized string or an array depending on the existence of an object cache, like it it now with the r13673 defect. I am not liking the idea of having mixed variables with a context dependency added. Update_option() is the one that should match add_option() for consistency in my view. That's where it was broken in the first place.
#5
follow-up:
↓ 6
@
12 years ago
The IRC chat log relevant to comment:3:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-02-04&sort=asc#m547662
#6
in reply to:
↑ 5
@
12 years ago
- Version changed from 3.1 to trunk
Replying to SergeyBiryukov:
The IRC chat log relevant to comment:3:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-02-04&sort=asc#m547662
Thanks Sergey, I see it's more of a 'off the top of my head' thought than a rationale...
What am trying to say here is that the options cache storage needs to be consistent. As for rboren's pointless serialization remark: For update_option() and add_option(), serialize() has to run anyway for the db query so there are no performance caveats there.
I shall point to the more pointless unserialization with get_option() which has no process to copy its unserialized value to the $wp_object_cache->cache, on first unserialization... Which means that with the basic wp object cache, get_option() has to run maybe_unserialize() and unserialize() for every other calls to the same serialized option, making get_option() quite redundant there.
The "caching by groups" concept (e.g. alloptions) with the current implementation of get_option() have unavoidable unserialization waste, one a way or another. And even with memory cache, because wp_load_alloptions() loads early, you cannot store objects in the 'alloptions' groups for any plugins, if the cached options are stored as objects (or arrays of object), due the classes being no yet loaded. Having them serialized allows for just-in-time unserialization for plugins.
It's not perfect but the current model is to store everything as serialized, not making them Arrays or Object ONLY on update_option() or add_option(), in a very inconsistent way like that.
#7
@
12 years ago
- Version changed from trunk to 3.0
Version number indicates when the bug was initially introduced/reported.
#10
@
11 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In 24760:
Partially revert r13673 to fix update_option's object caching