-
-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix type error when trying to discover module dependencies for metapackages #70
Fix type error when trying to discover module dependencies for metapackages #70
Conversation
Composer metapackages are not installed same way as other packages. With no install location there is nothing to discover. Starting with composer 2.5.6 the return type for `Composer\Installer\InstallationManager::getInstallPath()` method becomes nullable. This fixes type error produced for metapackages with defined extras. Signed-off-by: Aleksei Khudiakov <aleksey@xerkus.pro>
I am not sure how to test this. return type is defined as string, so I can't use mocks to force return of null without bumping composer to latest and I don't want to do that because we do not control composer version. The test for empty install path has no apparent change in behavior, so not sure how to test that one either. |
src/ComponentInstaller.php
Outdated
@@ -286,6 +286,12 @@ private function loadModuleClassesDependencies(PackageInterface $package): array | |||
$installer = $this->composer->getInstallationManager(); | |||
$packagePath = $installer->getInstallPath($package); | |||
|
|||
/** @psalm-suppress TypeDoesNotContainNull Package path become nullable in composer 2.5.6 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a composer.lock
update suffice here, or are we stuck on an older release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: if we can use a newer release, then this requires no test, as it's just a type error, and we can document $this->mapAutoloaders()
to only accept non-empty-string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock update will suffice but it also brings other issues that would need to be resolved in a separate PR, at that point this becomes unused suppression and can be removed.
On the other hand if I update lock file with only composer/composer only this would work.
lock file maintenance PR did pick up type change:
Error: src/ComponentInstaller.php:289:71: PossiblyNullArgument: Argument 3 of Laminas\ComponentInstaller\ComponentInstaller::mapAutoloaders cannot be null, possibly null value provided (see https://psalm.dev/078)
Signed-off-by: Aleksei Khudiakov <aleksey@xerkus.pro>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Xerkus assuming that removal of your source changes led to psalm picking up the error, and addition of the changes makes the psalm error go away, this is good to 🚢
Description
Composer metapackages are not installed same way as other packages. With no install location there is nothing to discover even if metapackage tries to define autoloading rules for whatever reason. No change in effective behavior is introduced.
Starting with composer 2.5.6 the return type for
Composer\Installer\InstallationManager::getInstallPath()
method becomes nullable. This change fixes type error produced for metapackages with defined extras.Fixes #69