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

Guzzle proxy support #29

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

holtkamp
Copy link
Contributor

As suggested in #27

@dennisdegreef
Copy link
Contributor

This looks awesome! Thanks for this!

One question though, why are docblocks being removed in this PR?

@holtkamp
Copy link
Contributor Author

One question though, why are docblocks being removed in this PR?

mmm, in my opinion they should only be used when adding information... Since PHP 7.1 proper return types are supported, so no PHPDoc is required anymore...

Also see this thread: https://twitter.com/akrabat/status/878345065696985088

@dennisdegreef
Copy link
Contributor

Interesting. I tend to agree with the provided point.
On the other hand, should we then allow multiple types for a proxy? Namely, a string or an array? Or should we choose one over the other?

@holtkamp
Copy link
Contributor Author

On the other hand, should we then allow multiple types for a proxy? Namely, a string or an array? Or should we choose one over the other?

First I went with array, since it provides more options for configuration. But eventually, to be able to use a static IP, a plain HTTP proxy suffices => so I went with the string.

Then probably only the string suffices.... But hey, why limit it? The current approach allows for best of both worlds and it is up to the user what info is handed over to Guzzle...

@dennisdegreef
Copy link
Contributor

So can we typehint on an array (also allowing a single entry) and validate the strings in that array?
As long as we are pushing towards stricter typehinted code, might as well only allow a single type as a parameter.

@holtkamp
Copy link
Contributor Author

holtkamp commented Jul 2, 2017

So can we typehint on an array

Sure, but why restrict the options? We can just follow the Guzzle interface for it right? string + array
http://docs.guzzlephp.org/en/stable/request-options.html#proxy

@holtkamp
Copy link
Contributor Author

holtkamp commented Jul 9, 2017

shameless bump on this? 😄

@holtkamp
Copy link
Contributor Author

holtkamp commented Aug 1, 2017

@dennisdegreef another bump on this?

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