Composer PHP version appears not to be respected

jrfnl commented 5 years ago

Problem/Motivation

Ok, so this is a fairly complicated one to explain, so please bear with me.

First off, I'm not sure if this is a Windows specific problem or a wider problem, but I have been able to consistently reproduce the issue on Windows.

> "vendor/bin/phpcs"

By default, if you call a script in the vendor/bin directory, the system default PHP version will be used. Important: This may not be the same PHP version as is used by Composer!

If you want commands to use the same PHP version as Composer uses, you need to define them as scripts and prefix them with @php. See: https://getcomposer.org/doc/articles/scripts.md#executing-php-scripts

  "scripts": {
    "check-cs": "@php \"vendor/bin/phpcs\""
  }

For this to work on Windows with PHPCS (though probably the same goes for other tools), you can't point to the script in the vendor/bin directory as Windows will just print out that script, so you need to point to the actual script in the dependency. See: https://github.com/composer/composer/issues/7645

  "scripts": {
    "check-cs": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs"
  }

Now, given a system default PHP version of 5.4.x and a PHP used by Composer of 7.2.x and having PHPUnit as a project dependency, the DealerDirect PHPCS plugin will appear to work correctly, but in reality will fail to work and the CodeSniffer.conf file is not created or updated.

The PHPUnit dependency is arbitrary, that is just the dependency through which I discovered the bug. The point is to have a dependency which has non-cross-version compatible PHP code and which will autoload at least one file from the dependency.

I've tested this both with v 0.4.4 as well as 0.5.0. In 0.5.0, this will fail silently. In 0.4.4, this will fail with error info:

Writing lock file
Generating autoload files
PHP CodeSniffer Config installed_paths set to ../../phpcompatibility/php-compatibility/,../../wp-cod
ing-standards/wpcs/

  [Symfony\Component\Process\Exception\ProcessFailedException]
  The command ""path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/bin/phpcs" "--
  config-set" "installed_paths" "../../phpcompatibility/php-compatibility/,../../wp-coding-standa
  rds/wpcs/"" failed.

  Exit Code: 255(Unknown error)

  Working directory: path/to/Dealerdirect-PHPUnit-Conflict-POC

  Output:
  ================
  Parse error: syntax error, unexpected 'function' (T_FUNCTION), expecting identifier (T_STRING)
  or \\ (T_NS_SEPARATOR) in path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/my
  clabs/deep-copy/src/DeepCopy/deep_copy.php on line 5

  Call Stack:
      0.0010     129408   1. {main}() path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/squizlabs/php_codesniffer/bin/phpcs:0
      0.0040     172544   2. PHP_CodeSniffer\Autoload::load() path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/squizlabs/php_codesniffer/bin/phpcs:17
      0.0050     174312   3. include('path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/autoload.php') path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/squizlabs/php_codesniffer/autoload.php:79
      0.0070     186384   4. ComposerAutoloaderInit6a82c0ae3a6b11bce995085b1e490fcd::getLoader()    path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/autoload.php:7
      0.0170     389304   5. composerRequire6a82c0ae3a6b11bce995085b1e490fcd() path/to/Dealerdirect-PHPUnit-Conflict-POC/vendor/composer/autoload_real.php:56

  Error Output:
  ================

install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [-
-no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-a
utoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<package
s>]...

Based on the 0.4.4. output, it seems that the system default PHP version is used as some point - not the PHP version used by Composer - as this is a PHP 5.4 parse error for PHP 7 code.

While I have to say that it is bewildering to me that the referenced - completely unrelated - file would be loaded at this point in the first place (I'd guess that's a misconfigured autoload section in that dependency), it highlights that the default PHP version is used to set the installed_paths instead of the Composer version.

IMO, a Composer plugin should always respect the PHP version on which Composer is being run, which is why I'm reporting this here as a bug.

System info:

Windows 7
Composer 1.8.0
Tested with version 0.4.4 and 0.5.0 of this plugin.

Expected behaviour

That the PHPCS installed_path is set properly.

Actual behaviour

The plugin echo's to the screen that the installed_path has been set, but in reality it hasn't been.

Steps to reproduce

I've set up a minimal Proof of Conflict repo here: https://github.com/jrfnl/Dealerdirect-PHPUnit-Conflict-POC

To reproduce, you will need a Windows machine, though it may fail other OS-es as well.

Set the default PHP version to PHP 5.4.x (i.e. set the various PATHs in Windows to point to a PHP 5.4.x install.)
Edit the composer.bat file to point PHP to a more recent version.
Clone the repo and check out the master branch.
Run composer install and then run phpcs-i. All should be fine and it will display The installed coding standards are MySource, PEAR, PSR1, PSR12, PSR2, Squiz, Zend, PHPCompatibility, WordPress, WordPress-Core, WordPress-Docs and WordPress-Extra.
Throw away composer.lock and the vendor directory.
Check out the POC branch.
Run composer install and then run phpcs-i. Now it will display The installed coding standards are MySource, PEAR, PSR1, PSR12, PSR2, Squiz and Zend.

Proposed changes

I haven't dug into the plugin code yet to see what would need to be changed, but I'm guessing that if there are exec() commands and such, that will be where problem lies.

jrfnl commented 5 years ago

I've done some digging leading to PR #80 which would solve this, but may not be the best way to go about things. At the very least it should give some idea of solution directions.

Potherca commented 5 years ago

Thank you for reporting this and taking the time and effort to provide a thorough reproduction scenario!

I'll try to reproduce things as soon as I have access to a windows environment, that would also give me opportunity to verify #80.

exussum12 commented 5 years ago

To confirm this is all envs. Have the exact same issue on linux and mac.

jrfnl commented 5 years ago

I just ran into this issue again myself. Would be awesome if the PR could get some attention as it really is pretty annoying/confusing when this happens.

exussum12 commented 5 years ago

Huge +1 from me too. When it fails there is hardly any indication of the error.

jrfnl commented 4 years ago

FYI: I've added another commit/branch to the POC repo test-the-fix to allow to easily test the fix as pulled in PR #80.

PHPCSStandards / composer-installer