Fixed support with composer v2

[kpicaza]

Q	A
Documentation	yes
Bugfix	yes
BC Break	no

Description

Fixes the type error described in the issue #11, but it removes part of the functionality added in issue #9.

[weierophinney]

@boesing I'll have you review this, as you did the work to make compat with Composer v2 happen initially.

[kpicaza]

The OperationInterface::getReason method will be removed https://github.com/composer/composer/issues/9230 and https://github.com/composer/composer/issues/8827. I am working on another approach to maintain the same functionality with whitelisted dev dependencies as in composer v1 without breaking composer 2 support. With this patch, it should not fail in current composer v2.0.0-RC1, but it will fail once the OperationInterface::getReason method was removed.

--UPDATE--

In composer v2, the install action is based on composer.lock contents, I found a way to obtain the same data as using the OperationInterface::getReason method, by reading the composer.lock data.

[boesing]

@weierophinney I am on vacation. Probably I get some time next week to have a look into this. If this is urgent, we might consider fixing it without me reviewing this.

[geerteltink]

As mentioned in #11 I think the chosen implementation is wrong. The used method officially returns a string or null and is forced

https://github.com/laminas/laminas-component-installer/blob/79665f90d81d6c273c359a46cc22e3bea47ca0b8/src/ComponentInstaller.php#L194-L195

The GenericRule is forced here because composer v1 returned it. In composer v2 this functionality has changed:

Install now really means synchronization with the lock file, so during package install there is no information available on rules from a composer update which had an effect on the selection of the package.

So there is no access to the rules anymore. As suggested in #11 we either remove the whitelist part as it won't work anymore or we implement it differently. This means that packages in require-dev will be added to development.config.php automatically.

If you want whitelisting to automatically install dependencies from a require-dev package (introduced in #9), we could also think about a extras.laminas.component-whitelist-dev section. This cleans up the original whitelist and we don't need access to the affected rules. We only need to check the current installed package against the whitelist (modules.config.php) and whitelist-dev (development.config.php) and install it into the targeted file.

[kpicaza]

Thanks @geerteltink, I appreciate a lot your time. I'm worried about it was my first PR to your project, and I added a bug. What would be the best method to tackle it? Fixing the breaking error in this PR(the initial patch + docs), and then opening another PR applying one of the proposed approaches?

[boesing]

Oh okay, I finally understand whats going on. You are trying to find out if a package is being installed by a dependency requirement by checking the installation reason.

I think this PR will fix the problem with the nullable Rule but the rule will always only contain the direct package which led to the installation.

Thus, I assume that this dependency tree might not work (didn't tested that yet): foo/bar => foo/baz => foo/foo

While foo/foo is being installed due to foo/baz, you cannot know if foo/bar is a dev or non-dev dependency.

You might have a look on how composer v2 solves the composer why command. I've studied that command when working on laminas/laminas-dependency-plugin as it was one of my approaches to provide composer v2 compatibility. Imho, we should not mess around with such logic, as it will totally blow your mind at some point.

Some questions I'd like to ask regarding this feature which I dont need to get answered, but you probably want to think about:

• what if foo/foo is being required by two packages, once by a non-dev dependency and once by a dev-dependency?
• what if one of those packages is being dropped due to composer remove? do we want to automatically move the foo/foo package from one to the other config file?

I've been in this situation in laminas/laminas-depdenency-plugin and deleted 2 weeks of work as I realized that messing with such configuration will never lead to a good solution.

I hope I could give some useful feedback here.

[boesing]

Hmm, I am actually working on #10 and the return value of OperationInterface::getReason is string. Why exactly do we assume that its either GenericRule or null when it should be string (obviously, the return value has to be string|null but that an issue of composer)?

~I cannot even find any call to InstallOperation::__construct where the reason argument is passed.~ OK, that was because I had a look at composer v2

I am not that deep into what #9 was initially about but probably we might totally re-work this feature at all instead of duct-taping bugs.

I've created a PR for composer to fix the return type of OperationInterface::getReason: composer/composer#9263

[boesing]

@kpicaza Would you mind to test this?

Thats an approach which probably works on composer v1 and v2 (I've copied the composer why logic).

[kpicaza]

@boesing sure, I'll give you feedback in the next few days.

[boesing]

Closing this in favor of #16