Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes new proxy code #60

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

simensen commented Mar 26, 2012

Proxy format needs special scheme (tcp:// or ssl://) for stream proxies, port is always needed in all cases.

  • It looks like the proxy handling for both cURL and native PHP streams requires a port to work
  • I updated the URL utility to add default ports for HTTP and HTTPS to let people be able to specify
    proxy as just and IP address or just an http:// address without a port. Not sure this is needed.
    (causes tests to break — see below)
  • For stream based proxies, needed to build out the URL correctly. As it was, the getUrl was a runtime
    error since it did not exist.

Tests are broken with this patch. I see that master currently strips :80 and :443 for HTTP and HTTPS for getHost. I think these tests should work just fine once merged from master. Right now tests fail with the likes of "Expected http://example.com but Actual http://example.com:80".

I can rebase against master and make sure all of the tests work if you prefer. Please let me know!

@simensen simensen referenced this pull request Mar 26, 2012

Closed

added proxy support #58

Contributor

simensen commented Mar 26, 2012

It might be worth noting that my proxy does not support auth so I have not been able to actually test out the auth stuff. I can verify that this works with an actual HTTP proxy on a network that will not allow web traffic so I know that it is working.

@stloyd stloyd and 1 other commented on an outdated diff Mar 26, 2012

lib/Buzz/Client/AbstractStream.php
+ if ($user = $this->getProxy()->getUser()) {
+ if ($password = $this->getProxy()->getPassword()) {
+ $proxyAuth = $user.':'.$password.'@';
+ } else {
+ $proxyAuth = $user.'@';
+ }
+ } else {
+ $proxyAuth = '';
+ }
+
+ // We actually need the port otherwise stream croaks. Had initially
+ // tried proxy->getHost() and it did not work since it removed the
+ // port it matched the scheme default.
+ $proxy = $this->proxy->getHostname() . ':' . $this->proxy->getPort();
+
+ if ('htttps' === $this->proxy->getScheme()) {
@stloyd

stloyd Mar 26, 2012

Contributor

Typo here =)

@simensen

simensen Mar 26, 2012

Contributor

Nice. :)

@stloyd stloyd and 1 other commented on an outdated diff Mar 26, 2012

lib/Buzz/Client/Curl.php
@@ -142,7 +142,10 @@ protected function prepare(Message\Request $request, Message\Response $response,
curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, $this->verifyPeer);
+
+ if ($proxy = $this->getProxy()) {
+ // We actually need the port otherwise curl croaks. Had initially
+ // tried proxy->getHost() and it did not work since it removed the
+ // port it matched the scheme default.
+ curl_setopt($curl, CURLOPT_PROXY, $proxy->getHost().':'.$proxy->getPort());
+ if ($user = $proxy->getUser()) {
@stloyd

stloyd Mar 26, 2012

Contributor

Did you tried: curl_setopt($curl, CURLOPT_PROXYPORT, $proxy->getPort(); ?

@simensen

simensen Mar 26, 2012

Contributor

Actually, yeah, that would make more sense. Also, I should have done getHostname() instead of getHost() anyway. Thanks.

Contributor

simensen commented Mar 28, 2012

@kriswallsmith Have you been able to try this? Also, it looks like maybe I botched the branch. It looks like this is includes your d215894 change for some reason. Let me know if you want me to rebase this and/or send another PR.

Owner

kriswallsmith commented Mar 28, 2012

I've rebase my branch, which is why your PR looks funny now. I've been able to get functional tests running also, but they're not all passing. I'm travelling today but will get back into this later this week.

Contributor

simensen commented Apr 4, 2012

@kriswallsmith I am sure you have your hands full, but let me know if you have time to work on this more this week. I'm happy to continue to make changes as you need me to make them. :)

Owner

kriswallsmith commented Apr 17, 2012

@simensen Please take a look at FunctionalTest in the proxy branch. I've created a test case to run all the various possibilities but haven't had any luck getting them all to pass. Maybe you'll do better?

Contributor

simensen commented Apr 17, 2012

@kriswallsmith I'll see what i can do. I'll rebase against your recent changes and see if I can get it to work. Thanks. =)

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