Introduce ComposerRequireChecker

[Fahl-Design]

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	yes

Description

Add maglnet/ComposerRequireChecker check when "code_checks" are required

• add new "Check" to src/create-jobs.js
• edit readme

[weierophinney]

I don't think we can automatically add this based on just having a composer.json present, and if the tool is not installed, the check will fail in that case.

This should act like the other checks, and only run if the configuration file for the composer require checker is present, as there's a reasonable assumption at that point that it's a package requirement of some sort.

[Fahl-Design]

Hi @weierophinney thanks the review

I don't think we can automatically add this based on just having a composer.json present, and if the tool is not installed, the check will fail in that case.

This should act like the other checks, and only run if the configuration file for the composer require checker is present, as there's a reasonable assumption at that point that it's a package requirement of some sort.

I was thinking about a global install of maglnet/ComposerRequireChecker because there is no need to require it on a project base and it works without a config file
BUT I get your point and now I think it's better on a project base to it can be added step by step to every package

[boesing]

I do feel uncomfortable on this as well.
What I have in mind would probably be a simple config flag (which is disabled by default) which can be switched on and tells the container to check the projects requirements.

This could be done by adding the composer-require-checker via phive or a global composer requirement to the container (as it is done for staabm/annotate-pull-request-from-checkstyle.

WDYT?

[Ocramius]

which is disabled by default

Having type: "library" is probably a sufficient decision point around checking for BC.

[boesing]

which is disabled by default

Having type: "library" is probably a sufficient decision point around checking for BC.

You are referring to the composer.json type, right?
The problem here is, that we actually have plenty of projects which would fail the composer-require-checker check.
Lets take laminas-validator for example, which should require laminas-db.
What I don't want is to have failing CI in all those components just because we enable this check by-default.

Another point for doing this opt-in would be that there is actually no such "default configuration file name" behavior as we have for psalm, phpunit, infection, e.g.

So you have to call composer-require-checker with --config-file if you want to provide a configuration. So there is no such composer-require-checker.json "default" which could be used and thus, you might end up checking with CRC without taking a specific config into account. If we do this via opt-in, using something like the following .laminas-ci.json would be both enable the check while fixing the config issue:

{
    "composer_require_checker": {
        "config": "path/to/config.json"
    }
}

[boesing]

So don't get me wrong, I am neither against this feature nor against the fact that laminas-validator should either require laminas-db or the other way around while moving the validator to laminas-db.

But as laminas components did not work like this in the past, enabling this by-default would be a huge no-go for me. I like the recent implementation tho (which checks for the presence of a config file) - I've just a few nitpicks.

[boesing]

Just for future readings

Even tho, this is an opinionated implementation (because we enforce projects to create/rename a missing/existing composer-require-checker.json), this ensures opt-in for the composer-require-checker feature.