False negative when not updating .lock correctly

maglnet / ComposerRequireChecker

A CLI tool to check whether a specific composer package uses imported symbols that aren't part of its direct composer dependencies

MIT License

892 stars 73 forks source link

False negative when not updating .lock correctly #406

Open Jean85 opened 1 year ago

Jean85 commented 1 year ago

Today I was bitten by one mistake that this tool unfortunately didn't catch.

To reproduce:

start using a dev dependency under src
move the dependency manually in the composer.json
run the checker

This happened with 3.8, but I switched to the shim and I can confirm it happens on the latest version too; I've patched my CI with a composer validate which will alert me of the issue, but I would expect this tool to fail too in this case.

Ocramius commented 1 year ago

@Jean85 is that because of composer.lock and installed.json not being in sync?

I don't think this tool should be in the crossfire here: that's already a problem in your setup, and this tool is (correctly) using installed.json as authoritative location for the declaration:

https://github.com/maglnet/ComposerRequireChecker/blob/5a801ae22c3423d0ee0213861158249d23e452ec/src/ComposerRequireChecker/FileLocator/LocateComposerPackageDirectDependenciesSourceFiles.php#LL52C66-L52C66

Ocramius commented 1 year ago

I would expect this tool to fail too in this case.

I would say here that the behavior is undefined: the setup is already broken-ish at this stage :thinking:

Jean85 commented 1 year ago

I didn't know it used installed.json, I thought it read the composer.lock file. I'm not sure it's undefined though, because looking at installed.json I would expect to see the culprit still as a dev package, since I didn't change the whole setup.

Let me check manually...

Ocramius commented 1 year ago

In theory, installed.json matches whatever composer install ended up doing :D

Jean85 commented 1 year ago

Then it's inconsistent in that behavior; I just checked and:

if I do composer install --no-dev (as my deploy did, releasing a bug) I don't get the package installed (as expected, since Composer is using the composer.lock as authoritative)
if I do a plain composer install in that inconsistent state, I get the same installed.json, with the package listed as dev (which is the inconsistency that caused my false negative).

Ocramius commented 1 year ago

@Jean85 I would check with @duncan3dc why we did https://github.com/maglnet/ComposerRequireChecker/commit/57cbad2ad328b20f01f3ae818d379b6aa6ec3a32

Specifically, why installed.json vs composer.lock :thinking:

Anyway, composer.json alone does not suffice, and that's clear from #105

Jean85 commented 1 year ago

This seems the reason: https://github.com/maglnet/ComposerRequireChecker/pull/105#issue-417504005

The wikimedia/ip-set package is set to not export composer.json.

This causes the tool to ignore the package when it's installed by dist and then report the symbols as unknown.

Do you think it makes sense to report this as a bug upstream to Composer?

Ocramius commented 1 year ago

If you think there is a problem in Composer itself, yes.

IMO, composer.lock taken as authoritative could be a good step here too, perhaps.

duncan3dc commented 1 year ago

I don't recall now why I chose installed.json over composer.lock, but I would presume it's because installed.json matches the format of composer.json, so it was a low impact replacement. If composer.lock contains everything this tool needs then I think it makes sense to use that

Ocramius commented 1 year ago

Yeh, but a reminder for @Jean85 that composer.lock and composer.json being in sync is not this tool's responsibility anyway :D

Jean85 commented 1 year ago

Yeh, but a reminder for @Jean85 that composer.lock and composer.json being in sync is not this tool's responsibility anyway :D

Absolutely! But I would expect composer.lock and installed.json to be, and that's why I was proposing to report it upstream to Composer!

fredden commented 1 year ago

I don't think the problem is that composer.lock and installed.json are out of sync, but that composer.lock and has an out-of-date content-hash. When I tested this locally yesterday, I could see that making changes to composer.json alone made the alert from this tool go away, without making any changes to composer.lock nor running any composer ... commands (and by extension installed.json).

Making changes to this tool to work around the problem reported here (composer.lock isn't up to date with composer.json) seems difficult, aside from running composer validate or similar*.

I was thinking we could read composer.lock instead of composer.json, but I can't immediately think of a good way to identify direct requirements without also reading composer.json.

* See an example of how to check the freshness of a lock-file here: https://github.com/ergebnis/composer-normalize/blob/df1d0f33df60e9ea3e9eda9bfbfa60346cad817c/src/Command/NormalizeCommand.php#L174-L184