PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
549 stars 36 forks source link

Config codings styles in composer.json from project #23

Closed fadoe closed 7 years ago

fadoe commented 7 years ago

Problem/Motivation

Your plugin only works, when the type in the composer.json from the codings standard is "phpcodesniffer-standard". The problem: this is not a requirement and not a standard.

Expected behaviour

I can install any coding standard that use the Codesniffer-Standard. There must not be a type "phpcodesniffer-standard" in composer.json from the coding standard. E.g. "move-elevator/symfony-coding-standard"

Actual behaviour

Only packages with the package type "phpcodesniffer-standard" working with this plugin. E.g. "frenck/php-compatibility".

Steps to reproduce

Require "frenck/php-compatibility" in composer.json as coding standard and it works. Require "move-elevator/symfony-coding-standard" in composer.json as coding standard and it doesn't work.

Proposed changes

Add a key "phpcodesniffer-standards" in the composer.json from the project and add all packages that are a coding standard:

    "require-dev": {
        "dealerdirect/phpcodesniffer-composer-installer": "^0.3.2",
        "frenck/php-compatibility": "^7.1",
        "move-elevator/symfony-coding-standard": "^2.0"
    },
    "extra": {
        "phpcodesniffer-standards": [
            "move-elevator/symfony-coding-standard"
        ]
    }

In Plugin::getPHPCodingStandardPackages:

        $extra = $this->composer->getPackage()->getExtra()["phpcodesniffer-standards"];

       // check if $extra is set

        $codingStandardPackages = array_filter(
            $this->composer->getRepositoryManager()->getLocalRepository()->getPackages(),
            function (PackageInterface $package) use ($extra) {
                if ($package instanceof AliasPackage) {
                    return false;
                }

                if (in_array($package->getName(), $extra)) {
                    return true;
                }

                return $package->getType() === Plugin::PACKAGE_TYPE;
            }
        );

Output from phpcs -i:

PHP CodeSniffer Config installed_paths set to /home/falk/Projekte/phpcodesniffer-composer-project/vendor/frenck/php-compatibility,/home/falk/Projekte/phpcodesniffer-composer-project/vendor/move-elevator/symfony-coding-standard/Standards
Potherca commented 7 years ago

Although we appreciate the effort that has gone into reporting this, we do not intend to support this feature.

The main reason are:

  1. In it's first incarnation, this package used to have this functionality. That didn't work too well in various scenarios (including meta-packages) and with some edge-cases. If you would like to know more details on this topic, I can ask @frenck to expand on it.
  2. This package is explicitly designed to work "as-is" as an alternative to other implementations that either do weird things installing sniffs or require actions from users. We intend to honor this design. There is only one requirement for it to work "out of the box" and that is the phpcodesniffer-standard declaration in the composer.json
  3. Adding this feature would introduce more complexity here. More logic would need to be added in other places (for instance in the upcoming feature to support relative paths). This would make this package harder to maintain for (from our perspective) very little gain. Requiring other packages to declare themselves as phpcodesniffer-standard adds zero code, no complexity and no extra maintenance overhead.

As an alternative to adding this feature we would suggest opening a pull-request in the mentioned repository, explaining why declaring phpcodesniffer-standard in the composer.json and using this package would be beneficial to them (and their users).

A package can declare phpcodesniffer-standard without requiring this installer and nothing changes at all. The package will be installed just as it always has, as a composer "library" package. We do not see any reason for maintainers not to honor such a PR.

We've set up a standard template that can be used for opening PR's explaining the workings and/or the benefit.[1]

Again, we really do appreciate the time and effort that has gone in to reporting this be we are declining to change the way the package works.

[1]: Feedback is welcome.