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

Support PHP CodeSniffer standards in packages installed outside of the vendor directory #63

Closed
echernyavskiy opened this issue Oct 23, 2018 · 7 comments
Assignees

Comments

@echernyavskiy
Copy link

echernyavskiy commented Oct 23, 2018

Problem/Motivation

The aim of this is to add proper support for PHP CodeSniffer standards in packages installed outside of the vendor directory. One such example is Drupal's Coder module which is most commonly installed into web/sites/all/modules/contributed/coder or web/modules/contributed/coder where the web directory is on the same level as vendor. The issue is that for such packages composer's getInstallPath() returns an install path relative to the project root directory, and such relative paths are not treated properly later in the body of this plugin's updateInstalledPaths().

Expected behaviour

The coding standard's install path is handled properly by this plugin's updateInstalledPaths() so that the coding standard is properly detected.

Actual behaviour

The coding standard's install path is processed in such a way that the value added to PHPCS's installed_paths is a non-existent directory.

Steps to reproduce

Require both dealerdirect/phpcodesniffer-composer-installer and drupal/coder in composer.json, run composer install. drupal/coder will be installed into the configured Drupal modules directory and will not be properly detected by this plugin.

Proposed changes

The suggestion is to ensure all coding standard install paths are absolute before searching for available rule-sets in updateInstalledPaths(). See #64.

@frenck
Copy link
Contributor

frenck commented Oct 25, 2018

Having Coding Standards in your main project is already supported. The only limitation is the maximum search depth, which is going to be configurable when #46 gets merged in.

Am I missing something?

@echernyavskiy
Copy link
Author

I'll try to be a bit more specific here. So given the following pretty typical Drupal project structure:

...
vendor
...
web
  - modules
    - contributed
      - coder
        - coder_sniffer
          - Drupal
            - ruleset.xml
...
composer.json
composer.lock

PHPCodeSniffer composer installer rightfully discovers this standard among others:

array(9) {
  [0]=>
  string(52) "/var/www/project"
  [1]=>
  string(29) "web/modules/contributed/coder"
  [2]=>
  string(98) "/var/www/project/vendor/escapestudios/symfony2-coding-standard"
  ...
  [8]=>
  string(84) "/var/www/project/vendor/slevomat/coding-standard"
}

But later on in updateInstalledPaths() it assumes that the path is absolute and attempts to get a relative path for an already relative one, which results in configuring PHPCodeSniffer with a non-existent standard install path:

string(58) "web/modules/contributed/coder/coder_sniffer/DrupalPractice"
string(77) "../../../../../../../../../../../web/modules/contributed/coder/coder_sniffer/"

@frenck
Copy link
Contributor

frenck commented Oct 26, 2018

I get it, nevertheless, I'm unable to reproduce.

I have set up a full Drupal project to test this out and this is what happens on my machine (note I've added a var_dump of the found $installedPaths array to the output for debugging.)

$ composer require drupal/coder                                                                                                                      
Using version ^8.3 for drupal/coder
./composer.json has been updated
    1/3:	http://repo.packagist.org/p/provider-2018-10$3f3940f9e55c8044a96080611abc07a61ac8963d9525ca5aa8adab8c837af3ec.json
    2/3:	http://repo.packagist.org/p/provider-latest$2d59e62583baac197336eab2a88ee9c54911e3def04e263e9f73fb3fef7553d1.json
    3/3:	http://repo.packagist.org/p/provider-2017$c78961d9a33367c72796bdbcb31d5d64c91da7d524f370a41582e2459b40d30b.json
    Finished: success: 3, skipped: 0, failure: 0, total: 3
> DrupalProject\composer\ScriptHandler::checkComposerVersion
Loading composer repositories with package information
Updating dependencies (including require-dev)
Nothing to install or update
Writing lock file
Generating autoload files
array(2) {
  [0]=>
  string(44) "/Volumes/CODE/dealerdirect/codesniffer-test3"
  [1]=>
  string(64) "/Volumes/CODE/dealerdirect/codesniffer-test3/vendor/drupal/coder"
}
PHP CodeSniffer Config installed_paths set to ../../drupal/coder/coder_sniffer/

Used composer.json for this test: https://gist.github.com/frenck/a03bd5695636d1105aea1895aea774e0

Please note, I've used the latest dev version of the installer in the above example, but I've got the same results with the v0.4.4 release.

@frenck
Copy link
Contributor

frenck commented Oct 26, 2018

@echernyavskiy I've ended up implementing your solution (slightly different / more incorporated with the plugin, since Filesystem is now used more widely by the Plugin).

Since I lack a reproduction scenario, I cannot ensure this fixes your problem.

@frenck
Copy link
Contributor

frenck commented Oct 26, 2018

@echernyavskiy It is in the v0.5.0 release. So if you could test it and let me know? That would be awesome 👍

@echernyavskiy
Copy link
Author

@frenck, yup, tested and it works in my setup as well, thanks.

@frenck
Copy link
Contributor

frenck commented Oct 27, 2018

Nice @echernyavskiy! Thanks for letting me know!

I'll go ahead and close up this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants