Support 'trunk' and 'nightly' version arguments for wp core download. #3127

stephenharris · 2016-07-08T10:18:32Z

Modifies Core_Command::get_download_url() to interpret 'trunk' and 'nightly' versions
Core_Command::get_download_url() now conforms to WordPress coding standards
Adds support for downloading and extracting .zip files as nightly builds are own available in zip format.
wp core download --version=trunk is only supported if ZipArchive is present.

- Modifies Core_Command::get_download_url() to interpret 'trunk' and 'nightly' versions - Core_Command::get_download_url() now conforms to WordPress coding standards - Adds support for downloading and extracting .zip files as nightly builds are own available in zip format. - wp core download --version=trunk is only supported if ZipArchive is present.

stephenharris · 2016-07-08T10:29:55Z

I'll follow up with some Behat tests.

Documentation tells me PharData supports zip files too. But it just produced garbled file contents (filenames and directory structure was intact). I haven't explored the potential of using command line tools as is currently used if available for extracting tar files.

md5 checksums are not available for trunk downloads so I just display a warning. You get a similar error when trying to overwrite an existing download.

One future change, which would be nice but isn't within the scope of this issue would be to move the logic for determining which tool to use (tar,PharData,ZipArchive etc) to a factory class which gives us back a instance of class fulfilling a common interface - so the command doesn't know (or care) what tool is being used to extract and move the download.

That would allow additional tools to be added easily, and would tidy the logic of extracting to a directory, and then moving the contents of the wordpress directory to the desired location out of the command class.

danielbachhuber · 2016-07-08T10:34:30Z

md5 checksums are not available for trunk downloads so I just display a warning. You get a similar error when trying to overwrite an existing download.

Makes sense.

One future change, which would be nice but isn't within the scope of this issue would be to move the logic for determining which tool to use (tar,PharData,ZipArchive etc) to a factory class which gives us back a instance of class fulfilling a common interface - so the command doesn't know (or care) what tool is being used to extract and move the download.

I agree. I think code abstraction could actually be a general theme for the following release (v0.25.0). Some of the internals have gotten quite messy.

danielbachhuber · 2016-07-08T11:02:51Z

php/commands/core.php

@@ -235,6 +259,29 @@ private static function _extract( $tarball, $dest ) {
 		self::_rmdir( dirname( $tempdir ) );
 	}

