From 5ed18d9aa2d08931cb9b4a61cb8beab388011e8d Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 24 Feb 2014 18:40:33 +0100 Subject: [PATCH 1/4] Fail over from source to dist and vice versa when downloads fail Any RuntimeException descendent will be caught and cause another download attempt using either source or dist depending on what was attempted first. --- src/Composer/Downloader/DownloadManager.php | 47 ++++++-- src/Composer/Factory.php | 2 +- tests/Composer/Test/ComposerTest.php | 3 +- .../Test/Downloader/DownloadManagerTest.php | 104 ++++++++++++++---- tests/Composer/Test/InstallerTest.php | 2 +- 5 files changed, 123 insertions(+), 35 deletions(-) diff --git a/src/Composer/Downloader/DownloadManager.php b/src/Composer/Downloader/DownloadManager.php index 51649cc3a..39a16611e 100644 --- a/src/Composer/Downloader/DownloadManager.php +++ b/src/Composer/Downloader/DownloadManager.php @@ -14,6 +14,7 @@ namespace Composer\Downloader; use Composer\Package\PackageInterface; use Composer\Downloader\DownloaderInterface; +use Composer\IO\IOInterface; use Composer\Util\Filesystem; /** @@ -23,6 +24,7 @@ use Composer\Util\Filesystem; */ class DownloadManager { + private $io; private $preferDist = false; private $preferSource = false; private $filesystem; @@ -31,11 +33,13 @@ class DownloadManager /** * Initializes download manager. * + * @param IOInterface $io The Input Output Interface * @param bool $preferSource prefer downloading from source * @param Filesystem|null $filesystem custom Filesystem object */ - public function __construct($preferSource = false, Filesystem $filesystem = null) + public function __construct(IOInterface $io, $preferSource = false, Filesystem $filesystem = null) { + $this->io = $io; $this->preferSource = $preferSource; $this->filesystem = $filesystem ?: new Filesystem(); } @@ -168,19 +172,44 @@ class DownloadManager $sourceType = $package->getSourceType(); $distType = $package->getDistType(); - if ((!$package->isDev() || $this->preferDist || !$sourceType) && !($preferSource && $sourceType) && $distType) { - $package->setInstallationSource('dist'); - } elseif ($sourceType) { - $package->setInstallationSource('source'); - } else { + $wantDist = !$package->isDev() || $this->preferDist || !$sourceType; + $wantSource = $preferSource && $sourceType; + + $types = array(); + if ($sourceType) { + $types[] = 'source'; + } + if ($distType) { + $types[] = 'dist'; + } + + if (empty($types)) { throw new \InvalidArgumentException('Package '.$package.' must have a source or dist specified'); } + if ($wantDist && !$wantSource) { + $types = array_reverse($types); + } + $this->filesystem->ensureDirectoryExists($targetDir); - $downloader = $this->getDownloaderForInstalledPackage($package); - if ($downloader) { - $downloader->download($package, $targetDir); + foreach ($types as $source) { + $package->setInstallationSource($source); + try { + $downloader = $this->getDownloaderForInstalledPackage($package); + if ($downloader) { + $downloader->download($package, $targetDir); + } + break; + } catch (\RuntimeException $e) { + $this->io->write( + 'Caught an exception while trying to download '. + $package->getPrettyString(). + ': '. + $e->getMessage().'' + ); + continue; + } } } diff --git a/src/Composer/Factory.php b/src/Composer/Factory.php index f829cb459..27f83dd87 100644 --- a/src/Composer/Factory.php +++ b/src/Composer/Factory.php @@ -344,7 +344,7 @@ class Factory $cache = new Cache($io, $config->get('cache-files-dir'), 'a-z0-9_./'); } - $dm = new Downloader\DownloadManager(); + $dm = new Downloader\DownloadManager($io); switch ($config->get('preferred-install')) { case 'dist': $dm->setPreferDist(true); diff --git a/tests/Composer/Test/ComposerTest.php b/tests/Composer/Test/ComposerTest.php index f667e88e5..df00c36f7 100644 --- a/tests/Composer/Test/ComposerTest.php +++ b/tests/Composer/Test/ComposerTest.php @@ -47,7 +47,8 @@ class ComposerTest extends TestCase public function testSetGetDownloadManager() { $composer = new Composer(); - $manager = $this->getMock('Composer\Downloader\DownloadManager'); + $io = $this->getMock('Composer\IO\IOInterface'); + $manager = $this->getMock('Composer\Downloader\DownloadManager', array(), array($io)); $composer->setDownloadManager($manager); $this->assertSame($manager, $composer->getDownloadManager()); diff --git a/tests/Composer/Test/Downloader/DownloadManagerTest.php b/tests/Composer/Test/Downloader/DownloadManagerTest.php index 48242a818..1728c583e 100644 --- a/tests/Composer/Test/Downloader/DownloadManagerTest.php +++ b/tests/Composer/Test/Downloader/DownloadManagerTest.php @@ -17,16 +17,18 @@ use Composer\Downloader\DownloadManager; class DownloadManagerTest extends \PHPUnit_Framework_TestCase { protected $filesystem; + protected $io; public function setUp() { $this->filesystem = $this->getMock('Composer\Util\Filesystem'); + $this->io = $this->getMock('Composer\IO\IOInterface'); } public function testSetGetDownloader() { $downloader = $this->createDownloaderMock(); - $manager = new DownloadManager(false, $this->filesystem); + $manager = new DownloadManager($this->io, false, $this->filesystem); $manager->setDownloader('test', $downloader); $this->assertSame($downloader, $manager->getDownloader('test')); @@ -43,7 +45,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->method('getInstallationSource') ->will($this->returnValue(null)); - $manager = new DownloadManager(false, $this->filesystem); + $manager = new DownloadManager($this->io, false, $this->filesystem); $this->setExpectedException('InvalidArgumentException'); @@ -69,7 +71,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue('dist')); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloader')) ->getMock(); @@ -101,7 +103,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue('source')); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloader')) ->getMock(); @@ -135,7 +137,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue('source')); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloader')) ->getMock(); @@ -167,7 +169,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue('dist')); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloader')) ->getMock(); @@ -190,7 +192,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->method('getType') ->will($this->returnValue('metapackage')); - $manager = new DownloadManager(false, $this->filesystem); + $manager = new DownloadManager($this->io, false, $this->filesystem); $this->assertNull($manager->getDownloaderForInstalledPackage($package)); } @@ -219,7 +221,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'target_dir'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -231,6 +233,62 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase $manager->download($package, 'target_dir'); } + public function testFullPackageDownloadFailover() + { + $package = $this->createPackageMock(); + $package + ->expects($this->once()) + ->method('getSourceType') + ->will($this->returnValue('git')); + $package + ->expects($this->once()) + ->method('getDistType') + ->will($this->returnValue('pear')); + $package + ->expects($this->any()) + ->method('getPrettyString') + ->will($this->returnValue('prettyPackage')); + + $package + ->expects($this->at(3)) + ->method('setInstallationSource') + ->with('dist'); + $package + ->expects($this->at(5)) + ->method('setInstallationSource') + ->with('source'); + + $downloaderFail = $this->createDownloaderMock(); + $downloaderFail + ->expects($this->once()) + ->method('download') + ->with($package, 'target_dir') + ->will($this->throwException(new \RuntimeException("Foo"))); + + $downloaderSuccess = $this->createDownloaderMock(); + $downloaderSuccess + ->expects($this->once()) + ->method('download') + ->with($package, 'target_dir'); + + $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') + ->setConstructorArgs(array($this->io, false, $this->filesystem)) + ->setMethods(array('getDownloaderForInstalledPackage')) + ->getMock(); + $manager + ->expects($this->at(0)) + ->method('getDownloaderForInstalledPackage') + ->with($package) + ->will($this->returnValue($downloaderFail)); + $manager + ->expects($this->at(1)) + ->method('getDownloaderForInstalledPackage') + ->with($package) + ->will($this->returnValue($downloaderSuccess)); + + $manager->download($package, 'target_dir'); + } + public function testBadPackageDownload() { $package = $this->createPackageMock(); @@ -243,7 +301,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->method('getDistType') ->will($this->returnValue(null)); - $manager = new DownloadManager(false, $this->filesystem); + $manager = new DownloadManager($this->io, false, $this->filesystem); $this->setExpectedException('InvalidArgumentException'); $manager->download($package, 'target_dir'); @@ -273,7 +331,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'target_dir'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -309,7 +367,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'target_dir'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -339,7 +397,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with('source'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -375,7 +433,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'target_dir'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -412,7 +470,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'target_dir'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -449,7 +507,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'target_dir'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -474,7 +532,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->method('getDistType') ->will($this->returnValue(null)); - $manager = new DownloadManager(false, $this->filesystem); + $manager = new DownloadManager($this->io, false, $this->filesystem); $manager->setPreferSource(true); $this->setExpectedException('InvalidArgumentException'); @@ -510,7 +568,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($initial, $target, 'vendor/bundles/FOS/UserBundle'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -547,7 +605,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($initial, 'vendor/bundles/FOS/UserBundle'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage', 'download')) ->getMock(); $manager @@ -588,7 +646,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($initial, $target, 'vendor/pkg'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage', 'download')) ->getMock(); $manager @@ -625,7 +683,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($initial, 'vendor/pkg'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage', 'download')) ->getMock(); $manager @@ -647,7 +705,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase $target = $this->createPackageMock(); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -670,7 +728,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase ->with($package, 'vendor/bundles/FOS/UserBundle'); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager @@ -687,7 +745,7 @@ class DownloadManagerTest extends \PHPUnit_Framework_TestCase $package = $this->createPackageMock(); $manager = $this->getMockBuilder('Composer\Downloader\DownloadManager') - ->setConstructorArgs(array(false, $this->filesystem)) + ->setConstructorArgs(array($this->io, false, $this->filesystem)) ->setMethods(array('getDownloaderForInstalledPackage')) ->getMock(); $manager diff --git a/tests/Composer/Test/InstallerTest.php b/tests/Composer/Test/InstallerTest.php index 4e992c577..cc17973bc 100644 --- a/tests/Composer/Test/InstallerTest.php +++ b/tests/Composer/Test/InstallerTest.php @@ -51,7 +51,7 @@ class InstallerTest extends TestCase { $io = $this->getMock('Composer\IO\IOInterface'); - $downloadManager = $this->getMock('Composer\Downloader\DownloadManager'); + $downloadManager = $this->getMock('Composer\Downloader\DownloadManager', array(), array($io)); $config = $this->getMock('Composer\Config'); $repositoryManager = new RepositoryManager($io, $config); From 35fbe3fd42b7b87e3a8b225d0631bdf64358cbf6 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 24 Feb 2014 18:53:34 +0100 Subject: [PATCH 2/4] Download failover means we can now always try github zip urls for dist --- src/Composer/Repository/Vcs/GitHubDriver.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Composer/Repository/Vcs/GitHubDriver.php b/src/Composer/Repository/Vcs/GitHubDriver.php index 3cf2befb5..1bcbf8a3e 100644 --- a/src/Composer/Repository/Vcs/GitHubDriver.php +++ b/src/Composer/Repository/Vcs/GitHubDriver.php @@ -117,10 +117,6 @@ class GitHubDriver extends VcsDriver */ public function getDist($identifier) { - if ($this->gitDriver) { - return $this->gitDriver->getDist($identifier); - } - $url = $this->getApiUrl() . '/repos/'.$this->owner.'/'.$this->repository.'/zipball/'.$identifier; return array('type' => 'zip', 'url' => $url, 'reference' => $identifier, 'shasum' => ''); From 1ccf4b0fc337aaecd5b34c6400c16161bfd5b849 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 24 Feb 2014 19:51:03 +0100 Subject: [PATCH 3/4] Correct the tests for dist urls for github --- .../Composer/Test/Repository/Vcs/GitHubDriverTest.php | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/Composer/Test/Repository/Vcs/GitHubDriverTest.php b/tests/Composer/Test/Repository/Vcs/GitHubDriverTest.php index a3cb9dd23..906c20071 100644 --- a/tests/Composer/Test/Repository/Vcs/GitHubDriverTest.php +++ b/tests/Composer/Test/Repository/Vcs/GitHubDriverTest.php @@ -283,19 +283,16 @@ class GitHubDriverTest extends \PHPUnit_Framework_TestCase $this->assertEquals('test_master', $gitHubDriver->getRootIdentifier()); - // Dist is not available for GitDriver - $dist = $gitHubDriver->getDist($identifier); - $this->assertNull($dist); + $dist = $gitHubDriver->getDist($sha); + $this->assertEquals('zip', $dist['type']); + $this->assertEquals('https://api.github.com/repos/composer/packagist/zipball/SOMESHA', $dist['url']); + $this->assertEquals($sha, $dist['reference']); $source = $gitHubDriver->getSource($identifier); $this->assertEquals('git', $source['type']); $this->assertEquals($repoSshUrl, $source['url']); $this->assertEquals($identifier, $source['reference']); - // Dist is not available for GitDriver - $dist = $gitHubDriver->getDist($sha); - $this->assertNull($dist); - $source = $gitHubDriver->getSource($sha); $this->assertEquals('git', $source['type']); $this->assertEquals($repoSshUrl, $source['url']); From 31fd6c233c29cc520ead2b01a7ba4b92a601ef56 Mon Sep 17 00:00:00 2001 From: Nils Adermann Date: Mon, 24 Feb 2014 19:52:20 +0100 Subject: [PATCH 4/4] Rethrow download exceptions when no options left & clean up code --- src/Composer/Downloader/DownloadManager.php | 22 ++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Composer/Downloader/DownloadManager.php b/src/Composer/Downloader/DownloadManager.php index 39a16611e..ab35d488f 100644 --- a/src/Composer/Downloader/DownloadManager.php +++ b/src/Composer/Downloader/DownloadManager.php @@ -172,28 +172,25 @@ class DownloadManager $sourceType = $package->getSourceType(); $distType = $package->getDistType(); - $wantDist = !$package->isDev() || $this->preferDist || !$sourceType; - $wantSource = $preferSource && $sourceType; - - $types = array(); + $sources = array(); if ($sourceType) { - $types[] = 'source'; + $sources[] = 'source'; } if ($distType) { - $types[] = 'dist'; + $sources[] = 'dist'; } - if (empty($types)) { + if (empty($sources)) { throw new \InvalidArgumentException('Package '.$package.' must have a source or dist specified'); } - if ($wantDist && !$wantSource) { - $types = array_reverse($types); + if ((!$package->isDev() || $this->preferDist) && !$preferSource) { + $sources = array_reverse($sources); } $this->filesystem->ensureDirectoryExists($targetDir); - foreach ($types as $source) { + foreach ($sources as $i => $source) { $package->setInstallationSource($source); try { $downloader = $this->getDownloaderForInstalledPackage($package); @@ -202,13 +199,16 @@ class DownloadManager } break; } catch (\RuntimeException $e) { + if ($i == count($sources) - 1) { + throw $e; + } + $this->io->write( 'Caught an exception while trying to download '. $package->getPrettyString(). ': '. $e->getMessage().'' ); - continue; } } }