Skip to content
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

Add support for PHP 8.1 #44

Merged
merged 23 commits into from
Dec 2, 2021
Merged

Add support for PHP 8.1 #44

merged 23 commits into from
Dec 2, 2021

Conversation

arueckauer
Copy link
Member

Q A
QA yes

Description

PHP 8.1 support.

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

@boesing
Copy link
Member

boesing commented Sep 30, 2021

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

{
    "ignore_php_platform_requirements": {
        "8.1": true
    }
}

Signed-off-by: Andi Rückauer <[email protected]>
src/Collection.php Outdated Show resolved Hide resolved
composer.lock Outdated Show resolved Hide resolved
Signed-off-by: Andi Rückauer <[email protected]>
…h PHP 8.1

Upgrade composer/composer to 2.0.14 to add compatibility with new key format of GitHub tokens

Signed-off-by: Andi Rückauer <[email protected]>
@arueckauer
Copy link
Member Author

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
Copy link
Member

boesing commented Oct 5, 2021

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
Copy link
Member

boesing commented Oct 5, 2021

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).

test/ConfigDiscoveryTest.php Outdated Show resolved Hide resolved
gsteel added a commit to netglue/laminas-messenger that referenced this pull request Oct 7, 2021
composer.json Outdated Show resolved Hide resolved
*/
public function __construct($projectDirectory = '')
public function __construct(string $projectDirectory = '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduce compatibility break, because changes public interface

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point.

IIRC I discussed this with @boesing in chat. The @param type hint states parameter is of type string. If implementation is breaking, it is due to a misuse.

Is that correct, @boesing?

Copy link
Member

@boesing boesing Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, __construct has some special behavior if it comes to arguments. So this is actually no BC break until the type-hint matches what we do state in the @var annotation.

(sidenote: imho laminas-component-installer is not a library and thus, semver does not really make sense here at all. But actually, we do not differ between libraries and components like this (composer plugin))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun-fact: this constructor argument is only added for unit tests so we can pass virtual filesystem directory

arueckauer and others added 6 commits November 29, 2021 15:03
…tives for Psalm unnecessary

Signed-off-by: Andi Rückauer <[email protected]>
- Bumps minimum supported PHP version to 7.4
- Updates to laminas-coding-standard 2.3 series
- Bumps minimum supported PHPUnit version to 9.5.5
- Updates how package replaces zend-component-installer by converting "replace" rule to a "conflict" rule against all versions, and removing the laminas-zendframework-bridge.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
…ts deprecations to exceptions

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
- Import the `ReturnTypeWillChange` attribute (`@use` annotation does not work for engine purposes)
- All dependencies are now installable under PHP 8.1, so no need to ignore platform requirements 🎉

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
The majority of these are due to third-party packages not providing specific enough of return type hints, leading to "mixed assignment" and similar.
Others are due to usage of things like filter/reduce/walk whereby we are operating on a subset of all arguments, but the last one is required for the operation (unused closure argument).

I did refactor the PackageProviderDetectionFactory to be more clear about what it provides, and to ensure it never passes a null argument.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney merged commit 33c3579 into laminas:2.6.x Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants