From f89e01d622cad70f302a2a3cbfb307e996645c59 Mon Sep 17 00:00:00 2001 From: Guillaume ZITTA Date: Tue, 14 Mar 2017 23:43:48 +0100 Subject: [PATCH] switch first / fallback order --- src/Composer/Downloader/ZipDownloader.php | 141 +++++----- .../Test/Downloader/ZipDownloaderTest.php | 260 +++++++++++++----- 2 files changed, 275 insertions(+), 126 deletions(-) diff --git a/src/Composer/Downloader/ZipDownloader.php b/src/Composer/Downloader/ZipDownloader.php index 88b2b51cf..73e5d61b0 100644 --- a/src/Composer/Downloader/ZipDownloader.php +++ b/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); + } } /** diff --git a/tests/Composer/Test/Downloader/ZipDownloaderTest.php b/tests/Composer/Test/Downloader/ZipDownloaderTest.php index d2204d682..4cadda847 100644 --- a/tests/Composer/Test/Downloader/ZipDownloaderTest.php +++ b/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); } }