Update laminas-coding-standard to v2

[Slamdunk]

No description provided.

[Slamdunk]

PHP Fatal error:  Uncaught TypeError: Argument 5 passed to SlevomatCodingStandard\Helpers\Annotation\ParameterAnnotation::__construct() must be an instance of PHPStan\PhpDocParser\Ast\PhpDoc\ParamTagValueNode or null, instance of PHPStan\PhpDocParser\Ast\PhpDoc\TypelessParamTagValueNode given, called in /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/AnnotationHelper.php on line 357 and defined in /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/Annotation/ParameterAnnotation.php:31

😭

[Ocramius]

Yup, you can force phpstan/phpdoc-parser: <1.6 to make it run, meanwhile (do it locally, run a composer update, then remove the clause and use composer update nothing)

[Slamdunk]

Wouldn't CI fail nevertheless, if I don't commit the phpdoc-parser: <1.6 requirement?

[Ocramius]

Only in the next lockfile update PR

[Slamdunk]

As I expected, build can't get green if I don't commit "phpstan/phpdoc-parser": "~1.5.0":
https://github.com/laminas/laminas-mail/runs/7627796218?check_suite_focus=true#step:3:416

I can commit it though, can't I?

[Ocramius]

The idea is to commit a composer.lock with an older phpstan/phpdoc-parser, by leaving composer.json intact.
CS checks run on locked dependencies only.

[Slamdunk]

Oh, now I get it.

But I don't see a laminas guide on how to produce the composer.lock consistently: is a composer update on lowest supported PHP binary enough?

[Ocramius]

There's no guide for that, afaik: I proposed an approach in #202 (comment)

1. add the dependency restriction to composer.json
2. run composer update
3. remove the dependency restriction from composer.json
4. run composer update nothing

this gives you a composer.lock in the state you need it.

[Slamdunk]

Thank you, I was missing the last step.

Now, down to the PR: LCSv2 introduces declare(strict_types=1); everywhere, should this change be considered a BC Break?

[Ocramius]

It really depends - declare(strict_types=1) is safe to perform in some cases, while in others, it can lead to massive breakages.

I wouldn't say that the laminas/laminas-mail code is very "safe", at least when looking at its large psalm baseline:

laminas-mail/psalm-baseline.xml

Lines 3904 to 3908 in 2ec0498

	<RedundantCondition occurrences="1">
	<code>assertNotNull</code>
	</RedundantCondition>
	</file>
	</files>

I think we need to disable that rule here. See how it's done in laminas/laminas-eventmanager:

https://github.com/laminas/laminas-eventmanager/blob/6c056ab3b4b464daa308fd9ca3e7416ca40cf200/phpcs.xml#L23-L25

[Slamdunk]

I've carefully fixed all the PHPCS issues.

I've also fixed most of the notable PSalm issues; the rest that I've baselined are just too tedious to be done manually now in PHPDocs.

Similarly to what I've done for laminas-form:v3.0.0 I'll be happy to clear all PSalm baseline once we can ship native types though.

[Ocramius]

The template parameter on AbstractPluginManager<InstanceType> should probably be filled:

https://github.com/laminas/laminas-servicemanager/blob/6cee2f3125e074c0960c13a9ffc1ed93127b860e/src/AbstractPluginManager.php#L35

[Slamdunk]

I've looked at laminas/laminas-validator#129 and commit d20feb2 is the result

[Slamdunk]

Error: src/Protocol/SmtpPluginManager.php:26:23: InvalidTypeImport: Type alias FactoriesConfigurationType imported from Laminas\ServiceManager\ConfigInterface

Because I've checked commit validity against latest deps, but PSalm in CI runs on lowest which misses all the latest types-related releases.

Should I revert this, or keep and baseline it?

[Ocramius]

but PSalm in CI runs on lowest which misses all the latest types-related releases.

It should run with locked dependencies.

This seems to occur because we're still on PHP 7.3, and the latest type improvements are on PHP 7.4.

Let's touch this in a follow-up that drops PHP 7.4 👍

[Ocramius]

Ah, I see that you did add the templated types. In order for that to work, we need to drop PHP 7.3, and then a composer update should bring us on par with the servicemanager improvements, as well as make the build green again 👍

[Ocramius]

LGTM.

What's still missing:

• drop PHP 7.4
• re-generate baseline with the new dependency upgrades (it should get rid of the templated types issues)

[Slamdunk]

What's still missing

Since you approved this PR, I guess I can merge this as-is (with failing build) and then open a follow up to drop PHP < 8.0.

Merging red builds isn't good, but neither is discussing such big changes in the same PR.
At least that's what I'd do in my repos.

Confirm please, just so that I can learn your preferred way of managing such situations.

[Ocramius]

No, please make it green first, be it through adjusted baseline or else, but let's not merge on red, as that just blocks other work, and will also lead to raised PRs by Renovate bot overnight :|