Merge pull request #9339 from Seldaek/fix-plugin-order

Fix plugin install order for plugins modifying downloads

src/Composer/DependencyResolver/Transaction.php

@@ -248,6 +248,9 @@ class Transaction
      */
     private function movePluginsToFront(array $operations)
     {
+        $dlModifyingPluginsNoDeps = array();
+        $dlModifyingPluginsWithDeps = array();
+        $dlModifyingPluginRequires = array();
         $pluginsNoDeps = array();
         $pluginsWithDeps = array();
         $pluginRequires = array();
@@ -261,6 +264,30 @@ class Transaction
                 continue;
             }
 
+            $isDownloadsModifyingPlugin = $package->getType() === 'composer-plugin' && ($extra = $package->getExtra()) && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true;
+
+            // is this a downloads modifying plugin or a dependency of one?
+            if ($isDownloadsModifyingPlugin || count(array_intersect($package->getNames(), $dlModifyingPluginRequires))) {
+                // get the package's requires, but filter out any platform requirements
+                $requires = array_filter(array_keys($package->getRequires()), function ($req) {
+                    return !PlatformRepository::isPlatformPackage($req);
+                });
+
+                // is this a plugin with no meaningful dependencies?
+                if ($isDownloadsModifyingPlugin && !count($requires)) {
+                    // plugins with no dependencies go to the very front
+                    array_unshift($dlModifyingPluginsNoDeps, $op);
+                } else {
+                    // capture the requirements for this package so those packages will be moved up as well
+                    $dlModifyingPluginRequires = array_merge($dlModifyingPluginRequires, $requires);
+                    // move the operation to the front
+                    array_unshift($dlModifyingPluginsWithDeps, $op);
+                }
+
+                unset($operations[$idx]);
+                continue;
+            }
+
             // is this package a plugin?
             $isPlugin = $package->getType() === 'composer-plugin' || $package->getType() === 'composer-installer';
 
@@ -286,7 +313,7 @@ class Transaction
             }
         }
 
-        return array_merge($pluginsNoDeps, $pluginsWithDeps, $operations);
+        return array_merge($dlModifyingPluginsNoDeps, $dlModifyingPluginsWithDeps, $pluginsNoDeps, $pluginsWithDeps, $operations);
     }
 
     /**

src/Composer/Installer/InstallationManager.php

@@ -180,7 +180,6 @@ class InstallationManager
      */
     public function execute(RepositoryInterface $repo, array $operations, $devMode = true, $runScripts = true)
     {
-        $promises = array();
         $cleanupPromises = array();
 
         $loop = $this->loop;
@@ -234,61 +233,14 @@ class InstallationManager
         }
 
         try {
-            foreach ($operations as $index => $operation) {
-                $opType = $operation->getOperationType();
-
-                // ignoring alias ops as they don't need to execute anything at this stage
-                if (!in_array($opType, array('update', 'install', 'uninstall'))) {
-                    continue;
-                }
-
-                if ($opType === 'update') {
-                    $package = $operation->getTargetPackage();
-                    $initialPackage = $operation->getInitialPackage();
-                } else {
-                    $package = $operation->getPackage();
-                    $initialPackage = null;
-                }
-                $installer = $this->getInstaller($package->getType());
-
-                $cleanupPromises[$index] = function () use ($opType, $installer, $package, $initialPackage) {
-                    // avoid calling cleanup if the download was not even initialized for a package
-                    // as without installation source configured nothing will work
-                    if (!$package->getInstallationSource()) {
-                        return;
-                    }
-
-                    return $installer->cleanup($opType, $package, $initialPackage);
-                };
-
-                if ($opType !== 'uninstall') {
-                    $promise = $installer->download($package, $initialPackage);
-                    if ($promise) {
-                        $promises[] = $promise;
-                    }
-                }
-            }
-
-            // execute all downloads first
-            if (!empty($promises)) {
-                $progress = null;
-                if ($this->outputProgress && $this->io instanceof ConsoleIO && !$this->io->isDebug() && count($promises) > 1) {
-                    $progress = $this->io->getProgressBar();
-                }
-                $this->loop->wait($promises, $progress);
-                if ($progress) {
-                    $progress->clear();
-                }
-            }
-
-            // execute operations in batches to make sure every plugin is installed in the
-            // right order and activated before the packages depending on it are installed
+            // execute operations in batches to make sure download-modifying-plugins are installed
+            // before the other packages get downloaded
             $batches = array();
             $batch = array();
             foreach ($operations as $index => $operation) {
                 if (in_array($operation->getOperationType(), array('update', 'install'), true)) {
                     $package = $operation->getOperationType() === 'update' ? $operation->getTargetPackage() : $operation->getPackage();
-                    if ($package->getType() === 'composer-plugin' || $package->getType() === 'composer-installer') {
+                    if ($package->getType() === 'composer-plugin' && ($extra = $package->getExtra()) && isset($extra['plugin-modifies-downloads']) && $extra['plugin-modifies-downloads'] === true) {
                         if ($batch) {
                             $batches[] = $batch;
                         }
@@ -306,7 +258,7 @@ class InstallationManager
             }
 
             foreach ($batches as $batch) {
-                $this->executeBatch($repo, $batch, $cleanupPromises, $devMode, $runScripts, $operations);
+                $this->downloadAndExecuteBatch($repo, $batch, $cleanupPromises, $devMode, $runScripts, $operations);
             }
         } catch (\Exception $e) {
             $runCleanup();
@@ -334,12 +286,91 @@ class InstallationManager
         $repo->write($devMode, $this);
     }
 
+    /**
+     * @param array $operations List of operations to execute in this batch
+     * @param array $allOperations Complete list of operations to be executed in the install job, used for event listeners
+     */
+    private function downloadAndExecuteBatch(RepositoryInterface $repo, array $operations, array &$cleanupPromises, $devMode, $runScripts, array $allOperations)
+    {
+        $promises = array();
+
+        foreach ($operations as $index => $operation) {
+            $opType = $operation->getOperationType();
+
+            // ignoring alias ops as they don't need to execute anything at this stage
+            if (!in_array($opType, array('update', 'install', 'uninstall'))) {
+                continue;
+            }
+
+            if ($opType === 'update') {
+                $package = $operation->getTargetPackage();
+                $initialPackage = $operation->getInitialPackage();
+            } else {
+                $package = $operation->getPackage();
+                $initialPackage = null;
+            }
+            $installer = $this->getInstaller($package->getType());
+
+            $cleanupPromises[$index] = function () use ($opType, $installer, $package, $initialPackage) {
+                // avoid calling cleanup if the download was not even initialized for a package
+                // as without installation source configured nothing will work
+                if (!$package->getInstallationSource()) {
+                    return;
+                }
+
+                return $installer->cleanup($opType, $package, $initialPackage);
+            };
+
+            if ($opType !== 'uninstall') {
+                $promise = $installer->download($package, $initialPackage);
+                if ($promise) {
+                    $promises[] = $promise;
+                }
+            }
+        }
+
+        // execute all downloads first
+        if (count($promises)) {
+            $this->waitOnPromises($promises);
+        }
+
+        // execute operations in batches to make sure every plugin is installed in the
+        // right order and activated before the packages depending on it are installed
+        $batches = array();
+        $batch = array();
+        foreach ($operations as $index => $operation) {
+            if (in_array($operation->getOperationType(), array('update', 'install'), true)) {
+                $package = $operation->getOperationType() === 'update' ? $operation->getTargetPackage() : $operation->getPackage();
+                if ($package->getType() === 'composer-plugin' || $package->getType() === 'composer-installer') {
+                    if ($batch) {
+                        $batches[] = $batch;
+                    }
+                    $batches[] = array($index => $operation);
+                    $batch = array();
+
+                    continue;
+                }
+            }
+            $batch[$index] = $operation;
+        }
+
+        if ($batch) {
+            $batches[] = $batch;
+        }
+
+        foreach ($batches as $batch) {
+            $this->executeBatch($repo, $batch, $cleanupPromises, $devMode, $runScripts, $allOperations);
+        }
+    }
+
     /**
      * @param array $operations List of operations to execute in this batch
      * @param array $allOperations Complete list of operations to be executed in the install job, used for event listeners
      */
     private function executeBatch(RepositoryInterface $repo, array $operations, array $cleanupPromises, $devMode, $runScripts, array $allOperations)
     {
+        $promises = array();
+
         foreach ($operations as $index => $operation) {
             $opType = $operation->getOperationType();
 
@@ -397,15 +428,20 @@ class InstallationManager
         }
 
         // execute all prepare => installs/updates/removes => cleanup steps
-        if (!empty($promises)) {
-            $progress = null;
-            if ($this->outputProgress && $this->io instanceof ConsoleIO && !$this->io->isDebug() && count($promises) > 1) {
-                $progress = $this->io->getProgressBar();
-            }
-            $this->loop->wait($promises, $progress);
-            if ($progress) {
-                $progress->clear();
-            }
+        if (count($promises)) {
+            $this->waitOnPromises($promises);
+        }
+    }
+
+    private function waitOnPromises(array $promises)
+    {
+        $progress = null;
+        if ($this->outputProgress && $this->io instanceof ConsoleIO && !$this->io->isDebug() && count($promises) > 1) {
+            $progress = $this->io->getProgressBar();
+        }
+        $this->loop->wait($promises, $progress);
+        if ($progress) {
+            $progress->clear();
         }
     }
 

tests/Composer/Test/DependencyResolver/TransactionTest.php

@@ -42,8 +42,28 @@ class TransactionTest extends TestCase
             $packageG = $this->getPackage('g/g', '1.0.0'),
             $packageA0first = $this->getPackage('a0/first', '1.2.3'),
             $packageFalias2 = $this->getAliasPackage($packageF, 'dev-bar'),
+            $plugin = $this->getPackage('x/plugin', '1.0.0'),
+            $plugin2Dep = $this->getPackage('x/plugin2-dep', '1.0.0'),
+            $plugin2 = $this->getPackage('x/plugin2', '1.0.0'),
+            $dlModifyingPlugin = $this->getPackage('x/downloads-modifying', '1.0.0'),
+            $dlModifyingPlugin2Dep = $this->getPackage('x/downloads-modifying2-dep', '1.0.0'),
+            $dlModifyingPlugin2 = $this->getPackage('x/downloads-modifying2', '1.0.0'),
         );
 
+        $plugin->setType('composer-installer');
+        foreach (array($plugin2, $dlModifyingPlugin, $dlModifyingPlugin2) as $pluginPackage) {
+            $pluginPackage->setType('composer-plugin');
+        }
+
+        $plugin2->setRequires(array(
+            'x/plugin2-dep' => new Link('x/plugin2', 'x/plugin2-dep', $this->getVersionConstraint('=', '1.0.0'), Link::TYPE_REQUIRE),
+        ));
+        $dlModifyingPlugin2->setRequires(array(
+            'x/downloads-modifying2-dep' => new Link('x/downloads-modifying2', 'x/downloads-modifying2-dep', $this->getVersionConstraint('=', '1.0.0'), Link::TYPE_REQUIRE),
+        ));
+        $dlModifyingPlugin->setExtra(array('plugin-modifies-downloads' => true));
+        $dlModifyingPlugin2->setExtra(array('plugin-modifies-downloads' => true));
+
         $packageD->setRequires(array(
             'f/f' => new Link('d/d', 'f/f', $this->getVersionConstraint('>', '0.2'), Link::TYPE_REQUIRE),
             'g/provider' => new Link('d/d', 'g/provider', $this->getVersionConstraint('>', '0.2'), Link::TYPE_REQUIRE),
@@ -54,6 +74,12 @@ class TransactionTest extends TestCase
             array('job' => 'uninstall', 'package' => $packageC),
             array('job' => 'uninstall', 'package' => $packageE),
             array('job' => 'markAliasUninstalled', 'package' => $packageEalias),
+            array('job' => 'install', 'package' => $dlModifyingPlugin),
+            array('job' => 'install', 'package' => $dlModifyingPlugin2Dep),
+            array('job' => 'install', 'package' => $dlModifyingPlugin2),
+            array('job' => 'install', 'package' => $plugin),
+            array('job' => 'install', 'package' => $plugin2Dep),
+            array('job' => 'install', 'package' => $plugin2),
             array('job' => 'install', 'package' => $packageA0first),
             array('job' => 'update', 'from' => $packageB, 'to' => $packageBnew),
             array('job' => 'install', 'package' => $packageG),