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

Allow simple-phpunit to be used with an HTTP proxy #20862

Merged
merged 1 commit into from Dec 13, 2016

Conversation

Projects
None yet
5 participants
@Cydonia7
Contributor

Cydonia7 commented Dec 10, 2016

Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR no

The title pretty much sums it up. I had to use the script behind a proxy and it did not work well so here is a little fix to take the http_proxy environment variable into account when downloading with fopen.

I don't think there needs to be a doc PR associated since this feature should be transparent for the end-user.

@nicolas-grekas

Is there a https_proxy environment variable also or not?

@@ -29,6 +29,16 @@ $COMPOSER = file_exists($COMPOSER = $oldPwd.'/composer.phar') || ($COMPOSER = rt
? $PHP.' '.escapeshellarg($COMPOSER)
: 'composer';
$options = [

This comment has been minimized.

@nicolas-grekas
'http' => [],
];
if (($proxy = getenv('http_proxy')) !== FALSE) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 11, 2016

Member

Yoda style, false always lowercase, no need for the brackets

@Cydonia7

This comment has been minimized.

Contributor

Cydonia7 commented Dec 11, 2016

https_proxy can indeed exist, not sure how I can take this into consideration. Should I add an https key to the context array?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 12, 2016

composer has a bunch of code to properly deal with proxies, see:
https://github.com/composer/composer/blob/master/src/Composer/Util/StreamContextFactory.php

Since we don't want to put that much logic here, I propose to simply use wget when isset($_SERVER['http_proxy']) || isset($_SERVER['https_proxy']).

This could then go as bug fix on 3.2 IMHO.

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Dec 12, 2016

In the Symfony Installer we also used a simplified proxy detection based on the Composer one. It seems to be working OK: https://github.com/symfony/symfony-installer/blob/master/src/Symfony/Installer/DownloadCommand.php#L243

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 12, 2016

@javiereguiluz it looks like this builds on a Guzzle feature? Which would mean not applicable here?

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Dec 12, 2016

@nicolas-grekas yes, I was only referring to these lines:

if (!empty($_SERVER['HTTP_PROXY']) || !empty($_SERVER['http_proxy'])) {
    $defaults['proxy'] = !empty($_SERVER['http_proxy']) ? $_SERVER['http_proxy'] : $_SERVER['HTTP_PROXY'];
}

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Dec 12, 2016

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 12, 2016

👍

@Cydonia7 Cydonia7 changed the base branch from master to 3.2 Dec 12, 2016

@nicolas-grekas nicolas-grekas added Bug and removed Feature labels Dec 12, 2016

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 13, 2016

Thank you @Cydonia7.

@fabpot fabpot merged commit 921b646 into symfony:3.2 Dec 13, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 13, 2016

bug #20862 Allow simple-phpunit to be used with an HTTP proxy (Cydonia7)
This PR was merged into the 3.2 branch.

Discussion
----------

Allow simple-phpunit to be used with an HTTP proxy

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | no

The title pretty much sums it up. I had to use the script behind a proxy and it did not work well so here is a little fix to take the `http_proxy` environment variable into account when downloading with fopen.

I don't think there needs to be a doc PR associated since this feature should be transparent for the end-user.

Commits
-------

921b646 Allow simple-phpunit to be used with an HTTP proxy

@fabpot fabpot referenced this pull request Dec 13, 2016

Merged

Release v3.2.1 #20897

@Cydonia7 Cydonia7 deleted the Cydonia7:phpunit-proxy branch Jan 12, 2017

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