switch first / fallback order

src/Composer/Downloader/ZipDownloader.php

@@ -30,8 +30,10 @@ use ZipArchive;
 class ZipDownloader extends ArchiveDownloader
 {
     protected $process;
-    protected static $hasSystemUnzip;
-    protected static $hasZipArchive;
+    public static $hasSystemUnzip;
+    public static $hasZipArchive;
+    public static $isWindows;
+    private $zipArchiveObject;
 
     public function __construct(IOInterface $io, Config $config, EventDispatcher $eventDispatcher = null, Cache $cache = null, ProcessExecutor $process = null, RemoteFilesystem $rfs = null)
     {
@@ -53,6 +55,10 @@ class ZipDownloader extends ArchiveDownloader
             self::$hasZipArchive = class_exists('ZipArchive');
         }
 
+        if (null === self::$isWindows) {
+            self::$isWindows = Platform::isWindows();
+        }
+
         if (!self::$hasZipArchive && !self::$hasSystemUnzip) {
             // php.ini path is added to the error message to help users find the correct file
             $iniMessage = IniHelper::getMessage();
@@ -69,62 +75,99 @@ class ZipDownloader extends ArchiveDownloader
      *
      * @param string $file      File to extract
      * @param string $path      Path where to extract file
-     * @param bool $isFallback  If true it is called as a fallback and should not throw exception
-     * @return bool|\Exception  True if succeed, an Exception if not
+     * @param bool $isLastChance  If true it is called as a fallback and should throw an exception
+     * @return bool             Success status
      */
-    protected function extractWithSystemUnzip($file, $path, $isFallback)
+    protected function extractWithSystemUnzip($file, $path, $isLastChance)
     {
+        if (! self::$hasZipArchive) {
+            // Force Exception throwing if the Other alternative is not available
+            $isLastChance = true;
+        }
+
+        if (! self::$hasSystemUnzip && ! $isLastChance) {
+            // This was call as the favorite extract way, but is not available
+            // We switch to the alternative
+            return $this->extractWithZipArchive($file, $path, true);
+        }
+
         $processError = null;
         // When called after a ZipArchive failed, perhaps there is some files to overwrite
-        $overwrite = $isFallback ? '-o' : '';
+        $overwrite = $isLastChance ? '-o' : '';
 
         $command = 'unzip -qq '.$overwrite.' '.ProcessExecutor::escape($file).' -d '.ProcessExecutor::escape($path);
 
         try {
             if (0 === $this->process->execute($command, $ignoredOutput)) {
-                return TRUE;
+                return true;
             }
 
-            $processError = 'Failed to execute ' . $command . "\n\n" . $this->process->getErrorOutput();
+            $processError = new \RuntimeException('Failed to execute ' . $command . "\n\n" . $this->process->getErrorOutput());
         } catch (\Exception $e) {
-            $processError = 'Failed to execute ' . $command . "\n\n" . $e->getMessage();
+            $processError = $e;
+        }
+
+        if (! self::$hasZipArchive) {
+            $isLastChance = true;
         }
 
-        if ( $isFallback ) {
-            $this->io->write($processError);
+        if ($isLastChance) {
+            throw $processError;
+        } else {
+            $this->io->write($processError->getMessage());
+            $this->io->write('Unzip with unzip command failed, falling back to ZipArchive class');
+            return $this->extractWithZipArchive($file, $path, true);
         }
-        return new \RuntimeException($processError);
     }
 
     /**
      * extract $file to $path with ZipArchive
      *
-     * @param string $file File to extract
-     * @param string $path Path where to extract file
-     * @return bool|\Exception True if succeed, an Exception if not
+     * @param string $file      File to extract
+     * @param string $path      Path where to extract file
+     * @param bool $isLastChance  If true it is called as a fallback and should throw an exception
+     * @return bool             Success status
      */
-    protected function extractWithZipArchive($file, $path)
+    protected function extractWithZipArchive($file, $path, $isLastChance)
     {
-        $zipArchive = new ZipArchive();
+        if (! self::$hasSystemUnzip) {
+            // Force Exception throwing if the Other alternative is not available
+            $isLastChance = true;
+        }
 
-        if (true !== ($retval = $zipArchive->open($file))) {
-            return new \UnexpectedValueException(rtrim($this->getErrorMessage($retval, $file)."\n"), $retval);
+        if (! self::$hasZipArchive && ! $isLastChance) {
+            // This was call as the favorite extract way, but is not available
+            // We switch to the alternative
+            return $this->extractWithSystemUnzip($file, $path, true);
         }
 
-        $extractResult = FALSE;
+        $processError = null;
+        $zipArchive = $this->zipArchiveObject ?: new ZipArchive();
+
         try {
-            $extractResult = $zipArchive->extractTo($path);
-        } catch (\Exception $e ) {
-            return $e;
+            if (true === ($retval = $zipArchive->open($file))) {
+                $extractResult = $zipArchive->extractTo($path);
+
+                if (true === $extractResult) {
+                    $zipArchive->close();
+                    return true;
+                } else {
+                    $processError = new \RuntimeException(rtrim("There was an error extracting the ZIP file, it is either corrupted or using an invalid format.\n"));
+                }
+            } else {
+                $processError = new \UnexpectedValueException(rtrim($this->getErrorMessage($retval, $file)."\n"), $retval);
+            }
+        } catch (\Exception $e) {
+            $processError = $e;
         }
 
-        if (true !== $extractResult) {
-            return new \RuntimeException(rtrim("There was an error extracting the ZIP file, it is either corrupted or using an invalid format.\n"));
+        if ($isLastChance) {
+            throw $processError;
+        } else {
+            $this->io->write($processError->getMessage());
+            $this->io->write('Unzip with ZipArchive class failed, falling back to unzip command');
+            return $this->extractWithSystemUnzip($file, $path, true);
         }
-
-        $zipArchive->close();
-
-        return TRUE;
     }
 
     /**
@@ -133,42 +176,14 @@ class ZipDownloader extends ArchiveDownloader
      * @param string $file File to extract
      * @param string $path Path where to extract file
      */
-    protected function extract($file, $path)
+    public function extract($file, $path)
     {
-        $resultZipArchive = NULL;
-        $resultUnzip = NULL;
-
-        if ( self::$hasZipArchive ) {
-            // zip module is present
-            $resultZipArchive = $this->extractWithZipArchive($file, $path);
-            if ($resultZipArchive === TRUE) {
-                return;
-            }
-        }
-
-        if ( self::$hasSystemUnzip ) {
-            // we have unzip in the path
-            $isFallback=FALSE;
-            if ( $resultZipArchive !== NULL) {
-                $this->io->writeError("\nUnzip using ZipArchive failed, trying with unzip");
-                $isFallback=TRUE;
-            };
-            $resultUnzip = $this->extractWithSystemUnzip($file, $path, $isFallback);
-            if ( $resultUnzip === TRUE ) {
-                return ;
-            }
-        };
-
-        // extract functions return TRUE or an exception
-        if ( $resultZipArchive !== NULL ) {
-            // zipArchive failed
-            // unZip not present or failed too
-            throw $resultZipArchive;
+        // Each extract calls its alternative if not available or fails
+        if (self::$isWindows) {
+            $this->extractWithZipArchive($file, $path, false);
         } else {
-            // unZip failed
-            // zipArchive not available
-            throw $resultUnzip;
-        };
+            $this->extractWithSystemUnzip($file, $path, false);
+        }
     }
 
     /**

tests/Composer/Test/Downloader/ZipDownloaderTest.php

@@ -13,6 +13,7 @@
 namespace Composer\Test\Downloader;
 
 use Composer\Downloader\ZipDownloader;
+use Composer\Package\PackageInterface;
 use Composer\TestCase;
 use Composer\Util\Filesystem;
 
@@ -22,24 +23,62 @@ class ZipDownloaderTest extends TestCase
      * @var string
      */
     private $testDir;
+    private $prophet;
 
     public function setUp()
     {
         $this->testDir = $this->getUniqueTmpDirectory();
+        $this->io = $this->getMock('Composer\IO\IOInterface');
+        $this->config = $this->getMock('Composer\Config');
     }
 
+
     public function tearDown()
     {
         $fs = new Filesystem;
         $fs->removeDirectory($this->testDir);
+        ZipDownloader::$hasSystemUnzip = null;
+        ZipDownloader::$hasZipArchive = null;
+    }
+
+    public function setPrivateProperty($name, $value, $obj = null)
+    {
+        $reflectionClass = new \ReflectionClass('Composer\Downloader\ZipDownloader');
+        $reflectedProperty = $reflectionClass->getProperty($name);
+        $reflectedProperty->setAccessible(true);
+        if ($obj === null) {
+            $reflectedProperty = $reflectedProperty->setValue($value);
+        } else {
+            $reflectedProperty = $reflectedProperty->setValue($obj, $value);
+        }
     }
 
+    /**
+     * @group only
+     */
     public function testErrorMessages()
     {
         if (!class_exists('ZipArchive')) {
             $this->markTestSkipped('zip extension missing');
         }
 
+        $this->config->expects($this->at(0))
+            ->method('get')
+            ->with('disable-tls')
+            ->will($this->returnValue(false));
+        $this->config->expects($this->at(1))
+            ->method('get')
+            ->with('cafile')
+            ->will($this->returnValue(null));
+        $this->config->expects($this->at(2))
+            ->method('get')
+            ->with('capath')
+            ->will($this->returnValue(null));
+        $this->config->expects($this->at(3))
+            ->method('get')
+            ->with('vendor-dir')
+            ->will($this->returnValue($this->testDir));
+
         $packageMock = $this->getMock('Composer\Package\PackageInterface');
         $packageMock->expects($this->any())
             ->method('getDistUrl')
@@ -54,110 +93,205 @@ class ZipDownloaderTest extends TestCase
             ->will($this->returnValue(array()))
         ;
 
-        $io = $this->getMock('Composer\IO\IOInterface');
-        $config = $this->getMock('Composer\Config');
-        $config->expects($this->at(0))
-            ->method('get')
-            ->with('disable-tls')
-            ->will($this->returnValue(false));
-        $config->expects($this->at(1))
-            ->method('get')
-            ->with('cafile')
-            ->will($this->returnValue(null));
-        $config->expects($this->at(2))
-            ->method('get')
-            ->with('capath')
-            ->will($this->returnValue(null));
-        $config->expects($this->at(3))
-            ->method('get')
-            ->with('vendor-dir')
-            ->will($this->returnValue($this->testDir));
-        $downloader = new ZipDownloader($io, $config);
+        $downloader = new ZipDownloader($this->io, $this->config);
+
+        ZipDownloader::$hasSystemUnzip = false;
 
         try {
             $downloader->download($packageMock, sys_get_temp_dir().'/composer-zip-test');
             $this->fail('Download of invalid zip files should throw an exception');
-        } catch (\UnexpectedValueException $e) {
+        } catch (\Exception $e) {
             $this->assertContains('is not a zip archive', $e->getMessage());
         }
     }
 
     /**
-     * @expectedException \Exception
-     * @expectedExceptionMessage ZipArchive Failed
+     * @expectedException \RuntimeException
+     * @expectedExceptionMessage There was an error extracting the ZIP file
      */
-    function testZipArchiveOnlyFailed() {
-        $downloader = new TestDownloader($this->getMock('Composer\IO\IOInterface'));
-        $e = new \Exception("ZipArchive Failed");
-        $downloader->setUp(TRUE, FALSE, $e, NULL);
+    public function testZipArchiveOnlyFailed()
+    {
+        MockedZipDownloader::$hasSystemUnzip = false;
+        MockedZipDownloader::$hasZipArchive = true;
+        $downloader = new MockedZipDownloader($this->io, $this->config);
+
+        $zipArchive = $this->getMock('ZipArchive');
+        $zipArchive->expects($this->at(0))
+            ->method('open')
+            ->will($this->returnValue(true));
+        $zipArchive->expects($this->at(1))
+            ->method('extractTo')
+            ->will($this->returnValue(false));
+
+        $this->setPrivateProperty('zipArchiveObject', $zipArchive, $downloader);
         $downloader->extract('testfile.zip', 'vendor/dir');
     }
 
-    function testZipArchiveOnlyGood() {
-        $downloader = new TestDownloader($this->getMock('Composer\IO\IOInterface'));
-        $downloader->setUp(TRUE, FALSE, TRUE, NULL);
+    /**
+     * @group only
+     */
+    public function testZipArchiveOnlyGood()
+    {
+        MockedZipDownloader::$hasSystemUnzip = false;
+        MockedZipDownloader::$hasZipArchive = true;
+        $downloader = new MockedZipDownloader($this->io, $this->config);
+
+        $zipArchive = $this->getMock('ZipArchive');
+        $zipArchive->expects($this->at(0))
+            ->method('open')
+            ->will($this->returnValue(true));
+        $zipArchive->expects($this->at(1))
+            ->method('extractTo')
+            ->will($this->returnValue(true));
+
+        $this->setPrivateProperty('zipArchiveObject', $zipArchive, $downloader);
         $downloader->extract('testfile.zip', 'vendor/dir');
     }
 
     /**
      * @expectedException \Exception
-     * @expectedExceptionMessage SystemUnzip Failed
+     * @expectedExceptionMessage Failed to execute unzip
      */
-    function testSystemUnzipOnlyFailed() {
-        $downloader = new TestDownloader($this->getMock('Composer\IO\IOInterface'));
-        $e = new \Exception("SystemUnzip Failed");
-        $downloader->setUp(FALSE, TRUE,  NULL, $e);
+    public function testSystemUnzipOnlyFailed()
+    {
+        MockedZipDownloader::$hasSystemUnzip = true;
+        MockedZipDownloader::$hasZipArchive = false;
+        $processExecutor = $this->getMock('Composer\Util\ProcessExecutor');
+        $processExecutor->expects($this->at(0))
+            ->method('execute')
+            ->will($this->returnValue(1));
+
+        $downloader = new MockedZipDownloader($this->io, $this->config, null, null, $processExecutor);
         $downloader->extract('testfile.zip', 'vendor/dir');
     }
 
-    function testSystemUnzipOnlyGood() {
-        $downloader = new TestDownloader($this->getMock('Composer\IO\IOInterface'));
-        $downloader->setUp(FALSE, TRUE,  NULL, TRUE);
+    public function testSystemUnzipOnlyGood()
+    {
+        MockedZipDownloader::$hasSystemUnzip = true;
+        MockedZipDownloader::$hasZipArchive = false;
+        $processExecutor = $this->getMock('Composer\Util\ProcessExecutor');
+        $processExecutor->expects($this->at(0))
+            ->method('execute')
+            ->will($this->returnValue(0));
+
+        $downloader = new MockedZipDownloader($this->io, $this->config, null, null, $processExecutor);
         $downloader->extract('testfile.zip', 'vendor/dir');
     }
 
-    function testSystemUnzipFallbackGood() {
-        $downloader = new TestDownloader($this->getMock('Composer\IO\IOInterface'));
-        $e = new \Exception("test");
-        $downloader->setUp(TRUE, TRUE, $e, TRUE);
+    public function testNonWindowsFallbackGood()
+    {
+        MockedZipDownloader::$isWindows = false;
+        MockedZipDownloader::$hasSystemUnzip = true;
+        MockedZipDownloader::$hasZipArchive = true;
+
+        $processExecutor = $this->getMock('Composer\Util\ProcessExecutor');
+        $processExecutor->expects($this->at(0))
+            ->method('execute')
+            ->will($this->returnValue(1));
+
+        $zipArchive = $this->getMock('ZipArchive');
+        $zipArchive->expects($this->at(0))
+            ->method('open')
+            ->will($this->returnValue(true));
+        $zipArchive->expects($this->at(1))
+            ->method('extractTo')
+            ->will($this->returnValue(true));
+
+        $downloader = new MockedZipDownloader($this->io, $this->config, null, null, $processExecutor);
+        $this->setPrivateProperty('zipArchiveObject', $zipArchive, $downloader);
         $downloader->extract('testfile.zip', 'vendor/dir');
     }
 
     /**
      * @expectedException \Exception
-     * @expectedExceptionMessage ZipArchive Failed
+     * @expectedExceptionMessage There was an error extracting the ZIP file
      */
-    function testSystemUnzipFallbackFailed() {
-        $downloader = new TestDownloader($this->getMock('Composer\IO\IOInterface'));
-        $e1 = new \Exception("ZipArchive Failed");
-        $e2 = new \Exception("SystemUnzip Failed");
-        $downloader->setUp(TRUE, TRUE, $e1, $e2);
+    public function testNonWindowsFallbackFailed()
+    {
+        MockedZipDownloader::$isWindows = false;
+        MockedZipDownloader::$hasSystemUnzip = true;
+        MockedZipDownloader::$hasZipArchive = true;
+
+        $processExecutor = $this->getMock('Composer\Util\ProcessExecutor');
+        $processExecutor->expects($this->at(0))
+          ->method('execute')
+          ->will($this->returnValue(1));
+
+        $zipArchive = $this->getMock('ZipArchive');
+        $zipArchive->expects($this->at(0))
+          ->method('open')
+          ->will($this->returnValue(true));
+        $zipArchive->expects($this->at(1))
+          ->method('extractTo')
+          ->will($this->returnValue(false));
+
+        $downloader = new MockedZipDownloader($this->io, $this->config, null, null, $processExecutor);
+        $this->setPrivateProperty('zipArchiveObject', $zipArchive, $downloader);
         $downloader->extract('testfile.zip', 'vendor/dir');
     }
-}
 
-class TestDownloader extends ZipDownloader {
-    public function __construct($io)
+    public function testWindowsFallbackGood()
     {
-        $this->io = $io;
-    }
+        MockedZipDownloader::$isWindows = true;
+        MockedZipDownloader::$hasSystemUnzip = true;
+        MockedZipDownloader::$hasZipArchive = true;
 
-    public function extract($file, $path) {
-        parent::extract($file, $path);
+        $processExecutor = $this->getMock('Composer\Util\ProcessExecutor');
+        $processExecutor->expects($this->atLeastOnce())
+            ->method('execute')
+            ->will($this->returnValue(0));
+
+        $zipArchive = $this->getMock('ZipArchive');
+        $zipArchive->expects($this->at(0))
+            ->method('open')
+            ->will($this->returnValue(true));
+        $zipArchive->expects($this->at(1))
+            ->method('extractTo')
+            ->will($this->returnValue(false));
+
+        $downloader = new MockedZipDownloader($this->io, $this->config, null, null, $processExecutor);
+        $this->setPrivateProperty('zipArchiveObject', $zipArchive, $downloader);
+        $downloader->extract('testfile.zip', 'vendor/dir');
     }
 
-    public function setUp($zipArchive, $systemUnzip, $zipArchiveResponse, $systemUnzipResponse) {
-        self::$hasZipArchive = $zipArchive;
-        self::$hasSystemUnzip = $systemUnzip;
-        $this->zipArchiveResponse = $zipArchiveResponse;
-        $this->systemUnzipResponse = $systemUnzipResponse;
+    /**
+     * @expectedException \Exception
+     * @expectedExceptionMessage Failed to execute unzip
+     */
+    public function testWindowsFallbackFailed()
+    {
+        MockedZipDownloader::$isWindows = true;
+        MockedZipDownloader::$hasSystemUnzip = true;
+        MockedZipDownloader::$hasZipArchive = true;
+
+        $processExecutor = $this->getMock('Composer\Util\ProcessExecutor');
+        $processExecutor->expects($this->atLeastOnce())
+          ->method('execute')
+          ->will($this->returnValue(1));
+
+        $zipArchive = $this->getMock('ZipArchive');
+        $zipArchive->expects($this->at(0))
+          ->method('open')
+          ->will($this->returnValue(true));
+        $zipArchive->expects($this->at(1))
+          ->method('extractTo')
+          ->will($this->returnValue(false));
+
+        $downloader = new MockedZipDownloader($this->io, $this->config, null, null, $processExecutor);
+        $this->setPrivateProperty('zipArchiveObject', $zipArchive, $downloader);
+        $downloader->extract('testfile.zip', 'vendor/dir');
     }
+}
 
-    protected function extractWithZipArchive($file, $path) {
-        return $this->zipArchiveResponse;
+class MockedZipDownloader extends ZipDownloader
+{
+    public function download(PackageInterface $package, $path, $output = true)
+    {
+        return;
     }
 
-    protected function extractWithSystemUnzip($file, $path, $fallback) {
-        return $this->systemUnzipResponse;
+    public function extract($file, $path)
+    {
+        parent::extract($file, $path);
     }
 }