Improved pool optimizer to also optimize replace definitions

Toflar · Toflar · commit c996457a039b · 2020-11-03T23:40:31.000+01:00
diff --git a/src/Composer/DependencyResolver/PoolOptimizer.php b/src/Composer/DependencyResolver/PoolOptimizer.php
@@ -115,23 +115,6 @@ private function prepare()
                 $this->addIrremovablePackageConstraint($package->getName(), new Constraint('==', $package->getVersion()));
                 $this->addIrremovablePackageConstraint($package->getAliasOf()->getName(), new Constraint('==', $package->getAliasOf()->getVersion()));
             }
-
-            // This is the lazy approach. We simply do not remove any packages that are replaced or replace others here.
-            // (maybe this can be improved?)
-            if ($package->getReplaces()) {
-                $this->addIrremovablePackageConstraint($package->getName(), new Constraint('==', $package->getVersion()));
-
-                foreach ($package->getReplaces() as $link) {
-                    // Make sure we do not replace ourselves (if someone made a mistake and tagged it)
-                    // See e.g. https://github.com/BabDev/Pagerfanta/commit/fd00eb74632fecc0265327e9fe0eddc08c72b238#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780
-                    // TODO: should that go into package itself?
-                    if ($package->getName() === $link->getTarget()) {
-                        continue;
-                    }
-
-                    $this->addIrremovablePackageConstraint($link->getTarget(), $link->getConstraint());
-                }
-            }
         }
 
         // Mark the packages as irremovable based on the constraints
@@ -207,6 +190,22 @@ private function optimizeByIdenticalDependencies()
                         $groupHashParts[] = 'require:' . (string) $requireConstraint;
                     }
 
+                    if ($package->getReplaces()) {
+                        foreach ($package->getReplaces() as $link) {
+                            // Make sure we do not replace ourselves (if someone made a mistake and tagged it)
+                            // See e.g. https://github.com/BabDev/Pagerfanta/commit/fd00eb74632fecc0265327e9fe0eddc08c72b238#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780
+                            // TODO: should that go into package itself?
+                            if ($package->getName() === $link->getTarget()) {
+                                continue;
+                            }
+
+                            if (CompilingMatcher::match($link->getConstraint(), Constraint::OP_EQ, $package->getVersion())) {
+                                // Use the same hash part as the regular require hash because that's what the replacement does
+                                $groupHashParts[] = 'require:' . (string) $link->getConstraint();
+                            }
+                        }
+                    }
+
                     if (isset($this->conflictConstraintsPerPackage[$packageName])) {
                         foreach ($this->conflictConstraintsPerPackage[$packageName] as $conflictConstraint) {
                             if (CompilingMatcher::match($conflictConstraint, Constraint::OP_EQ, $package->getVersion())) {
diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/replaces.test b/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/replaces.test
@@ -1,5 +1,5 @@
 --TEST--
-Test replacers and replaced packages remain untouched.
+Test replaced packages are correctly removed.
 
 --REQUEST--
 {
@@ -48,23 +48,12 @@ Test replacers and replaced packages remain untouched.
                 "package/b": "^1.0"
             }
         },
-        {
-            "name": "package/b",
-            "version": "1.0.0",
-            "replace": {
-                "package/c": "^1.0"
-            }
-        },
         {
             "name": "package/b",
             "version": "1.0.1",
             "replace": {
                 "package/c": "^1.0"
             }
-        },
-        {
-            "name": "package/c",
-            "version": "1.0.0"
         }
 ]
 

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`--TEST--`
`2`		`-Test replacers and replaced packages remain untouched.`
	`2`	`+Test replaced packages are correctly removed.`
`3`	`3`
`4`	`4`	`--REQUEST--`
`5`	`5`	`{`
`@@ -48,23 +48,12 @@ Test replacers and replaced packages remain untouched.`
`48`	`48`	`"package/b": "^1.0"`
`49`	`49`	`}`
`50`	`50`	`},`
`51`		`- {`
`52`		`- "name": "package/b",`
`53`		`- "version": "1.0.0",`
`54`		`- "replace": {`
`55`		`- "package/c": "^1.0"`
`56`		`- }`
`57`		`- },`
`58`	`51`	`{`
`59`	`52`	`"name": "package/b",`
`60`	`53`	`"version": "1.0.1",`
`61`	`54`	`"replace": {`
`62`	`55`	`"package/c": "^1.0"`
`63`	`56`	`}`
`64`		`- },`
`65`		`- {`
`66`		`- "name": "package/c",`
`67`		`- "version": "1.0.0"`
`68`	`57`	`}`
`69`	`58`	`]`
`70`	`59`