Support force_ip_resolve in StreamHandler #1659

c960657 · 2016-11-15T19:36:35Z

The option force_ip_resolve that was added in #1608 is only supported for CurlHandler.

This patch adds support for force_ip_resolve to StreamHandler.

sagikazarmark

Thanks for your contribution. I added a few comments, can you please take a look at them?

sagikazarmark · 2016-12-13T19:03:53Z

src/Handler/StreamHandler.php

@@ -286,7 +288,7 @@ private function createStream(RequestInterface $request, array $options)
            foreach ($options as $key => $value) {
                $method = "add_{$key}";
                if (isset($methods[$method])) {
-                    $this->{$method}($request, $context, $value, $params);
+                    $this->{$method}($request, $context, $value, $params, $uri);


Is it really necessary to add the URI as a parameter? It's accessible from the request, isn't it?

It is, but I chose to preserve the original URI in the request in case other callbacks, middleware etc. need it. The modified $url is not the actual URL being requested but a version where the hostname has been replaced with an IP address, i.e. it lacks information about HTTP virtual host. This version is only used to "trick" fopen() into binding to the proper IP. The virtual host information is sent to the server in the Host HTTP header.

Ah, okay, you are totally right. For some reasons I thought you are trying to set the host in the original request. Even then, adding the URI to all methods bellow seems to be too much. How about a resolveUri or resolveAddress or something similar method? It would make things easier IMO.

sagikazarmark · 2016-12-13T19:04:45Z

src/Handler/StreamHandler.php

@@ -277,6 +278,7 @@ private function createStream(RequestInterface $request, array $options)

        $params = [];
        $context = $this->getDefaultContext($request, $options);
+        $uri = $request->getUri()->withFragment('');


Why is this better than the original code?

In fact it's not a good idea IMO as the reference for this object will not be the same as you expected.

See my comment below on why I created a separate $uri variable outside the callback.

sagikazarmark · 2016-12-13T19:05:04Z

src/Handler/StreamHandler.php

@@ -350,7 +352,7 @@ private function getDefaultContext(RequestInterface $request)
        return $context;
    }

-    private function add_proxy(RequestInterface $request, &$options, $value, &$params)
+    private function add_proxy(RequestInterface $request, &$options, $value, &$params, UriInterface &$uri)


Objects are passed by reference

True, but URIs are immutable, so I have to change the value by reference.

sagikazarmark · 2016-12-13T19:13:51Z

src/Handler/StreamHandler.php

+            if (!isset($records[0]['ip'])) {
+                throw new ConnectException("Could not resolve IPv4 address for host '{$uri->getHost()}'", $request);
+            }
+            $uri = $uri->withHost($records[0]['ip']);


Sadly this is also not going to work as you expect. HTTP Messages are immutable, which means if you "modify" one them you will get a new object. The new URI will not be present in the request object. What you can do is saving the resolved IP address in the options array and passing it to the URI, then the URI to the request somewhere in the above code.

I'm not sure what you mean. I am not modifying the HTTP message here, only $uri that is passed by reference.

c960657 · 2016-12-14T07:20:12Z

If you have IPv6 connectivity, you can test the code using this script:

$c = new \GuzzleHttp\Client();
print (string) $c->get('http://icanhazip.com/s', ['force_ip_resolve' => 'v4'])->getBody();
print (string) $c->get('http://icanhazip.com/s', ['force_ip_resolve' => 'v6'])->getBody();

sagikazarmark

Can you please also add StreamHandler to the list of supportet handlers in the note added in #1608?

sagikazarmark · 2016-12-22T21:45:05Z

src/Handler/StreamHandler.php

+            if ('v4' === $options['force_ip_resolve']) {
+                $records = dns_get_record($uri->getHost(), DNS_A);
+                if (!isset($records[0]['ip'])) {
+                    throw new ConnectException("Could not resolve IPv4 address for host '{$uri->getHost()}'", $request);


Can you please use sprintf instead? I am not really fan of this kind of resolution.

sagikazarmark · 2016-12-22T21:46:24Z

src/Handler/StreamHandler.php

                $this->lastHeaders = $http_response_header;
                return $resource;
            }
        );
    }

+    private function resolveUri(UriInterface $uri, array $options)


Maybe call it resolveIp or resolveAddress? Since that's what we do here, isn't it?

Done. I chose the name resolveHost (we are resolving the hostname to an IP address).

Rename resolveUri() to resolveHost() Check that URI host is not already an IP Pass $request to ConnectException

sagikazarmark · 2017-05-01T09:43:48Z

Thanks for your work and patience. 6.3 is on it's way

c960657 force-pushed the force-ip-resolve-stream branch from 9e03f48 to f2337fe Compare November 15, 2016 21:13

sagikazarmark requested changes Dec 13, 2016

View reviewed changes

Support force_ip_resolve in StreamHandler

e980738

c960657 force-pushed the force-ip-resolve-stream branch from f2337fe to e980738 Compare December 15, 2016 07:27

sagikazarmark requested changes Dec 22, 2016

View reviewed changes

Review fixes

fbab002

Rename resolveUri() to resolveHost() Check that URI host is not already an IP Pass $request to ConnectException

sagikazarmark modified the milestone: 6.3.0 Jan 13, 2017

Merge branch 'master' into force-ip-resolve-stream

eb79b37

c960657 force-pushed the force-ip-resolve-stream branch from 8b0fa76 to eb79b37 Compare March 29, 2017 10:27

sagikazarmark approved these changes May 1, 2017

View reviewed changes

sagikazarmark merged commit 8353b3f into guzzle:master May 1, 2017

c960657 deleted the force-ip-resolve-stream branch May 1, 2017 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support force_ip_resolve in StreamHandler #1659

Support force_ip_resolve in StreamHandler #1659

c960657 commented Nov 15, 2016

sagikazarmark left a comment

sagikazarmark Dec 13, 2016

c960657 Dec 14, 2016 •

edited

sagikazarmark Dec 14, 2016

c960657 Dec 15, 2016

sagikazarmark Dec 13, 2016

sagikazarmark Dec 13, 2016

c960657 Dec 14, 2016

sagikazarmark Dec 13, 2016

c960657 Dec 14, 2016

sagikazarmark Dec 13, 2016

c960657 Dec 14, 2016

c960657 commented Dec 14, 2016

sagikazarmark left a comment

sagikazarmark Dec 22, 2016

c960657 Dec 29, 2016

sagikazarmark Dec 22, 2016

c960657 Dec 29, 2016

sagikazarmark commented May 1, 2017

Support force_ip_resolve in StreamHandler #1659

Support force_ip_resolve in StreamHandler #1659

Conversation

c960657 commented Nov 15, 2016

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c960657 Dec 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c960657 commented Dec 14, 2016

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark commented May 1, 2017

c960657 Dec 14, 2016 •

edited