WordPress.org
Get WordPress

Make WordPress Core

Opened 13 months ago

Last modified 4 weeks ago

#59402 assigned defect (bug)

Plugin cannot be uninstalled if uninstall crashes

Reported by: kkmuffme's profile kkmuffme Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.7
Component: Plugins Keywords: has-patch early needs-testing changes-requested
Focuses: Cc:

Description

If a plugin register the "register_uninstall_hook" conditionally (e.g. only on activation,...) a plugin cannot be uninstalled anymore if it exits (e.g. timeout,...) during uninstall.

This is because the uninstall file/callback is removed from the option before the actual uninstall happens:
​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/plugin.php#L1253
​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/plugin.php#L1269

Change History (15)

#1 @sabernhardt
12 months ago

  • Component changed from General to Plugins

This ticket was mentioned in ​PR #6018 on ​WordPress/wordpress-develop by ​@kkmuffme.
8 months ago #2

  • Keywords has-patch added

Plugin cannot be uninstalled if uninstall times out/crashes and hook is registered conditionally.
This PR will remove the plugin from uninstallable plugins only after the uninstall hook ran to fix this bug.

This ticket was mentioned in ​Slack in #core by kkmuffme. ​View the logs.
8 months ago

#4 @swissspidy
8 months ago

  • Milestone changed from Awaiting Review to 6.5

Would be good to check out the code history to see why it was added this way.

Apart from that it looks reasonable.

#5 @swissspidy
8 months ago

  • Owner set to swissspidy
  • Status changed from new to assigned
  • Version set to 2.7

Introduced in 8585 / #5625 and mostly unchanged since.

#6 @swissspidy
8 months ago

  • Keywords early needs-testing added
  • Milestone changed from 6.5 to 6.6

Lets fix this early in 6.6 as this code hasn't been touched in ages and it would be good to test it longer in a cycle.

#7 @kkmuffme
6 months ago

@swissspidy could you merge this now, since WP 6.5 is released and this is the earliest possible for 6.6

#8 @rajinsharwar
6 months ago

  • Keywords changes-requested added

Added some comments @kkmuffme. I believe the patch still needs some work, and thorough testing to avoid any regression later in the release. Also, we may need some unit tests for this as well for this case. if you could get some time to work on some unit tests, we may get this merged early with 6.6. :)

#9 @swissspidy
6 months ago

  • Owner swissspidy deleted

#10 @kkmuffme
6 months ago

@rajinsharwar could you please let me know where you added the comments? I see none on the PR.

Also, we may need some unit tests for this as well for this case

Any existing unit tests already cover this, don't they?
The changes of this PR cannot be unit tested per se in addition to any existing uninstall unit tests, since this PR fixes an issue where the code exits (fatal, timeout, explicit exit).

#11 @rajinsharwar
5 months ago

@kkmuffme The comments were pending, can you check now?

#12 @kkmuffme
5 months ago

@rajinsharwar replied

This ticket was mentioned in ​Slack in #core by nhrrob. ​View the logs.
5 months ago

#14 @oglekler
4 months ago

  • Milestone changed from 6.6 to 6.7

This is an early ticket and at is too late for it in this cycle, so, I am moving it to the next milestone.

#15 @stoyangeorgiev
4 weeks ago

  • Milestone changed from 6.7 to Future Release

Discussed at at a bug-scrub session.

props to @khoipro for checking this one.

The PR needs some additional changes that were commented. Moving this one to a Future Release until then.

Note: See TracTickets for help on using tickets.