Skip to content

Use "https_proxy" env variable if defined to set the CURLOPT_PROXY option.#54

Merged
mattheworiordan merged 2 commits intoably:masterfrom
DamienHarper:master
Feb 19, 2019
Merged

Use "https_proxy" env variable if defined to set the CURLOPT_PROXY option.#54
mattheworiordan merged 2 commits intoably:masterfrom
DamienHarper:master

Conversation

@DamienHarper
Copy link
Copy Markdown
Contributor

No description provided.

@funkyboy
Copy link
Copy Markdown
Contributor

funkyboy commented Jun 9, 2018

Looks good. Thanks.
I wonder if there’s a quick way to test when the proxy has been set @DamienHarper

@DamienHarper
Copy link
Copy Markdown
Contributor Author

@funkyboy Not sure we can find a quick way to test.
The company I work for rely on a hosting provider which blocks any trafic to the outside that do not go through its proxy. Both my previous PR and this one solve the problem in our use case, that's how I test it ;)

@funkyboy
Copy link
Copy Markdown
Contributor

funkyboy commented Jun 9, 2018

I think some unit test along the lines of these is enough https://github.com/ably/ably-php/blob/master/tests/TypesTest.php
Like “set proxy and check that is set”, without actually performing a request.

@DamienHarper
Copy link
Copy Markdown
Contributor Author

@funkyboy @jdavid any chance this one gets merged and released (it's almost the same as #51 which has already been merged and released today)?

@mattheworiordan
Copy link
Copy Markdown
Member

@DamienHarper we had assumed no one needed this so parked it. I assume you still need to connect through a proxy? If so, I will see if we can get this bundled in later this week after our 1.1 release (hopefully tomorrow)

@DamienHarper
Copy link
Copy Markdown
Contributor Author

@mattheworiordan thanks for the feedback.
As you guess, we still are using the same hosting provider (we won't change anytime soon) so we still need to go through their proxy (for both HTTP and HTTPS protocols)
Until this PR is merged and released, the only solution for us right now is to use our own fork including the code from this PR.

@mattheworiordan
Copy link
Copy Markdown
Member

@jdavid are you happy for us to merge this?

Copy link
Copy Markdown
Contributor

@jdavid jdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DamienHarper

@mattheworiordan mattheworiordan merged commit 962f203 into ably:master Feb 19, 2019
@mattheworiordan
Copy link
Copy Markdown
Member

@DamienHarper this is merged and now released as 1.1.1. Sorry this took so long!

@DamienHarper
Copy link
Copy Markdown
Contributor Author

@mattheworiordan @funkyboy @jdavid thanks!

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.

4 participants