WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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

  1. Install markjaquith's APC object caching plugin
  2. Activate my plugin. Each refresh will append an object to the array
  3. Note output is:
    array(0) {
    }
    
  4. Refresh. Note new output is:
    array(1) {
      [0]=>
      object(rmccue_Test_Object)#275 (0) {
      }
    }
    
  5. 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)

22381.diff (592 bytes) - added by rmccue 2 years ago.
Partially revert r13673 to fix update_option's object caching
testbroken.php (353 bytes) - added by rmccue 2 years ago.
Test file to reproduce the issue. Requires APC to demonstrate
22381.alternate.diff (604 bytes) - added by rmccue 2 years ago.
Alternate patch that changes add_option to match update_option (will need fixing in APC instead)

Download all attachments as: .zip

Change History (14)

comment:1 @nacin2 years ago

  • Milestone changed from Awaiting Review to 3.6

@rmccue2 years ago

Partially revert r13673 to fix update_option's object caching

@rmccue2 years ago

Test file to reproduce the issue. Requires APC to demonstrate

comment:2 @rmccue2 years ago

  • Version set to 3.1

Looks like this has been around for a while.

@rmccue2 years ago

Alternate patch that changes add_option to match update_option (will need fixing in APC instead)

comment:3 follow-up: @rmccue2 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.

comment:4 in reply to: ↑ 3 @hexalys2 years ago

  • Cc themacmaster@… added

Replying to rmccue:

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.

rmccue, could you explain the 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.

Version 0, edited 2 years ago by hexalys (next)

comment:6 in reply to: ↑ 5 @hexalys2 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.

Last edited 2 years ago by hexalys (previous) (diff)

comment:7 @SergeyBiryukov2 years ago

  • Version changed from trunk to 3.0

Version number indicates when the bug was initially introduced/reported.

comment:8 @inderpreet992 years ago

  • Cc inderpreet99@… added

comment:9 @bradparbs2 years ago

  • Cc brad@… added

comment:10 @nacin2 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 24760:

Make update_option() consistent with add_option() for how serializable data is stored in the object cache.

props rmccue.
fixes #23381.

comment:11 @nacin2 years ago

[24760] was reviewed with ryan (rboren).

Note: See TracTickets for help on using tickets.