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

Added Requests_Transport_cURL support for proxy options #37

Closed
wants to merge 1 commit into from

Conversation

gmarintes
Copy link

We can actually use the hooks to do this, however, $options is much simpler and easier to use IMHO.

@rmccue
Copy link
Collaborator

rmccue commented Jan 15, 2013

This looks good, but I have a few comments:

  1. Can you make this work for fsockopen too please? The transport used has to be transparent to the end user, so it has to work identically on both.
  2. Can you write tests for this please? While the changes appear good, I have no idea if it actually works without those. :)

Thanks for contributing!

@gmarintes
Copy link
Author

Hi Ryan,

Thanks.

I am in a hurry at the moment and won't be able to support fsockopen.
fsockopen proxy support does not seem to be as straightforward as curl
proxy.

As for the test, I'm not sure how I would write one for this feature,
since the test itself must work on all environments.
I can only say that I am using it in my project and its working as expected.

Best regards,
Glenn

On 1/15/2013 8:22 AM, Ryan McCue wrote:

This looks good, but I have a few comments:

  1. Can you make this work for fsockopen too please? The transport
    used has to be transparent to the end user, so it has to work
    identically on both.
  2. Can you write tests for this please? While the changes appear
    good, I have no idea if it actually works without those. :)

Thanks for contributing!


Reply to this email directly or view it on GitHub
#37 (comment).

@ozh ozh mentioned this pull request Sep 7, 2013
@rmccue
Copy link
Collaborator

rmccue commented Sep 24, 2013

Fixed as part of #70; thanks for the patch anyway!

@rmccue rmccue closed this Sep 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants