-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add rollback auto update #3958
Add rollback auto update #3958
Conversation
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 for prepping the PR @afragen! I've left a few thoughts in this review
Co-authored-by: Colin Stewart <[email protected]>
Co-authored-by: Colin Stewart <[email protected]>
Co-authored-by: Colin Stewart <[email protected]>
allows for cleanup of filter hooks from fatal plugin updates
deactivate, use include(), then reactivate. More closely follows plugin_sandbox_scrape()
reactivates plugins with error exceptions
$ref_temp_backups->setValue( $rollback_updater, $temp_backup ); | ||
|
||
// Call Rollback's restore_temp_backup(). | ||
$rollback_updater->restore_temp_backup(); |
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'm probably missing something here (or this is handled in the flow), but I noticed that both this and delete_temp_backup()
return WP_Error
.
How are these errors usually handled?
Or if it fails, is the assumption that's the best it can do?
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.
Assuming the errors are handled or expected in some way, I'd love to see a comment explaining the flow.
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.
Or if it fails, is the assumption that's the best it can do?
That's essentially it yeah.
During an upgrade, restoration and deletion of temp backups occurs during the 'shutdown'
action, triggered in a registered shutdown function, when the upgrade process fails.
While these methods might fail, by this point, the previously installed version has already been backed up, the new version downloaded and unpacked to the plugins directory. We should therefore have the permissions and diskspace to do a restore and delete. PHP's shutdown
is immune to timeouts, so that's another potential cause of failure avoided.
Nevertheless, we worded the email carefully in saying "may have been restored from a temporary backup" to account for cases that are very hard to predict given our established permissions, diskspace, and timeout immunity.
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.
Thank you! That makes sense. I would love to see this described in a comment, since it may be clear to some folks, but wasn't immediately so for me, and might make it easier for folks to work on in the future.
Co-authored-by: Mukesh Panchal <[email protected]>
Closing in favor of #5287 |
This PR adds the rollback auto update functionality to core. This is already part of the Rollback Update Failure feature plugin.
This requires PR2225. ✅
Trac ticket: https://core.trac.wordpress.org/ticket/58281
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.