Opened 8 years ago
Closed 8 years ago
#38903 closed defect (bug) (fixed)
Prevent `update_option()` from updating when the old and new values contain identical objects.
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (16)
#3
@
8 years 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
@
8 years 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.
#5
follow-up:
↓ 6
@
8 years ago
Ha ha, that is an infinitely better way to test this. Thanks @peterwilsoncc
I've updated the patch.
#6
in reply to:
↑ 5
@
8 years 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
@
8 years ago
@salcode ignore me, I was thinking of assertNotFalse()
. Your patch is great as it. :)
#10
@
8 years 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
In 38903.diff:
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.