search

laminas / laminas-component-installer

Composer plugin for injecting modules and configuration providers into application configuration
https://docs.laminas.dev/laminas-component-installer/
BSD 3-Clause "New" or "Revised" License
27 stars 12 forks source link

composer 2 issue #11

Closed geerteltink closed 4 years ago

geerteltink commented 4 years ago

Bug Report

Q A
Version(s) 2.3.0
PHP 7.4

Summary

Fatal error:  Uncaught TypeError: Argument 1 passed to Laminas\ComponentInstaller\ComponentInstaller::isADevDependency() must be an instance of Composer\DependencyResolver\GenericRule, null given, called in /project/vendor/laminas/laminas-component-installer/src/ComponentInstaller.php on line 214 and defined in /project/vendor/laminas/laminas-component-installer/src/ComponentInstaller.php:862
Stack trace:
#0 /project/vendor/laminas/laminas-component-installer/src/ComponentInstaller.php(214): Laminas\ComponentInstaller\ComponentInstaller->isADevDependency()
#1 [internal function]: Laminas\ComponentInstaller\ComponentInstaller->onPostPackageInstall()
#2 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(165): call_user_func()
#3 phar:///usr/local/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(118): Composer\EventDispatcher\EventDispatcher->doDispatch()
#4 phar:///usr/local/bin/compose in /project/vendor/laminas/laminas-component-installer/src/ComponentInstaller.php on line 862
geerteltink commented 4 years ago

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

These lines seems to be the issue in composer 2, added in #9.

$event->getOperation()->getReason(); returns either a string or null, it is never a GenericRule.

@kpicaza any ideas?

kpicaza commented 4 years ago

Hi @geerteltink, I'm checking it with composer 2 now.

geerteltink commented 4 years ago

FYI, locking to ~2.2.0 fixes the issue with composer 2 and works perfectly.

kpicaza commented 4 years ago

I use the OperationInterface::getReason() method to know if the currently installed dependency is a dependency of another package in dev-dependencies, to inject it in the development-config.php file, or the primary config file. In Composer 1.x version, all post-package-install events have the reason filled with a GenericRule object. Since the 2.x version, the reason is always null.

I have an easy patch to avoid breaking. The issue with it is that the wanted functionality in #9 will fail because it will install the "dev-whitelisted" package in the correct config file, but the "dev-whitelisted" package's dependencies will be installed in the primary config file by default if they are whitelisted.
image

The next screenshots show how the dependencies of the dev-whitelisted package will install in the primary config file when they haven't the operation reason filled. image image image

@geerteltink Should I also open the Pull Request with this hotfix? I'm finding how to get the required info from the composer event, but I haven't found it currently.

kpicaza commented 4 years ago

I opened an issue in composer/composer https://github.com/composer/composer/issues/9230 to ask if that is the expected behavior with the event's operation reason in composer v2.

geerteltink commented 4 years ago

Looks like it was removed in 2.0.0.alpha3:

Is there any other way we can determine the parent package?

I have an easy patch to avoid breaking. The issue with it is that the wanted functionality in #9 will fail because it will install the "dev-whitelisted" package in the correct config file, but the "dev-whitelisted" package's dependencies will be installed in the primary config file by default if they are whitelisted.

So if I understand correctly, in this example you want to install laminas-api-tools/api-tools as a dev requirement. That part seems to work with you patch (without whitelisting it). You want to install all other api tools as well, which are whitelisted. However whitelisted packages will go to modules.config.php and those de dependencies should go into development.config.php.

I can think of several solutions.

  1. Ask the composer project to bring back the install reason.
  2. Install all the packages independently as dev requirements, no need for whitelisting.
  3. Add an extras.laminas.component-whitelist-dev section that makes sure that installed packages end up in the development config file, which might be even cleaner than the current solution for composer v1.
  4. Accept #12 and mention the function does not work in v2 or only when doing it via option 2.

/cc @weierophinney any thoughts?

kpicaza commented 4 years ago

Thanks for your response, and sorry about my bad English. You understand the example correctly.

Is there any other way we can determine the parent package?

I'm am debugging the composer event and I can't find it at least on the create-project command context.

  1. Install all the packages independently as dev requirements, no need for whitelisting.

I think that it should be the right way because if I install a package as a dev dependency, I'll expect to have its dependencies as dev too.

  1. Accept #12 and mention the function does not work in v2 or only when doing it via option 2.

I will update the readme to explain the functionality and its current limitations with Composer v2

boesing commented 4 years ago

Fixed with #16