WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

#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:

Description

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) {
  ["url"]=>
  string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
  ["meta_data"]=>
  object(stdClass)#370 (3) {
    ["attachment_id"]=>
    int(292)
    ["height"]=>
    int(708)
    ["width"]=>
    int(1260)
  }
}

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

Attachments (4)

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

Download all attachments as: .zip

Change History (16)

@peterwilsoncc
4 months ago

#1 @peterwilsoncc
4 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.

@salcode
4 months ago

Updated patch file to include test

#2 follow-up: @salcode
4 months ago

Test written at #wcus 2016 contributor day

#3 @iaaxpage
4 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
4 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.

@salcode
4 months ago

#5 follow-up: @salcode
4 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
4 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
4 months ago

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

#8 @peterwilsoncc
4 months ago

  • Keywords needs-refresh removed

#9 @salcode
4 months ago

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

#10 @peterwilsoncc
4 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
3 months ago

  • Component changed from General to Options, Meta APIs

#12 @peterwilsoncc
3 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.