Fixed base package replacers being removed incorrectly

[Toflar]

Fixes #11626

If the replaced package is part of the require request itself, we must also load it. We actually incorrectly adjusted the test in #11449.

I hope the changes are correct, requesting a review by @naderman :)

[Toflar]

I've investigated this problem more in-depth and I don't think this is the right solution. In fact, I think the bug is not even related to #11449 at all. I'm assuming that 11449 just by coincidence raised another issue to the surface. I'm trying to explain where I think the actual bug resides:

If you run the test case from #11626 and diff the logic between 2.5.8 and current main, for both versions, the bug is actually that drupal/classy is removed from $this->packages although it shouldn't. This is when it gets removed:

In 2.5.8, however, this was not an issue because the package was re-added to the pool again in a later step. This does not happen in main anymore due to #11449.

So I think the actual bug here is the removeLoadedPackage() call on drupal/classy.

[Seldaek]

I had a short night so take this with a grain of salt but after looking at this my impression is that the unlockPackage call is correct here, and it correctly removes the package which was loaded from the lock file, but then the $this->markPackageNameForLoadingIfRequired($request, $replace); call fails to load it as it is only required by the root package and nothing else.. So IMO the fix in this PR makes sense as it fixes that problem.

The root package not being checked there is probably simply an omission, and it seems reasonable to me that it would trigger a package load once the package is unlocked.

But anyway I got another fix which also works and is maybe more general as it handles it at the place where other unlocked packages get loaded, I'm not sure which is best to be honest.

See #11634

[Toflar]

I see, I'd vote to leaving it in the markPackageNameForLoadingIfRequired() as this is where it logically belongs. Updated the PR here to reuse isRootRequire().

[Seldaek]

Yeah both are really fine by me and more or less equivalent I think. @naderman any also-sleep-deprived counter opinion? 😆

[naderman]

Finally understood why this fix works on the original bug report. drupal/core removed the replace for drupal/classy in some 9.x version. The update of drupal/paragraphs with all deps means however that some older drupal/core version compatible with the selected versions (^9.2) get loaded which still have the replace. This in turn results in drupal/classy getting unlocked as it is now also part of "-W/with-all-dependencies" for the update. As the actually chosen newer drupal version does not replace drupal/classy, the package itself needs to be loaded. That doesn't currently happen because loading of root requirement packages on replace unlocking is missing. So this is right.

Going to still add a test specifically for this scenario outside of the multi repo replace unlocking one that had to be updated here by coincidence.

[naderman]

There was 1 failure:

1) Composer\Test\DependencyResolver\PoolBuilderTest::testPoolBuilder with data set "load-replaced-root-package-if-replacer-dropped.test" ('load-replaced-root-package-if...d.test', 'Ensure that a root package ge...rement', array('replaced/pkg-1.0.0.0', 'replacer/pkg-1.0.0.0', 'replacer/pkg-1.1.0.0', 'root/dep-1.2.0.0'), array('replaced/pkg-1.0.0.0', 'replacer/pkg-1.0.0.0', 'replacer/pkg-1.1.0.0', 'root/dep-1.2.0.0'), array(), array(array('*', '1.0.0'), array(array('root/dep', '1.1.0', array('1.*')), array('replacer/pkg', '1.1.0'), array('replaced/pkg', '1.0.0')), array('root/dep'), true), array(array(array('root/dep', '1.2.0', array('1.*')), array('replacer/pkg', '1.0.0', array('1.0.0')), array('replacer/pkg', '1.1.0'), array('replaced/pkg', '1.0.0'))), array())
Unoptimized pool does not match expected package set
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => 'replaced/pkg-1.0.0.0'
-    1 => 'replacer/pkg-1.0.0.0'
-    2 => 'replacer/pkg-1.1.0.0'
-    3 => 'root/dep-1.2.0.0'
+    0 => 'replacer/pkg-1.0.0.0'
+    1 => 'replacer/pkg-1.1.0.0'
+    2 => 'root/dep-1.2.0.0'
 )

/home/naderman/projects/composer/composer/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php:142

Worked out a test reproducing this which fails on main. Added here.

[Toflar]

Makes sense! Thanks for the test - funny we had one already (tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/load-replaced-package-if-replacer-dropped.test) - just didn't cover root requirements being replaced :) Glad we have another one now! 🥳

[T2L]

@Toflar @naderman @Seldaek thanks for such a quick turnaround! You are awesome