search

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

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

Closed jrfnl closed 4 years ago

jrfnl commented 5 years ago

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 commented 5 years ago

Confirm this fixes it. Nice job :)

jrfnl commented 5 years ago

@exussum12 Thanks for testing & confirming!

jrfnl commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

Potherca commented 5 years ago

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 commented 5 years ago

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

Potherca commented 5 years ago

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 commented 5 years ago

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

Should this exit code be 1 ?

Potherca commented 5 years ago

Yup. I think that might be another bug.

mjrider commented 5 years ago

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

jrfnl commented 5 years ago

Rebased.

jrfnl commented 4 years ago

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 https://github.com/Dealerdirect/phpcodesniffer-composer-installer/issues/79#issuecomment-563053844, 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.

jrfnl commented 4 years ago

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