Skip to content
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

Cleanup files when upgrading/downgrading #2382

Merged
merged 18 commits into from Jan 21, 2016

Conversation

@gilbitron
Copy link
Contributor

commented Jan 15, 2016

Resolves #2183

Not sure if this is the best solution but it is one way of doing it.

gilbitron added 2 commits Jan 15, 2016
@@ -1150,6 +1164,29 @@ private function get_updates( $assoc_args ) {
return array_values( $updates );
}
private function _maybe_remove_unused_files( $version_from, $version_to) {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

No underscore needed

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

Also, I'm not a fan of methods with maybe in the name, as it makes the method ambiguous. We should do the version comparison beforehand.

if ( ! empty( $files_to_remove ) ) {
foreach ( $files_to_remove as $file ) {
if ( file_exists( $file ) ) {
unlink( $file );

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

We should WP_CLI::log() which files are removed.

This comment has been minimized.

Copy link
@gilbitron

gilbitron Jan 15, 2016

Author Contributor

This could potentially be a lot of files. Would it better to just output a count of files?

$files_to_remove = array();
$includes_folder = defined( 'WPINC' ) ? WPINC : '/wp-includes';
if ( version_compare( $version_from, '4.4', '>=' ) && version_compare( $version_to, '4.4', '<' ) ) {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

You can use self::get_core_checksums() to get a full list of the files present in each version, and then compare the two.

Then the wp-includes/rest-api.php file should exist

When I run `wp core update --version=4.3.2 --force`
Then the wp-includes/rest-api.php file should not exist

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

We should have a few basic tests here to confirm WordPress is still running as expected, lest downgrading deleted all of the files in the install.

This comment has been minimized.

Copy link
@gilbitron

gilbitron Jan 15, 2016

Author Contributor

Sure. Got an idea of the kind of tests you would like to see?

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

Option get and set; maybe create a post and edit a comment.

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

Can we add one more test for an expected WP 4.3.2 file?

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

Also, a test for Then STDOUT should contain: with a list of the files removed would be great.

@@ -143,6 +151,10 @@ public function download( $args, $assoc_args ) {
unlink($temp);
}
if ( version_compare( $from_version, '4.4', '>=' ) && version_compare( $assoc_args['version'], '4.4', '<' ) ) {

This comment has been minimized.

Copy link
@gilbitron

gilbitron Jan 15, 2016

Author Contributor

Could we remove the version comparison check and just do this for all upgrades/downgrades?

This comment has been minimized.

Copy link
@danielbachhuber

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

Although, we need to ensure we safely handle the cases where WordPress.org doesn't return checksums for a version (for whatever reason).

This comment has been minimized.

Copy link
@gilbitron

gilbitron Jan 15, 2016

Author Contributor

Currently if either of the checksum calls fail no files will be removed. Is this a reasonable fallback?

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

Currently if either of the checksum calls fail no files will be removed. Is this a reasonable fallback?

I think so, although we should WP_CLI::warn() when it happens so the user knows to go clean up.

This comment has been minimized.

Copy link
@gilbitron

gilbitron Jan 15, 2016

Author Contributor

👍

unlink( ABSPATH . $file );
}
}
WP_CLI::log( count($files_to_remove) . ' files cleaned up' );

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 15, 2016

Member

I think we should log each of the files removed, to give greater visibility to the end user of what's happening. When something goes wrong, the count doesn't help me debug at all.

This comment has been minimized.

Copy link
@gilbitron

gilbitron Jan 15, 2016

Author Contributor

👍

@gilbitron gilbitron changed the title Cleanup files when downgrading from WP 4.4 Cleanup files when upgrading/downgrading Jan 15, 2016

Then the wp-includes/rest-api.php file should exist

When I run `wp core download --version=4.3.2 --force`
Then the wp-includes/rest-api.php file should not exist

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

Can we add one more test for an expected WP 4.3.2 file?

@@ -99,6 +99,14 @@ public function download( $args, $assoc_args ) {
$download_url = str_replace( '.zip', '.tar.gz', $offer['download'] );
}
$from_version = '';
$includes_folder = defined( 'WPINC' ) ? WPINC : 'wp-includes';

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

When would WP_INC not be defined?

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

Actually, given this command runs on before_wp_load, when would it be defined?

@@ -1150,6 +1165,40 @@ private function get_updates( $assoc_args ) {
return array_values( $updates );
}
private function remove_unused_files( $version_from, $version_to, $locale ) {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

Can we rename this method to cleanup_extra_files()?

@@ -1150,6 +1165,40 @@ private function get_updates( $assoc_args ) {
return array_values( $updates );
}
private function remove_unused_files( $version_from, $version_to, $locale ) {
if ( ! $version_from || ! $version_to ) {
WP_CLI::warning( 'Failed to find WordPress version. Please cleanup unused files manually.' );

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

You can drop "unused" from this sentence, as "unused is ambiguous"

}
if ( $count ) {
WP_CLI::success( number_format( $count ) . ' unused files cleaned up' );

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

Use WP_CLI::log() instead of WP_CLI::success(), and drop "unused"

if ( $count ) {
WP_CLI::success( number_format( $count ) . ' unused files cleaned up' );
}

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Jan 19, 2016

Member

If $count ended up being empty, can we have a message that says there weren't any files to be removed?

@danielbachhuber danielbachhuber added this to the 0.23.0 milestone Jan 19, 2016

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

@gilbitron looking really good! just a few more minor nits

@gilbitron

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2016

@danielbachhuber Changes committed 👍

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Jan 20, 2016

@gilbitron Looks good 👍 I broke your build though :( Can you merge master?

@gilbitron

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

@gilbitron it looks like your test is failing on older versions. To make it more precise, you could have it start with an empty directory, install WordPress 4.4, and then perform the downgrade. On older versions, because it's starting with the older version of WordPress, it appears the cleanup process happens a bit differently.

@gilbitron

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

@danielbachhuber I fixed it by removing the file count from the test (to account for differences in the way cleanup is done).

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

Great — thanks @gilbitron !

danielbachhuber added a commit that referenced this pull request Jan 21, 2016
Merge pull request #2382 from gilbitron/cleanup-files
Cleanup files when upgrading/downgrading

@danielbachhuber danielbachhuber merged commit 12e74f4 into wp-cli:master Jan 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.