PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
559 stars 36 forks source link

Send an exit code when the script terminates #93

Closed jrfnl closed 4 years ago

jrfnl commented 4 years ago

Proposed Changes

This fixes the issue identified in https://github.com/Dealerdirect/phpcodesniffer-composer-installer/pull/80#issuecomment-531586417 where even when the setting of installed_paths failed, the plugin would exit with exit code 0.

A 0 exit code will be returned if successful, 1 - or the exit code returned by PHPCS itself - if not.

This will allow wrapper scripts and/or process scripts which take exit codes into account to fail correctly and/or notify users of failure.

Testing this change

  1. On this repo, throw away the vendor directory and composer.lock file.
  2. Make sure you are on the master branch.
  3. Run composer install.
  4. Now for the sake of testing this change, copy the vendor/squizlabs/php_codesniffer/CodeSniffer.conf.dist file to vendor/squizlabs/php_codesniffer/CodeSniffer.conf and make it read only.
  5. Now run composer install-codestandards --verbose. The output will look something like:

    Composer version 1.9.1 2019-11-01 17:20:17
    
    > install-codestandards: Dealerdirect\Composer\Plugin\Installers\PHPCodeSniffer\Plugin::run
    Running PHPCodeSniffer Composer Installer
    Failed to set PHP CodeSniffer installed_paths Config
    ERROR: Config file I:\path\to\phpcodesniffer-composer-installer\vendor\squizlabs\php_codesniffer\CodeSniffer.conf is not writable
  6. Run echo $? and take note of the exit code being 0 (= success).
  7. Now switch to this branch and run composer install-codestandards --verbose again.

    Composer version 1.9.1 2019-11-01 17:20:17
    
    > install-codestandards: Dealerdirect\Composer\Plugin\Installers\PHPCodeSniffer\Plugin::run
    Running PHPCodeSniffer Composer Installer
    Failed to set PHP CodeSniffer installed_paths Config
    ERROR: Config file I:\path\to\phpcodesniffer-composer-installer\vendor\squizlabs\php_codesniffer\CodeSniffer.conf is not writable
  8. Run echo $? and take note of the exit code now being 1 (= failure).

Another test which tests another part of this change:

  1. Follow steps 1 to 3 from above.
  2. Now for the sake of testing this change, delete the vendor/squizlabs/php_codesniffer directory.
  3. On master, run composer install-codestandards --verbose. The output will look something like:

    Composer version 1.9.1 2019-11-01 17:20:17
    
    > install-codestandards: Dealerdirect\Composer\Plugin\Installers\PHPCodeSniffer\Plugin::run
    Running PHPCodeSniffer Composer Installer
    PHPCodeSniffer is not installed
  4. Run echo $? and take note of the exit code being 0 (= success).
  5. Now switch to this branch and run composer install-codestandards --verbose again.

    Composer version 1.9.1 2019-11-01 17:20:17
    
    > install-codestandards: Dealerdirect\Composer\Plugin\Installers\PHPCodeSniffer\Plugin::run
    Running PHPCodeSniffer Composer Installer
    PHPCodeSniffer is not installed
  6. Run echo $? and take note of the exit code now being 1 (= failure).
jrfnl commented 4 years ago

@Potherca @mjrider Anything I can do to move this PR (and the others) forward ?

jrfnl commented 4 years ago

@Potherca @mjrider Anything I can do to move this PR (and the others) forward ?

@Potherca @mjrider :point_up:

Potherca commented 4 years ago

Thanks for the reminder! Crazy/Busy/Hectic, as always.

I've got time to look at this (and other issues) coming Friday. Will report back then. 👍

tobiasbaehr commented 4 years ago

This changes breakes my CI, because when I run --no-dev, then composer removes squizlabs/php_codesniffer + this package and this package exit with "PHPCodeSniffer is not installed".

Is it not possible to check whether composer removes "squizlabs/php_codesniffer" and print just a warning without exitcode 1?

mjrider commented 4 years ago

@tobiasbaehr please open a issue for this, including the steps your CI takes to trigger this case

jrfnl commented 4 years ago

@tobiasbaehr Good point, not a situation I'd considered. Let's discuss this in a separate issue as @mjrider suggested, but I imagine exiting silently when PHPCS is not found, exiting with exitcode 1 when PHPCS is found, but the setting of the paths failed would work ?

jrfnl commented 4 years ago

Oh and just to be clear: the reason I'd not considered it, is that I would expect the require for this plugin to be at the same "level" as for PHPCS/external standards, so if your PHPCS require would be dev only, I'd expect the same for this plugin.

More information on why these are not aligned in your case would be appreciated, so I can properly understand the usecase.

Potherca commented 4 years ago

I'm bit pressed for time right now, but I've got some time later today to pick this up. If a ticket has not been made by then by @tobiasbaehr, I'll make one myself.

I would like to get a fix and deploy a v0.6.1 release for the issue he mentioned ASAP, prefereably before the end of the weekend. (Luckily, I had already reserved time for any fall-out from the v0.6.0 release)

tobiasbaehr commented 4 years ago

@Potherca

Ok I created a new issue for this use case #103