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

Send an exit code when the script terminates #93

Merged
merged 2 commits into from
Jan 17, 2020

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Dec 17, 2019

Proposed Changes

This fixes the issue identified in #80 (comment) 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).

This fixes the issue identified in #80 (comment) 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.
@jrfnl
Copy link
Member Author

jrfnl commented Dec 27, 2019

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

@jrfnl
Copy link
Member Author

jrfnl commented Jan 15, 2020

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

@Potherca @mjrider ☝️

@Potherca
Copy link
Member

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. 👍

Potherca
Potherca previously approved these changes Jan 17, 2020
@tobiasbaehr
Copy link

tobiasbaehr commented Jan 24, 2020

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

mjrider commented Jan 24, 2020

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

@jrfnl
Copy link
Member Author

jrfnl commented Jan 24, 2020

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

jrfnl commented Jan 24, 2020

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

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

@Potherca

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

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

Successfully merging this pull request may close these issues.

4 participants