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

Don't access vendor/bin in composer ci #37

Merged
merged 1 commit into from Feb 23, 2015
Merged

Don't access vendor/bin in composer ci #37

merged 1 commit into from Feb 23, 2015

Conversation

thiemowmde
Copy link
Contributor

Remove "vendor/bin/" as suggested by @Krinkle in acd15a0#commitcomment-9861167.

I'm also moving the ci command to the top.

JeroenDeDauw added a commit that referenced this pull request Feb 23, 2015
Don't access vendor/bin in composer ci
@JeroenDeDauw JeroenDeDauw merged commit 7a6d1e0 into master Feb 23, 2015
@JeroenDeDauw JeroenDeDauw deleted the noVendor branch February 23, 2015 20:20
@JeroenDeDauw
Copy link
Collaborator

pfft

Restoring correct behavior by reverting

@Krinkle
Copy link

Krinkle commented Mar 6, 2015

@JeroenDeDauw Have you tested it properly? Obviously composer does not, cannot and should not modify PATH in your own main bash process. Merely the subprocesses spawned by composer test and composer run-script ... Can you test again by running phpcs --version from the line in composer.json?

I've relied on that to work in countless repositories, platforms and environments. Both in composer and npm. I'd be really surprised if that doesn't work.

Krinkle referenced this pull request Mar 6, 2015
Includes a small set of CS fixes so the CI does not fail on the
initial CS ruleset.
@JeroenDeDauw
Copy link
Collaborator

I noticed this because running composer ci gave me an error caused by it using the old version of PHPCS I have on my laptop

@Krinkle
Copy link

Krinkle commented Mar 6, 2015

@JeroenDeDauw It works for me locally, and I'm using it via Travis CI and Jenkins in many projects. When I run self-update to latest master its broken indeed. I ran composer self-update --rollback since it's clearly a regression. Filed upstream at composer/composer#3820. Thanks!

(Note: One should run composer run-script <task> not composer <task>)

@JeroenDeDauw
Copy link
Collaborator

@Krinkle Thanks for looking into this further

To be honest I'm a bit dubious about making this change even if it does work. In practical terms it seems to not matter the slightest, except for one case I can think off: someone that is not familiar with composer commands reading the code. For them the more explicit version ought to be clearer.

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

Successfully merging this pull request may close these issues.

None yet

3 participants