Make WordPress Core

#38903 closed defect (bug) (fixed)

Prevent `update_option()` from updating when the old and new values contain identical objects.

Reported by: peterwilsoncc Owned by: peterwilsoncc
Milestone: 4.8 Priority: normal
Severity: normal Version: 2.0
Component: Options, Meta APIs Keywords: early has-patch has-unit-tests
Focuses: Cc:


When an object is included in an option, passing an unchanged value to update_option will trigger an UPDATE query.

Given the data below, the meta_data will have a different resource ID for the old and new values. $value === $old_value will always evaluate untrue and the database will be updated and the caches cleared.

array(2) {
  string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
  object(stdClass)#370 (3) {

Followup from #38866, props due @bradyvercher for finding.

Attachments (4)

38903.diff (663 bytes) - added by peterwilsoncc 18 months ago.
38903.2.diff (2.3 KB) - added by salcode 18 months ago.
Updated patch file to include test
38903.3.diff (1.9 KB) - added by salcode 18 months ago.
38903.4.diff (2.3 KB) - added by peterwilsoncc 18 months ago.

Download all attachments as: .zip

Change History (16)

#1 @peterwilsoncc
18 months ago

  • Keywords has-patch needs-unit-tests added

In 38903.diff:

  • run the old and new values through maybe_serialize() before comparing.

Bringing this over from a patch in #38866.

I'm not sure of the perfromance implication of serialising in this manner, on the surface it's doesn't seem great, so some data would be helpful here.

18 months ago

Updated patch file to include test

#2 follow-up: @salcode
18 months ago

Test written at #wcus 2016 contributor day

#3 @iaaxpage
18 months ago

  • Keywords needs-unit-tests removed

@peterwilsoncc I just pulled the patch and ran the option unit tests and they passed.

phpunit tests/phpunit/tests/option/option.php

The method test_the_basics asserts this to false if the value is the same.

$this->assertFalse( update_option( $key, $value ) ); // Value is the same

#4 in reply to: ↑ 2 @peterwilsoncc
18 months ago

Replying to salcode:

Test written at #wcus 2016 contributor day

Thanks for the tests @salcode.

Rather than using the hook to detect if the function exists early, a more direct test would be to test get_num_queries() returns the same value before and after the function call. It will save a bit of messing around.

Testing the returned value is false would also be ideal.

18 months ago

#5 follow-up: @salcode
18 months ago

Ha ha, that is an infinitely better way to test this. Thanks @peterwilsoncc

I've updated the patch.

#6 in reply to: ↑ 5 @peterwilsoncc
18 months ago

  • Keywords has-unit-tests needs-refresh added
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

Replying to salcode:

I've updated the patch.

Thanks for the refresh.

One gotcha in the patch, assertFalse isn't available in the version of phpunit WP uses when testing PHP 5.6 and earlier. To test for false, the code is:

$this->assertSame( false, update_option( 'array_w_object', $array_w_object ) );

My intention is to commit the code this weekend, I can update the patch then if you wish.

#7 @peterwilsoncc
18 months ago

@salcode ignore me, I was thinking of assertNotFalse(). Your patch is great as it. :)

#8 @peterwilsoncc
18 months ago

  • Keywords needs-refresh removed

#9 @salcode
18 months ago

Great, thanks for checking one item off my to do list. :)

#10 @peterwilsoncc
18 months ago

In 38903.4.diff:

  • avoid the maybe_serialize calls if they are not needed. This is to avoid the performance cost of serialization in most instances.
  • minor formatting changes in the unit tests for coding standards

#11 @peterwilsoncc
18 months ago

  • Component changed from General to Options, Meta APIs

#12 @peterwilsoncc
18 months ago

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

In 39564:

Options: Prevent unnecessary SQL updates by update_option.

Previously an option containing an object would trigger an SQL UPDATE on all calls to update_option, even if the old and new values were identical. This was due to the old and new values having differing resource IDs.

This change compares the old and new values as serialized data to remove the resource ID from the comparison.

Props salcode, bradyvercher, peterwilsoncc.
Fixes #38903.

Note: See TracTickets for help on using tickets.