Make WordPress Core

Opened 9 months ago

Last modified 10 days ago

#59402 assigned defect (bug)

Plugin cannot be uninstalled if uninstall crashes

Reported by: kkmuffme's profile kkmuffme Owned by:
Milestone: 6.7 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 (14)

#1 @sabernhardt
8 months ago

  • Component changed from General to Plugins

This ticket was mentioned in PR #6018 on WordPress/wordpress-develop by @kkmuffme.


5 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.


4 months ago

#4 @swissspidy
4 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
4 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
4 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
3 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
2 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
8 weeks ago

  • Owner swissspidy deleted

#10 @kkmuffme
8 weeks 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 weeks ago

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

#12 @kkmuffme
5 weeks ago

@rajinsharwar replied

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


4 weeks ago

#14 @oglekler
10 days 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.

Note: See TracTickets for help on using tickets.