WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#31625 closed defect (bug) (fixed)

Function register_uninstall_hook doesn't have a method to remove the outdated data

Reported by: voron_eril Owned by: dd32
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.1.1
Component: Plugins Keywords: has-patch needs-testing
Focuses: performance Cc:

Description

Function register_uninstall_hook doesn't have a method to remove the outdated data. Here's a problem that I faced recently:
Subheading plugin used register_uninstall_hook to register its uninstaller in the old versions. But in new version register_uninstall_hook call was removed and uninstall.php was added.
And the plugin file was renamed from index.php to subheading.php.
As a result this is what I get in a serialized array:

  ["subheading/index.php"]=>
  array(2) {  [0]=>  object(__PHP_Incomplete_Class)#242 (5) {


The digit after __PHP_Incomplete_Class is always different, so on every update_option call Wordpress lauches MySQL

"UPDATE `wp_options` SET `option_value` = 'a:8:{i:0;b:0;s:33 ".

But because writing to a field locks a table row, this causes the situation when server in fact can process only one visitor at a time.

Attachments (2)

31625.diff (1.2 KB) - added by polevaultweb 5 years ago.
Upgrade routine added
31625.2.diff (1.2 KB) - added by polevaultweb 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 @DrewAPicture
5 years ago

@dd32: Would you mind taking a gander at this?

#2 @dd32
5 years ago

  • Milestone changed from Awaiting Review to 4.4

It looks like register_uninstall_hook() was used incorrect here in the first place, register_uninstall_hook() is designed to be given a function callback (or a static class variable), NOT a array/object instance like was provided here.

We've blocked those being used since [16339] so the plugin in question would've had to be run over 5 years ago (Okay, so it could be more recent if they'd not updated their site).

There's three options here:

  1. wontfix
  2. add a pre option save filter to make sure it only ever contains the right contents
  3. add a once-off upgrade routine to check to see if it contains any non-string/static callbacks

I see nothing wrong with doing #3 here, so if someone wants to whip up a patch before I do..

Do we have any other arrays in options which store similar data?

#3 @DrewAPicture
5 years ago

  • Keywords needs-patch added

#4 @DrewAPicture
5 years ago

  • Keywords punt added
  • Milestone changed from 4.4 to Future Release

Punting to Future Release in lieu of a patch. See comment:2 for suggested approaches.

#5 @DrewAPicture
5 years ago

  • Keywords punt removed

@polevaultweb
5 years ago

Upgrade routine added

#6 @polevaultweb
5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Patch added with upgrade routine. I incremented the current db version to an arbitrary 36999 as I wasn't sure how it would be calculated. I have also assumed this would be for 4.6.

#7 @dd32
5 years ago

  • Milestone changed from Future Release to 4.6
  • Owner set to dd32
  • Status changed from new to accepted

Other than not needing to check $update (update_option() will check if the value has changed) 31625.diff seems good to me.
You don't need to create a new patch for that though, can strip that out during commit easily enough (feel free to upload a new one if you'd like)

I incremented the current db version to an arbitrary 36999 as I wasn't sure how it would be calculated.

We set it to the SVN Commit number of the commit we're making when the patch is committed, so an arbitrary number is good :) although I usually just use +1 (and then +1 again for the next test iteration.. etc)

@polevaultweb
5 years ago

#8 @polevaultweb
5 years ago

@dd32 Thanks for that. I've uploaded a new patch with the SVN commit incremented (thanks for the info). I have removed the $update check, I did know that honest ;)

I have added a check for the initial get option as well. Cheers!

#9 @polevaultweb
4 years ago

@dd32 did you get chance to look at this again? Cheers

#10 @ocean90
4 years ago

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

In 37965:

Plugins: Clean up uninstall_plugins option during database upgrade.

register_uninstall_hook() is designed to be given a function callback (or a static class variable), not an array/object instance. This got blocked in [16339] but the option itself was never cleaned up.

Props polevaultweb.
Fixes #31625.

Note: See TracTickets for help on using tickets.