Plugin not working correctly when sniffs install depth is equal to "1"

bastianschwarz commented 7 years ago

Problem/Motivation

Since I use the "*" constraint in our projects, the package updated to 0.3.0 today. Right now, the default PHPCompatiblity sniff can not be found. I have not yet looked into it myself in the hope that you might know right away what the issue is.

Expected behaviour

PHPComptability sniff should still be installed

$ ./vendor/bin/phpcs -i
The installed coding standards are Zend, PHPCS, MySource, PSR2, PEAR, PSR1, Squiz and PHPCompatibility

Actual behaviour

Sniff is not installed:

$ vendor/bin/phpcs -i
The installed coding standards are Zend, PHPCS, MySource, PSR2, PEAR, PSR1 and Squiz

Steps to reproduce

composer.json:

  "require-dev": {
    "squizlabs/php_codesniffer": "^2",
    "dealerdirect/phpcodesniffer-composer-installer": "*",
    "frenck/php-compatibility": "*"
  }

Delete vendor folder (if 0.2.1 which still works was installed, the sniff is still there)

$ composer update
$ vendor/bin/phpcs 
$ ./vendor/bin/phpcs --standard=PHPCompatibility --extensions=php ./src
$ ./vendor/bin/phpcs -i

Proposed changes

Is there a migration I missed or needs frenck/PHPCompatibility a new version too?

Thanks in advance for any help.

frenck commented 7 years ago

@bastianschwarz Thank you for this bug report.

There are no migrations needed, so nothing wrong on your end.

I will run test against the latest version asap and report back.

frenck commented 7 years ago

I was not successful in reproducing.

I've created an empty directory and place the following composer.json:

{
    "name": "frenck/test",
    "require": {
        "squizlabs/php_codesniffer": "^2",
        "dealerdirect/phpcodesniffer-composer-installer": "0.2.1",
        "frenck/php-compatibility": "^7.1"
    }
}

Ran composer install, and it worked as expected. Next I've modified the version of this plugin in the composer.json to * and ran composer update.

$ ./vendor/bin/phpcs -i
The installed coding standards are MySource, PEAR, PHPCS, PSR1, PSR2, Squiz, Zend and PHPCompatibility

$ ./vendor/bin/phpcs --config-show
installed_paths: /Users/frenck/code/test/vendor/frenck/php-compatibility

I've also tried a version with a fresh install using the latest version directly, this also worked fine.

Some extra questions:

Did you update anything else? Like the PHP_CodeSniffer package?
Are having this issue in a public project I could take a look at?
Is this install global of localized in a project?

bastianschwarz commented 7 years ago

No, the PHP_CodeSniffer version was not updated and it's a local install, not a global one.

I'm at home now and have to wait for my food, so I'll see if I can repoduce the issue with any of my own projects which use the same setup.

bastianschwarz commented 7 years ago

Just reproduced it with one of my projects: https://github.com/codenamephp/platform.di

There is an ant build.xml in the project root that executes the compatiblity check, basically the same call I did in the issue description.

$ ant phpcomp -> works
$ composer update; ant phpcomp -> updates to 0.3.0 -> still works (sniff still in place from the previous install)
$ rm -rf vendor; composer update; ant phpcomp -> installs 0.3.0 again -> Exception
revert to 0.2.* ->$ composer update -> installs 0.2.1 again -> still exception
$ rm -rf vendor; composer update -> installs 0.2.1 -> works

Seems like there is something wrong when the components are already installed and the versions are switched?

frenck commented 7 years ago

I will check your project locally.

Note: You're talking about exceptions. It would be really useful if you'd include them in this issue report.

bastianschwarz commented 7 years ago

Sure, it's just the default "Sniff not found"

phpcomp:
  [phpcomp] PHP Fatal error:  Uncaught PHP_CodeSniffer_Exception: Referenced sniff "PHPCompatibility" does not exist in /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1167
  [phpcomp] Stack trace:
  [phpcomp] #0 /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/CodeSniffer.php(780): PHP_CodeSniffer->_expandRulesetReference(Object(SimpleXMLElement), '/home/bastian/w...', 0)
  [phpcomp] #1 /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/CodeSniffer.php(578): PHP_CodeSniffer->processRuleset('/home/bastian/w...')
  [phpcomp] #2 /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(956): PHP_CodeSniffer->initStandard(Array, Array, Array)
  [phpcomp] #3 /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(113): PHP_CodeSniffer_CLI->process()
  [phpcomp] #4 /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/scripts/phpcs(25): PHP_CodeSniffer_CLI->runphpcs()
  [phpcomp] #5 {main}
  [phpcomp]   thrown in /home/bastian/workspace/codenamephp/platform.di/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 1167
  [phpcomp] Result: 255

frenck commented 7 years ago

@bastianschwarz I was able to reproduce the issue with this repository, thank you. I've actually pinpointed the situation where it goes wrong, let me explain.

Running a composer update on your repository actually causes 31 packages to update and 1 new dependency is installed newly. Including:

dealerdirect/phpcodesniffer-composer-installer (v0.2.1 => v0.3.0)
squizlabs/php_codesniffer (2.7.0 => 2.8.0)
frenck/php-compatibility (7.0.8 => 7.1.1)

I tried from scratch again, this time upgrading the 3 packages above in the order listed, running ant phpcomp between each package update.

After upgrading dealerdirect/phpcodesniffer-composer-installer (v0.2.1 => v0.3.0) it actually worked as expected.

After upgrading squizlabs/php_codesniffer (2.7.0 => 2.8.0) it broke down the way you described.

Lastly (while still being broken) I upgraded frenck/php-compatibility (7.0.8 => 7.1.1) and now it actually worked again! So the issue occurs when PHP_CodeSniffer is upgraded.

Updated issue description:

The plugin is unable to handle upgrades on the PHP_CodeSniffer package. It seems like the plugin is not triggered?

Since PHP_CodeSniffer actually stores its settings inside her own vendor directory, these settings are lost on upgrade. This is bad design on the PHP_CodeSniffer side. Nevertheless, since this plugin is not acting properly on this upgrade either, the configuration is not fixed / recreated.

Workaround: You could easily work around this by triggering an extra composer update command, which will fix the configuration for you.

composer update
composer update nothing
ant phpcomp

bastianschwarz commented 7 years ago

Sadly, the workaround did not work for me, neither for the update nor a clean install so I had a look at the plugin.

What I found:

After a clean install, the installedPaths are empty. The "frenck/php-compatibility" package is found by getPHPCodingStandardPackages() but since the Finder in updateInstalledPaths() sets the depth to >1 (which is depth 2), but the ruleset.xml in the frenck/php-compatibility is in level 1 within the install path, the path is never added to the config.

Since I'm already kinda tired and I don't know if this is really just a "greater than vs greater than equals" issue or if I'm missing something in the big picture, I was kinda too lazy to fork, write a test and create a pull request. ;)

If I change the depth to >0, it worked for a clean install and for the update.

Maybe @christopher-hopper sees instantly if the >1 is acutally needed or if >0 is sufficent enough?

Wirone commented 7 years ago

@bastianschwarz same here, needed to change minimal depth in order to properly install PHPCompatibility. Half an hour lost.

Potherca commented 7 years ago

@bastianschwarz Thanks for the details, it is evening here, I will take a look at this issue in the morning. @Wirone I'm sorry for your lost of time and any frustrations that might have caused. @frenck I'll set up some test scenario's Friday to validate the various use-cases that the plugin is supposed to support. Maybe add those unit-tests we talked about.

I've you can all spare us a day or so, this issue will most likely be resolved.

christopher-hopper commented 7 years ago

I'm taking a look at the Finder depth method docs and testing some scenarios.

:+1: for some unit tests. I'll see if I can suggest a couple for this particular issue to prevent it ever reoccurring. I'll push that separately probably as it's part of the larger CI/Unit Testing push in #9

Apologise for not seeing this during development. Not sure how it fell through the cracks. Again, +1 for CI and tests.

Potherca commented 7 years ago

We've been debugging this issue and the Finder Depth does indeed seem to be the root of the issue. However, it turns out that several other things coincide, causing some confusing behaviour:

When this plugin is installed globally, composer will load the global plugin rather than the one from the local repository. Despite being documented behaviour, this caused trouble for @frenck when trying to reproduce this issue, as another version of the plugin was used than the one we thought we were looking at.
When working with an existing vendor directory, if composer update is run, composer loads the old plugin. To clarify, the following steps occur:
1. The composer update command is run
2. A composer object is created
3. Version 0.2.1 of the plugin (which functions correctly) is loaded
4. Composer updates all dependencies (Updating the plugin code to v0.3.0)
5. The v0.2.1 plugin is triggered.

Unless PHP Codesniffer is updated, custom sniffs will not be installed as they will still be triggered.

If PHP Codesniffer is updated, the custom sniffs will be removed. With the v0.2.1 version this is not a problem, as the sniffs will be installed again. With the v0.3.0 version (because of the finder-depth bug), custom sniffs are not installed.

The suggested solution is to change the >1 to >=1 (as @bastianschwarz suggested) and make a new release v0.3.1 as soon as possible.

The (for us) unexpected behaviour surrounding updates and global installs can then be documented and released as v0.3.2.

There might possibly also be an issue with the logic that validates whether or not PHP CodeSniffer is installed in relation with PHP Codesniffer updates. This will be researched as a separate issue.

frenck commented 7 years ago

I've just released version v0.3.1, which includes a hotfix for the search depth issue.

Please note, as @Potherca wrote already, composer will use the previous installed plugin version while upgrading. If you still wind up with a broken setup after upgrading the plugin, please run composer update nothing in order to fix your configuration.

@bastianschwarz Thank you for reporting this issue!

bastianschwarz commented 7 years ago

@frenck Just tested it in one of our projects, works! Thanks for the fix!

Wirone commented 7 years ago

We also upgraded to 0.3.1 and it's working. Thanks!

PHPCSStandards / composer-installer