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

Composer PHP version appears not to be respected #79

Closed
jrfnl opened this issue Jan 21, 2019 · 6 comments · Fixed by #80
Closed

Composer PHP version appears not to be respected #79

jrfnl opened this issue Jan 21, 2019 · 6 comments · Fixed by #80
Assignees

Comments

@jrfnl
Copy link
Member

jrfnl commented Jan 21, 2019

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: composer/composer#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
Copy link
Member Author

jrfnl commented Jan 21, 2019

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
Copy link
Member

Potherca commented May 7, 2019

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
Copy link

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

@jrfnl
Copy link
Member Author

jrfnl commented Aug 13, 2019

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
Copy link

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

@jrfnl
Copy link
Member Author

jrfnl commented Dec 9, 2019

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.

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

Successfully merging a pull request may close this issue.

3 participants