Respect PHP version used by Composer and provide better feedback on failure #80

jrfnl · 2019-01-21T17:35:29Z

Proposed Changes

Improve feedback to user on failure to set paths

Related to #79.

When the setting of installed_paths fails, show an error instead of giving the impression that the setting of the installed_paths succeeded.

Use the PHP version used by Composer

Possibly not the best way to do this, but it does work, so consider this a proof of concept, if not the solution.

The getPhpExecCommand() method in the Composer\EventDispatcher\EventDispatcher class basically does what is needed: find the PHP executable used by Composer and turn it into an executable command.

Unfortunately, that method is protected, making it difficult to call that method from within the plugin.

This now copies that method into the plugin and uses it to create the command to pass on to the ProcessExecutor.

The copied method has some minor modifications to comply with the coding standards used within this project.

Based on the proof of conflict referenced in #79, I can confirm that this would solve the issue.

Related Issues

Fixes #79

exussum12 · 2019-05-13T11:37:18Z

src/Plugin.php

+
+        $command  = ProcessExecutor::escape($phpPath);
+        $command .= $phpArgs;
+        $command .= ' -d allow_url_fopen=' . ProcessExecutor::escape(ini_get('allow_url_fopen'));


Just curious why are these needed ?

ah never mind, its the comment above

exussum12 · 2019-05-13T11:39:58Z

Confirm this fixes it. Nice job :)

jrfnl · 2019-05-13T11:43:03Z

@exussum12 Thanks for testing & confirming!

jrfnl · 2019-08-02T21:46:50Z

Composer 1.9.0 has just been released and I noticed an entry in the changelog which may be useful to solve this issue, though that would mean that only Composer 1.9.0+ would be supported and I haven't run any tests yet to see if it is available to plugins as well as Composer scripts.

Anyway, just thought I'd leave a note about it here.

Added PHP_BINARY as env var pointing to the PHP process when executing Composer scripts as shell scripts

Ref: https://github.com/composer/composer/releases/tag/1.9.0

exussum12 · 2019-08-03T07:25:44Z

Your fix basically uses that.

https://github.com/symfony/process/blob/master/PhpExecutableFinder.php#L36

The issue fixed in composer is for when the scripts called by composer are shell scripts (This is a direct php call)

at least thats as i understand it - so this fix is still needed

jrfnl · 2019-08-03T08:59:17Z

@exussum12 Thanks for looking into it. I hadn't had time yet. I was hoping it would remove the need for the function copy which is part of this PR.

jrfnl · 2019-08-28T00:47:08Z

@frenck @Potherca Is there anything I can do to move this PR forward ?

Potherca · 2019-08-28T18:14:36Z

For me, all that is needed is that I find a moment where I have both time, energy and a development machine available. As you may have noticed, it can take a while before the stars are thusly aligned...

I'm thinking that it might be beneficial at this point to add another developer to the list of maintainers as neither me nor @frenck can easily allocate time for this from our other responsibilities.

Potherca

Made some minor changes, other than that, looks good.

Potherca · 2019-09-11T19:27:45Z

To get Travis passing MR #86 needs to be merged.

src/Plugin.php

Potherca · 2019-09-15T17:54:27Z

Awaiting MR #86, these changes can be seen to pass the build, except for PHP5.3:

Failed to set PHP CodeSniffer installed_paths Config

jrfnl · 2019-09-15T18:02:27Z

The command "composer install-codestandards" exited with 0.

Should this exit code be 1 ?

Potherca · 2019-09-15T18:04:10Z

Yup. I think that might be another bug.

mjrider · 2019-09-20T19:28:09Z

Could you please rebase this on master, because the build issues have been resolved

jrfnl · 2019-09-20T22:02:19Z

Rebased.

jrfnl · 2019-12-09T04:19:54Z

I've rebased yet again and added an additional commit fixing the failure against PHPCS 2.x.

The build will currently fail on some code style issues which are addressed in PR #88. I'll rebase this PR again once that PR has been merged so you can see the build passing.

I'd really, really love to see this merged & released before the 1-year anniversary of the bug report.....

