Invalid "installed_paths" config value

Wirone commented 7 years ago

Problem/Motivation

Relates to v0.4.0

PHPCodeSniffer's installed_paths config value is set incorrectly.

Two problems:

frenck/php-compatibility is duplicated: once it's absolute, once it's relative path
Paths contain directories different than phpcodesniffer-standard type of package (../../doctrine)

Expected behaviour

Two problems:

Paths should contain unique values
Paths should contain only packages with phpcodesniffer-standard type

Actual behaviour

-> % composer run-script install-codesniffs
> Dealerdirect\Composer\Plugin\Installers\PHPCodeSniffer\Plugin::run
PHP CodeSniffer Config installed_paths set to <path_to_project>/vendor/frenck/php-compatibility,../../doctrine/,../../frenck/php-compatibility/

Steps to reproduce

Require doctrine/doctrine-cache-bundle in Composer.

Proposed changes

Get rid of $searchPaths = [getcwd()];, should fix both issues.

frenck commented 7 years ago

@Wirone Thank you for opening this issue.

I will start with the second issue "Paths should contain only packages with phpcodesniffer-standard type". In general, I agree on this. Nevertheless, we also support coding standards in the root package itself (See #19 and #20). That's why the 'getcwd()' was added.

The first problem "Paths should contain unique values" might be a result of this... I'm not sure, I will test this and get back to you.

The first issue might also be caused by an upgrade of the plugin. Versions prior to v0.4.0 used absolute paths, since v0.4.0 relative paths are used on project installations and absolute paths on global installations. (See #14). An upgrade "might" cause ending up with the duplicates in your PHP_CodeSniffer configuration.

Wirone commented 7 years ago

@frenck I think you did not understand correctly, or I don't understand "root package" purpose - nevermind, I'll try to explain more.

I have Symfony app where I have frenck/php-compatibility and doctrine/doctrine-cache-bundle as dependencies. When Plugin::run() is called frenck/php-compatibility is found as the only package with phpcodesniffer-standard, but since root of my app is added to search paths, doctrine/doctrine-cache-bundle also is matched because it has ruleset.xml. This is wrong in my opinion, even though it's PHPCodeSniffer ruleset. However the saved path is ../../doctrine which does not make sense because this is directory with multiple Doctrine packages.

PS. I removed installed_paths from CodeSniffer.conf and after running plugin there is no duplicates. So only second problem is relevant (but after upgrading everyone will get duplicates so it could be fixed too by checking realpaths).

frenck commented 7 years ago

@Wirone The root package support is for "main" repositories that contain coding standards themself instead of a separate package. (Wordpress is having a specific use case as well, see WordPress-Coding-Standards/WordPress-Coding-Standards#855).

I do see an issue and I will try to explain why this is a bug... but actually not a bug. (That's me saying: We could improve here, but that not the root cause of the problem).

The actual problem is the doctrine/doctrine-cache-bundle. It contains an ruleset.xml in the root of the package. This is kinda weird, since the repository is not a coding standard, it is a library. I guess they tried to override some settings in the coding standard they DO use (instaclick/coding-standard) in their ruleset.xml. They should have used a file called either phpcs.xml or phpcs.xml.dist, as documented by PHP_CodeSniffer. This plugin does not pickup on these latter files.

How we could improve this plugin:

Get a list of packages installed that do NOT match the phpcodesniffer-standard type AND is NOT the root package.
Add the actual Composer installation paths of these packages and exclude these from being picked up.

To recap: We could improve a little, the actual root cause is with doctrine/doctrine-cache-bundle.

Potherca commented 7 years ago

after upgrading everyone will get duplicates

Currently v0.3 is getting as much downloads as v0.4, so this could still be an issue for some users. Although this could cause some inconvenience, I'm not sure we want to spend the energy to resolve that...

As for the "ignore ruleset in package root of non-codesniff packages" bug, I feel that should be resolved.

The most straightforward approach, as far as I can see, is to add an exclude() to the $finder for the directories of all non-codesniff packages. Alternatively 2 searches could be done, once in the root package (ignoring vendor entirely) and once only for the specified codesniff packages.

I am inclined to view this as a bug... (apparently the scope of the finder is not limited enough).

dingo-d commented 7 years ago

Any news about solving this? I think that it's also happening to me. I added in my project's composer.json

{
  "require-dev": {
    "dealerdirect/phpcodesniffer-composer-installer": "*",
    "jakub-onderka/php-console-highlighter": "*",
    "jakub-onderka/php-parallel-lint": "*",
    "infinum/coding-standards-wp": "*"
  }
}

Which results in

Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 7 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.1.1): Loading from cache
  - Installing dealerdirect/phpcodesniffer-composer-installer (v0.4.3): Downloading (100%)
  - Installing jakub-onderka/php-console-color (0.1): Loading from cache
  - Installing jakub-onderka/php-console-highlighter (v0.3.2): Loading from cache
  - Installing jakub-onderka/php-parallel-lint (v0.9.2): Loading from cache
  - Installing wp-coding-standards/wpcs (0.13.0): Downloading (100%)
  - Installing infinum/coding-standards-wp (0.2.5): Downloading (100%)
dealerdirect/phpcodesniffer-composer-installer suggests installing dealerdirect/qa-tools (All the PHP QA tools you'll need)
Writing lock file
Generating autoload files
PHP CodeSniffer Config installed_paths set to ../../../wp-content/plugins/,../../infinum/,../../wp-coding-standards/wpcs/,../../infinum/coding-standards-wp/

Infinum's coding standards composer can be seen here.

Now when I run ./vendor/bin/phpcs -i in the terminal I get

The installed coding standards are PEAR, Zend, PSR2, MySource, Squiz, PSR1, acf-content-analysis-for-yoast-seo, coding-standards-wp, WordPress-VIP, WordPress, WordPress-Extra, WordPress-Docs, WordPress-Core and coding-standards-wp

which is not right. I should get the default standards from PHPCS, WPCS and Infinum standard.

Is there a fix for this, or are my post install and update scripts maybe causing this in the Infinum's coding standards?

frenck commented 6 years ago

This issue is actually not an issue. All cases are not a match for the plugin to work with. Since there is nothing to do, I'll go ahead and close this one.

Wirone commented 2 years ago

@jrfnl You've unlocked the Necromancer badge! 😅

Potherca commented 2 years ago

PHPCSStandards / composer-installer