-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Plugin cannot be uninstalled if uninstall times out/crashes #6018
base: trunk
Are you sure you want to change the base?
Plugin cannot be uninstalled if uninstall times out/crashes #6018
Conversation
…is registered conditionally Fix https://core.trac.wordpress.org/ticket/59402
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a new variable here, see suggestions.
If we're worried about plugins interfering with the global variable, we should consider a name a la $_wp_uninstallable_plugins
if anything. But again probably not needed.
* call uninstall_ hook if there is no uninstall callable or no uninstall.php * ensure that callable is valid before calling to avoid fatals * add after_uninstall_plugin hook * fix PHP notice from already defined constant
if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) { | ||
// The constant value does not indicate the plugin being uninstalled | ||
// e.g. in bulk actions it is the first plugin uninstalled. | ||
// The value should not be used and is only available for legacy reasons. | ||
define( 'WP_UNINSTALL_PLUGIN', $plugin_basename ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) { | |
// The constant value does not indicate the plugin being uninstalled | |
// e.g. in bulk actions it is the first plugin uninstalled. | |
// The value should not be used and is only available for legacy reasons. | |
define( 'WP_UNINSTALL_PLUGIN', $plugin_basename ); | |
/** | |
* The constant value does not indicate the plugin being uninstalled. | |
* | |
* e.g. in bulk actions it is the first plugin uninstalled. | |
* | |
* The value should not be used and is only available for legacy reasons. | |
*/ | |
if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) { | |
define( 'WP_UNINSTALL_PLUGIN', $plugin_basename ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, should there really be a blank line between each of them though?
unset( $uninstallable_plugins[ $plugin_basename ] ); | ||
} | ||
|
||
$uninstall_file = WP_PLUGIN_DIR . '/' . dirname( $plugin_basename ) . '/uninstall.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to have a file_exists
check over here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
if ( isset( $uninstallable_plugins[ $file ] ) ) { | ||
unset( $uninstallable_plugins[ $file ] ); | ||
update_option( 'uninstall_plugins', $uninstallable_plugins ); | ||
// must be a separate variable in case we have a null or false callable value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// must be a separate variable in case we have a null or false callable value | |
// Must be a separate variable in case we have a null or false callable value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Will fix to uppercase things all at once when all other review points are discussed/finalized
} else { | ||
$uninstall_file = WP_PLUGIN_DIR . '/' . $plugin_basename; | ||
if ( ! is_readable( $uninstall_file ) ) { | ||
// the plugin was previous uninstalled already but the option not updated due to a race condition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the plugin was previous uninstalled already but the option not updated due to a race condition | |
// The plugin was previously uninstalled already but the option was not updated due to a race condition. |
* Fires in uninstall_plugin() once the plugin has been uninstalled. | ||
* | ||
* The action concatenates the 'uninstall_' prefix with the basename of the | ||
* plugin passed to uninstall_plugin() to create a dynamically-named action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* plugin passed to uninstall_plugin() to create a dynamically-named action. | |
* Plugin passed to uninstall_plugin() to create a dynamically-named action. |
$file = trim( $file, '/' ); | ||
return $file; | ||
$file = preg_replace( '#^(?>' . preg_quote( $plugin_dir, '#' ) . '|' . preg_quote( $mu_plugin_dir, '#' ) . ')/#', '', $file, 1 ); | ||
return trim( $file, '/' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return trim( $file, '/' ); | |
$file = trim( $file, '/' ); | |
return $file; |
Let's not change this if it's not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep a bad practice?
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.
Trac ticket: https://core.trac.wordpress.org/ticket/59402
Trac ticket: https://core.trac.wordpress.org/ticket/43098
Trac ticket: https://core.trac.wordpress.org/ticket/26735