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

Add support for PHP 8.1 #44

Closed arueckauer closed 2 years ago

arueckauer commented 2 years ago
Q A
QA yes

Description

PHP 8.1 support.

This also fixes known incompatibilites with PHP 8.1 described in #35 and #36.

boesing commented 2 years ago

To make composer install pass, you need to add .laminas-ci.json to the project root.

{
    "ignore_php_platform_requirements": {
        "8.1": true
    }
}
arueckauer commented 2 years ago

As version ^2.0 of composer/xdebug-handler is required, vimeo/psalm needs to be updated to ^4.8. Since from there to ^4.10 only one more error is thrown, I did the update to the latest version.

Would be happy to get some hints on the four argument errors. With the unreferenced param and @internal errors, I am not sure, how to fix it or if it could be added to the baseline.

boesing commented 2 years ago

Since the psalm errors are not related to the diff, there is no way of adding comments to those lines by using github.

I will list the issues below and add suggestions on how to fix these:

Argument 1 of Laminas\ComponentInstaller\ComponentInstaller::mapAutoloaders expects array<string, array<int|string, list|string>>, parent type array{classmap?: list, files?: list, psr-0?: array<string, array<array-key, string>|string>, psr-4?: array<string, array<array-key, string>|string>} provided (see https://psalm.dev/194)

Okay, to make psalm happy, we could ~import the psalm/phpstan type (which is used in the PackageInterface of composer)~ copy and paste the return type from composer (sadly they did not used a local type for that - might be worth changing that, not sure if thats something composer wants to implement) and then change the type for mapAutoloaders. https://github.com/composer/composer/blob/cb1e2482580beaf3e511278dd629ded599cddc1f/src/Composer/Package/PackageInterface.php#L300


Param $packages is never referenced in this method (see https://psalm.dev/188)

Param $type is never referenced in this method (see https://psalm.dev/188)

Param $key is never referenced in this method (see https://psalm.dev/188)

Param $injector is never referenced in this method (see https://psalm.dev/188)

Just change unused params to $_ - this should be fine for both the coding standard and psalm. any variable prefixed with _ is being ignored if it comes to unused parameters, so if there are more than one unused argument within a function, you can also add $_1, e.g.


Unable to determine the type that $allowedTypes is being assigned to (see https://psalm.dev/032)

This could be fixed by adding generics to the Collection implementation. https://psalm.dev/docs/annotating_code/templated_annotations/#param-class-stringt


Argument 1 of Composer\Repository\InstalledRepository::__construct expects array<array-key, Composer\Repository\RepositoryInterface>, array{Composer\Repository\RootPackageRepository|null, Composer\Repository\InstalledRepositoryInterface, Composer\Repository\PlatformRepository} provided (see https://psalm.dev/004)

So it seems that we somehow have an annotation which is null for the packageRepository property. Either remove that null and ensure that its definitely not set to null at any point (e.g. by using webmozart/assert) or use webmozart/assert before passing that argument to the InstalledRepository


Constructor PHPUnit\Framework\ExpectationFailedException::__construct is internal to PHPUnit but called from LaminasTest\ComponentInstaller (see https://psalm.dev/175)

Constructor PHPUnit\Framework\ExpectationFailedException::__construct is internal to PHPUnit but called from LaminasTest\ComponentInstaller (see https://psalm.dev/175)

Constructor PHPUnit\Framework\ExpectationFailedException::__construct is internal to PHPUnit but called from LaminasTest\ComponentInstaller (see https://psalm.dev/175)

Just change this to self::fail() rather than throwing an exception here.

boesing commented 2 years ago

I definitely want to deep-dive more into this as I did plenty of work to make both composer v1 and v2 compatible with this component. dropping v1 because of a minor PHP version should be avoided at any cost.

Could you probably re-add composer v1 as a dependency (and ensure that it will be installed when using composer update --prefer-lowest)? I'd love to see which tests are failing. If only PHP 8.1 tests are failing when using lowest deps, I probably would be fine with adding that test to exclude within .laminas-ci.json.

But maybe this component needs also a different CI pipeline so we have better coverage for the dedicated composer versions. I'll think about that somewhen in the next weeks.

Please ping me here or on slack if you think I might've forgotten this (which could probably be true).