As per my comment in the ticket #79 (comment), I've added another branch to the POC repo to make testing this fix even easier, but as can be seen in the comments from other people in this PR thread and based on my own tests, the fix works.

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

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`. Technical choices: As PHPCS returns exit code `0`, even when the setting of the `installed_paths` failed, we need to verify that the paths we tried to set, have actually been set. To that end, a new function `verifySaveSuccess()` has been added. This function takes the paths which were to be saved and compares then to the paths which are actually set in PHPCS after the save to determine whether or not the plugin was successful. A `0` exit code will be returned if successful, `1` if not.

Related to 79. When the setting of `installed_paths` fails, show an error instead of giving the impression that the setting of the `installed_paths` succeeded.

Possible not the best way to do this, but it does work, so consider this a proof of concept, if not the solution. The `getPhpExecCommand()` method in the `Composer\EventDispatcher\EventDispatcher` class basically does what is needed: find the PHP executable used by Composer and turn it into an executable command. Unfortunately, that method is `protected`, making it difficult to call that method from within the plugin. This now copies that method into the plugin and uses it to create the command to pass on to the `ProcessExecutor`. The copied method has some minor modifications to comply with the coding standards used within this project. Based on the proof of conflict referenced in 79, I can confirm that this would solve the issue.

- Moves command outside of exec call, for readability - Moves error message composition to begin of method, to keep it together with other preparations and keep `if/else` statement cleaner - Adds whitespace to separate actions that do not belong together or for readability - Removes redundant variable concat

jrfnl · 2019-12-14T20:36:41Z

Rebased & force pushed now #88 has been merged. We should now be able to see a 100% passing build.

Potherca

Any/All concerns I had have been addressed. As far as I am concerned, this can merged.

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 was referenced Jan 21, 2019

Composer PHP version appears not to be respected #79

Closed

PHPCS invocation fails on Windows 10 if installed globally #50

Closed

exussum12 reviewed May 13, 2019

View reviewed changes

exussum12 previously approved these changes May 13, 2019

View reviewed changes

truls1502 approved these changes Jul 27, 2019

View reviewed changes

truls1502 previously approved these changes Jul 27, 2019

View reviewed changes

Potherca added this to the Release v0.5.1 (Next Minor Release) milestone Sep 11, 2019

Potherca dismissed stale reviews from truls1502 and exussum12 via 4307903 September 11, 2019 19:24

Potherca previously approved these changes Sep 11, 2019

View reviewed changes

jrfnl commented Sep 11, 2019

View reviewed changes

src/Plugin.php Outdated Show resolved Hide resolved

src/Plugin.php Outdated Show resolved Hide resolved

src/Plugin.php Show resolved Hide resolved

Potherca dismissed their stale review via b022a78 September 15, 2019 17:41

Potherca mentioned this pull request Sep 27, 2019

phpcs not recognizing installed_paths #84

Closed

jrfnl added a commit that referenced this pull request Dec 9, 2019

Send an exit code when the script terminates

5afbea6

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

jrfnl added a commit that referenced this pull request Dec 9, 2019

Send an exit code when the script terminates

c19a535

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

jrfnl and others added 4 commits December 14, 2019 21:35

Improve feedback to user on failure to set paths

c7a2e52

Related to 79. When the setting of `installed_paths` fails, show an error instead of giving the impression that the setting of the `installed_paths` succeeded.

Fix compatibility with PHPCS 2.x for the fix

22d3cdc

jrfnl requested review from mjrider and Potherca December 15, 2019 16:28

Potherca approved these changes Dec 16, 2019

View reviewed changes

Potherca merged commit 2531231 into PHPCSStandards:master Dec 16, 2019

Potherca added the bug - confirmed label Dec 16, 2019

jrfnl deleted the feature/79-bugfix branch December 17, 2019 02:13

jrfnl mentioned this pull request Dec 17, 2019

Send an exit code when the script terminates #93

Merged

Potherca modified the milestones: Release v0.5.1 (Next Minor Release), Release v0.6.0 (Next Major Release) Jan 18, 2020

jrfnl mentioned this pull request Mar 5, 2022

Tests: add new PreexistingPHPCSInstalledPathsConfigTest + bug fix #167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect PHP version used by Composer and provide better feedback on failure #80

Respect PHP version used by Composer and provide better feedback on failure #80

jrfnl commented Jan 21, 2019 •

edited

Loading

exussum12 May 13, 2019

exussum12 May 13, 2019

exussum12 commented May 13, 2019

jrfnl commented May 13, 2019

jrfnl commented Aug 2, 2019

exussum12 commented Aug 3, 2019

jrfnl commented Aug 3, 2019

jrfnl commented Aug 28, 2019

Potherca commented Aug 28, 2019

Potherca left a comment

Potherca commented Sep 11, 2019

Potherca commented Sep 15, 2019

jrfnl commented Sep 15, 2019

Potherca commented Sep 15, 2019

mjrider commented Sep 20, 2019

jrfnl commented Sep 20, 2019

jrfnl commented Dec 9, 2019 •

edited

Loading

jrfnl commented Dec 14, 2019

Potherca left a comment

Respect PHP version used by Composer and provide better feedback on failure #80

Respect PHP version used by Composer and provide better feedback on failure #80

Conversation

jrfnl commented Jan 21, 2019 • edited Loading

Proposed Changes

Improve feedback to user on failure to set paths

Use the PHP version used by Composer

Related Issues

exussum12 May 13, 2019

Choose a reason for hiding this comment

exussum12 May 13, 2019

Choose a reason for hiding this comment

exussum12 commented May 13, 2019

jrfnl commented May 13, 2019

jrfnl commented Aug 2, 2019

exussum12 commented Aug 3, 2019

jrfnl commented Aug 3, 2019

jrfnl commented Aug 28, 2019

Potherca commented Aug 28, 2019

Potherca left a comment

Choose a reason for hiding this comment

Potherca commented Sep 11, 2019

Potherca commented Sep 15, 2019

jrfnl commented Sep 15, 2019

Potherca commented Sep 15, 2019

mjrider commented Sep 20, 2019

jrfnl commented Sep 20, 2019

jrfnl commented Dec 9, 2019 • edited Loading

jrfnl commented Dec 14, 2019

Potherca left a comment

Choose a reason for hiding this comment

jrfnl commented Jan 21, 2019 •

edited

Loading

jrfnl commented Dec 9, 2019 •

edited

Loading