+	private static function _extract_zip( $zipfile, $dest ) {
+		if ( ! class_exists( 'ZipArchive' ) ) {
+			throw new \Exception( 'Extracting a zip file requires PharData' );


This exception isn't quite right. Where is it caught?

I think we should WP_CLI::error() if ZipArchive isn't find. The check should actually happen much earlier, to make sure we don't leave a partially downloaded file around.

@danielbachhuber It's caught here: https://github.com/stephenharris/wp-cli/blob/9eea025df9b69eab496b6c2a5dc3207a05702f4a/php/commands/core.php#L216

R.e. the early check, makes sense

stephenharris · 2016-07-08T12:05:20Z

@danielbachhuber Any idea why that one test is failing? It failed for me locally too, but it appears to be false positive. I don't understand why a WordPress version already exists if I have a 'clean directory'?

Oddly, it does seem to be specifically trunk. I ran it locally using nightly and it worked.

stephenharris · 2016-07-08T12:05:50Z

Link to test: 9ec1d14#diff-5106025468b362f7cee39e2f2349ef80R99

stephenharris · 2016-07-08T12:08:07Z

@danielbachhuber ignore the above, I see the error in test now. I misread the error output.

danielbachhuber · 2016-07-08T12:08:18Z

Any idea why that one test is failing? It failed for me locally too, but it appears to be false positive. I don't understand why a WordPress version already exists if I have a 'clean directory'?

Is it somehow running wp core download twice, and it's the second instance that's failing?

danielbachhuber · 2016-07-08T12:09:53Z

php/commands/core.php

@@ -134,7 +134,8 @@ public function download( $args, $assoc_args ) {
 		$locale = \WP_CLI\Utils\get_flag_value( $assoc_args, 'locale', 'en_US' );

 		if ( isset( $assoc_args['version'] ) ) {
-			$version = $assoc_args['version'];
+			$version = strtolower( $assoc_args['version'] );
+			$version = ( 'nightly' === $version ? 'trunk' : $version );
 			$download_url = $this->get_download_url($version, $locale, 'tar.gz');


When version=trunk, we're passing tar.gz as a file type and getting zip in return. Will that cause bugs later on?

Potentially yes. What would be the best way of handling this? We should ask for what we actually want. But what should happen if we ask for a 'tar.gz' of the nightly build - an exception?

Potentially yes. What would be the best way of handling this? We should ask for what we actually want.

Given it's a private method, so we don't need to worry about backwards compat concerns, could we just remove the third argument and make sure the calling function handles the archive file correctly, regardless of type?

But what should happen if we ask for a 'tar.gz' of the nightly build - an exception?

Generally, we aren't using exceptions within WP-CLI. Calling WP_CLI::error() is a better way of handling errors where WP-CLI doesn't know how to proceed.

Given it's a private method, so we don't need to worry about backwards compat concerns, could we just remove the third argument and make sure the calling function handles the archive file correctly, regardless of type?

That would be fine for the download command - but the update command passes responsibility to WordPress' Core_Upgrader, which I believe only expects a zip.

So I think we should either note in the docbloc that get_download_url() always returns a zip for trunk version or call WP_CLI::error(). The second option doesn't feel necessary.

So I think we should either note in the docbloc that get_download_url() always returns a zip for trunk version or call WP_CLI::error(). The second option doesn't feel necessary.

Ok. Just to make sure the execution path is accommodated for, let's call WP_CLI::error() when tar.gz is provided as a version for nightly.

stephenharris · 2016-07-08T12:14:14Z

The test was running download twice (as was intended), but I thought the error was occurring for the first command not the second. Fixed with 9eea025.

danielbachhuber · 2016-07-08T16:23:40Z

The test was running download twice (as was intended), but I thought the error was occurring for the first command not the second.

Heh, good find.

danielbachhuber · 2016-07-08T16:26:12Z

features/core-download.feature

@@ -95,3 +95,87 @@ Feature: Download WordPress
      """
      File removed: wp-content
      """
+
+  Scenario: Installing trunk


Can we add a test case for a non-US locale too?

What's the expected behaviour here? Currently it just ignores the locale setting as they aren't any language specific nightly builds.

I'd recommend erring if a non-en_US locale is provided. If someone is providing a locale, then they should receive an error that the nightly isn't provided in the locale, not have it unexpectedly work and download the wrong file.

danielbachhuber · 2016-07-08T16:32:54Z

@stephenharris Left some comments on small things to be fixed up.

…lds.

… a zip file as tarballs are not available.

…an error. Adds test.

… supported.

stephenharris · 2016-07-08T21:49:37Z

@danielbachhuber implemented those changes. For the extension check I had to use regular expressions as pathinfo() gives (correctly) the extension of file.tar.gz as .gz.

danielbachhuber · 2016-07-08T22:57:48Z

features/core-download.feature

+
+    When I run `wp core download --version=nightly`
+    Then the wp-settings.php file should exist
+    And the {SUITE_CACHE_DIR}/core/wordpress-nightly-en_US.zip file should exist


Oh, given the cache expiry is set to 6 months, we probably shouldn't be caching the nightly build. Sorry I missed that before :(

That's a good point, missed that.

danielbachhuber · 2016-07-08T23:01:42Z

@stephenharris Sorry, couple more things.

Also, could we have some improved description for the --version=<version> parameter in the docs? Specifically, mention that you can include a version number or nightly.

stephenharris · 2016-07-08T23:50:50Z

@danielbachhuber The documentation for --version is behaving weirdly for me. It's breaking before the 'latest' and 'nightly' (see https://cloudup.com/c6pMLEZScve). Are quotation marks not allowed there?

danielbachhuber · 2016-07-08T23:52:09Z

The documentation for --version is behaving weirdly for me. It's breaking before the 'latest' and 'nightly' (see https://cloudup.com/c6pMLEZScve). Are quotation marks not allowed there?

No, WP-CLI is trying to be smart by word-wrapping. You can just ignore it for now.

danielbachhuber · 2016-07-09T12:36:55Z

🚢 Good work with this!

danielbachhuber added command:core command:core-download reporter-feedback labels Jul 8, 2016

danielbachhuber reviewed Jul 8, 2016
View reviewed changes

Stephen Harris added 3 commits July 8, 2016 12:19

Add missing full top.

5bf4750

Make 'nightly' an alias for 'trunk'

b7423a6

Add behat tests

9ec1d14

danielbachhuber reviewed Jul 8, 2016
View reviewed changes

Fix test: use --force on the second time of downloading

9eea025

danielbachhuber reviewed Jul 8, 2016
View reviewed changes

stephenharris added 7 commits July 8, 2016 21:47

Make 'nightly' rather than 'trunk' the canonical term for nightly bui…

f47f3b4

…lds.

Correct error message

b460dd1

Add early check so we don't download a zip file if we can't unzip it.

d150a21

When requesting the download url of the nightly build, always request…

dfd697b

… a zip file as tarballs are not available.

Respond with an error if a tarball is requested for the nightly version.

a3bc842

Give an error if an unknown extension is requested.

6cc9cf4

Split ifelse statement up for readability.

32eb232

stephenharris added 3 commits July 8, 2016 22:37

Fix typo - check file type not version

b575475

If a non-default locale is selected for the nightly build, exit with …

649719a

…an error. Adds test.

Check for extension type, and return an error if the extension is not…

e9d7d83

… supported.

danielbachhuber reviewed Jul 8, 2016
View reviewed changes

danielbachhuber added this to the 0.24.0 milestone Jul 8, 2016

stephenharris added 3 commits July 9, 2016 00:22

Fix typo

b9d3a69

Don't cache nightly builds

bad34e0

Add details of acceptable values to --version documentation

da09c5a

danielbachhuber removed the reporter-feedback label Jul 9, 2016

danielbachhuber merged commit c368523 into wp-cli:master Jul 9, 2016

danielbachhuber mentioned this pull request Jul 9, 2016

Use wp core download --version=trunk in Travis #3133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'trunk' and 'nightly' version arguments for wp core download. #3127

Support 'trunk' and 'nightly' version arguments for wp core download. #3127

stephenharris commented Jul 8, 2016 •

edited by danielbachhuber

Loading

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

danielbachhuber Jul 8, 2016

danielbachhuber Jul 8, 2016

stephenharris Jul 8, 2016

stephenharris commented Jul 8, 2016

stephenharris commented Jul 8, 2016

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

danielbachhuber Jul 8, 2016

stephenharris Jul 8, 2016

danielbachhuber Jul 8, 2016

stephenharris Jul 8, 2016

danielbachhuber Jul 8, 2016

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

danielbachhuber Jul 8, 2016

stephenharris Jul 8, 2016

danielbachhuber Jul 8, 2016

danielbachhuber commented Jul 8, 2016

stephenharris commented Jul 8, 2016

danielbachhuber Jul 8, 2016

stephenharris Jul 8, 2016

danielbachhuber commented Jul 8, 2016

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

danielbachhuber commented Jul 9, 2016

Support 'trunk' and 'nightly' version arguments for wp core download. #3127

Support 'trunk' and 'nightly' version arguments for wp core download. #3127

Conversation

stephenharris commented Jul 8, 2016 • edited by danielbachhuber Loading

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenharris commented Jul 8, 2016

stephenharris commented Jul 8, 2016

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jul 8, 2016

stephenharris commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jul 8, 2016

stephenharris commented Jul 8, 2016

danielbachhuber commented Jul 8, 2016

danielbachhuber commented Jul 9, 2016

stephenharris commented Jul 8, 2016 •

edited by danielbachhuber

Loading