Replaced package is not installed if its replacer is updated to a version that no longer replaces#10410
Conversation
…he replaced package is not loaded
14a0074 to
ebb9725
Compare
Toflar
left a comment
There was a problem hiding this comment.
@Seldaek I am late to the party - just stumbled across this PR and wanted to see if this is still relevant against the current main.
What I did is I took @driskell's test and ran it against the current main branch. It fails so it made me think we might still need this PR.
The test is expecting this:
--EXPECT--
[
"root/no-update-1.0.0.0 (locked)",
"root/dep-1.2.0.0",
"replaced/pkg-1.0.0.0",
"replacer/pkg-1.1.0.0"
]
--EXPECT-OPTIMIZED--
[
"root/no-update-1.0.0.0 (locked)",
"root/dep-1.2.0.0",
"replaced/pkg-1.0.0.0",
"replacer/pkg-1.1.0.0"
]
While the current main is expecting this:
--EXPECT--
[
"root/no-update-1.0.0.0 (locked)",
"root/dep-1.2.0.0",
"replacer/pkg-1.1.0.0"
]
--EXPECT-OPTIMIZED--
[
"root/no-update-1.0.0.0 (locked)",
"root/dep-1.2.0.0",
"replacer/pkg-1.1.0.0"
]
As you can see, the difference is replaced/pkg-1.0.0.0.
And I do think the changes in this PR are correct. I ran the current $pool against the Solver. If I do this, I get:
Problem 1
- root/no-update is locked to version 1.0.0 and an update of this package was not requested.
- root/no-update 1.0.0 requires replaced/pkg 1.0.0 -> found replaced/pkg[1.0.0] but these were not loaded, likely because it conflicts with another require.
Which seems wrong and the changes in this PR do fix this issue and make the Solver find a valid solution. The resulting LockTransaction then is
Upgrading <info>replacer/pkg</info> (<comment>1.0.0</comment> => <comment>1.1.0</comment>)
Upgrading <info>root/dep</info> (<comment>1.1.0</comment> => <comment>1.2.0</comment>)
Installing <info>replaced/pkg</info> (<comment>1.0.0</comment>)
This seems to be the desired solution. Not sure about the Installing <info>replaced/pkg</info> (<comment>1.0.0</comment>) part as having this in the LockTransaction seems unnessecary but this might be the case in other replaced packages too and probably not related to this PR.
|
The reason That's why you get the Apologies if the package names in the test are a little unclear! |
|
To clarify - the |
|
Thanks @Toflar for checking this - I think to merge this we should have an additional test on the installer fixture level to highlight the actual bug being fixed by this change? |
Happy to help - not sure what test is required though - if you could elaborate I can take a look. I had thought as this was an issue with the package list passed to the solver the current test was enough. |
|
Basically you can promote your test to a level even higher (aka full e2e test) by adding another test to the functional tests of the Installer (https://github.com/composer/composer/tree/main/tests/Composer/Test/Fixtures/installer). Without your changes it should end up displaying the problem I mentioned in #10410 (review) and with your fix, this will change 😊 |
|
I've added the integration test in #11449. |
During discussion in #9619 @naderman discovered a failing test. This PR adds the test as it was proposed along with a possible fix for it. In summary, if a package (A) that replaces another (B) is updated to a new version that no longer replaces package B, then package B does not get unlocked and added to the pool, therefore the update would presumedly fail unless the package B was also added to the update allow list.
I believe from the discussion in #9619 the replaced package should get loaded and added automatically.