Issue in composer.lock generation ?

[drupol]

Hello,

Today I encountered an unexpected behaviour while attempting to upgrade from version 2.6.6 to 2.7.1. After making a thorough bisect, I pinpointed the introduction of the weird behaviour to this specific commit: 042a8c2 (related to #11787).

Originally, this issue has been found while Composer is used in conjunction with a custom plugin: nix-community/composer-local-repo-plugin. Note, this issue cannot be reproduced (pun intended!) when using Composer < 2.7.

Reproduce the issue with the Composer plugin

What makes this issue complicated is that the core of the problem involves a custom Composer plugin (nix-community/composer-local-repo-plugin), which is essential for illustrating the encountered issue. This plugin plays a critical role in achieving total reproducibility for PHP/Composer-based projects within the Nix ecosystem (introduced in NixOS/nixpkgs#248184).

The plugin works by generating a local Composer repository (https://getcomposer.org/doc/05-repositories.md#composer) from a project's composer.json and composer.lock files, including the packages.json file, then, update each package's dist information in composer.lock file to point to the local repository instead. This repository then serves as a composer repository type within composer.json. Our objective is to use this plugin to create a stable version of the files that will be of the vendor directory at the end (using the Composer cache is not a valid option). This stability is crucial for Nix, as it relies on predictable inputs (in this case, the composer repository) to calculate its hashes accurately. The development and implementation of this plugin were motivated by the necessity for predictable outputs to ensure the reproducibility of PHP/Composer based builds... and it was working super fine so far (despite that I wish I could get rid of the plugin one day, ideas are welcome!). The issue emerged following the aforementioned commit, affecting the plugin's operation and, consequently, the reproducibility of our PHP/Composer-based projects. This situation underscores the importance of that small plugin in the Nix ecosystem and the need for a resolution that maintains the integrity of our requirements. Find more details on how to install and use the plugin in the README file of the project.

No Nix is needed to reproduce the issue locally, you can reproduce it:

1. By installing Composer >= 2.7.0
2. By installing the plugin: composer global require nix-community/composer-local-repo-plugin
3. By cloning the following gist: git clone https://gist.github.com/ab25340ea5fa46e3c2f49c1d9ed5eb71.git
4. By running the shell script run.sh from that gist

Reproduce the issue without the Composer plugin

Since many commands are required to reproduce the issue, I've set up a temporary repository at https://github.com/drupol/composer-issue-11850 containing the minimum required file to reproduce the issue. Follow the next steps to quickly reproduce it locally:

1. Clone the repo: git clone https://github.com/drupol/composer-issue-11850
2. In that new directory, run the script: ./run.sh

To summarize, this PR contains only 2 commits:

• The first commit fix (or get rid?) of the issue by adding a simple extra condition in the complex logic that generates the composer.lock file. I'm still a bit unsure of the proposed fix, this is the reason why I ask you to carefully review this. The fix add a simple checks on $package->getDistType() but maybe we should do further checks, perhaps we should also compare $package->getDistUrl() and $package->getDistSha1Checksum() and if any of those properties have changed. I had a first version implementing that, but I changed it until I have some feedback from you.
• The second commit refactor that logic by just using early conditions to reduce the code indentation and give a better understanding on what is going on in the logic workflow. It is a no-op commit and totally opinionated, I can get rid of it if you don't like it.

Please, let me know what you think of that, as I'm willing to fix the issue in Nix and also contribute to making Composer better.

Thanks.

[Seldaek]

I'm not sure I grasp why the dist type changes, but I can see if it changed then yeah maybe we should not update it.. but then I'd still update the source reference I think. So probably need to make only the dist updates conditional on having the same dist type.

If the dist type changed we should probably also just call setSourceReference instead of setSourceDistReferences to ensure we leave the dist reference alone.

[drupol]

Nice, glad this has been recognized as a bug.

Now, do you think we should do the following?

$packageDist = sprintf(
    '%s%s%s',
    $package->getDistSha1Checksum(),
    $package->getDistType(),
    $package->getDistUrl()
);
$presentPackageDist = sprintf(
    '%s%s%s',
    $presentPackage->getDistSha1Checksum(),
    $presentPackage->getDistType(),
    $presentPackage->getDistUrl()
);

if ($presentPackage->getSourceReference() && $presentPackage->getSourceType() === $package->getSourceType() && $presentPackageDist === $packageDist) {
  // ... rest of the code
}

[drupol]

(just rebased since you fixed the CI)

edit: looks like it's not fixed yet :(

[Seldaek]

Now, do you think we should do the following?

no. Because if the dist reference changes, the url will likely be different, and the sha1 as well, so this does not mean anything.

[drupol]

My idea was to check if anything has changed in the dist information (Checksum, type or URL).

But OK fair enough. Is there left to do in here?

[drupol]

I confirm that the implemented changes fixes the issue.

[Seldaek]

@naderman can you please review the approach here? Most important is the last commit. It switches the mirror update from: "take newest package and reapply the old references to it" to "keep old package but apply newest dist/source urls and mirrors to it". I think it's a more meaningful way to do this, and it should still achieve the desired goal.. But well it carries some risk of unintended consequences for sure.

[drupol]

That last commit efa03f9 is breaking it again :(

[drupol]

@Seldaek To avoid overriding what you did in this branch, I've created another branch. It contains a fix that makes this one works again, find it here: drupol#1

Basically, the fix is just that:

 if ($presentPackage->getDistType() !== $package->getDistType()) {
-     return $presentPackage;
+     continue;
 }

The rest of the changes are minimal, just to remove the call to count.

[Seldaek]

Ok merging this as it fixes other issues and does not break nix more than it already was broken ;)