Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

lithium\net\http\Service::head() should support $path parameter #640

Closed
ppadron opened this Issue · 3 comments

2 participants

@ppadron

In the current implementation of the Service class, the head() method accepts only an $options parameter, this way all HEAD requests are sent to the root of the hostname. Like the other methods, it should allow $path, $data (to be sent in the query string) and $options.

I had a small fix for this: http://pastium.org/view/97fb0153c0baadc66d03b25aab399c8a

The switch statement was added to support different values of the 'return' option key found in Lithium test cases.

However, I'm not sure if we should call the Response::headers() method or return the public property Response::$headers. They return different values: headers() will return an array where each element is a header line, but the $headers property will return an associative array where the key is the header field name.

I'd stick with the associative array because it seems more useful to me, but I don't know if we should access the $headers property when there is a method available.

What do you think? When consensus is reached on this I'll submit a pull request.

@nateabele
Owner

Nope, you're right. The job of the headers() method is to export the associative array to something that can be written to the browser, so $headers is good.

However, I wasn't kidding when I said switch() blocks suck (cc: @gwoo). Here's the same functionality, rewritten in one less line of code, that does more and is more flexible:

diff --git a/net/http/Service.php b/net/http/Service.php
index 8baa8ec..5ceb35e 100644
--- a/net/http/Service.php
+++ b/net/http/Service.php
@@ -36,7 +36,14 @@ class Service extends \lithium\core\Object {
     *
     * @var array
     */
-   protected $_autoConfig = array('classes' => 'merge');
+   protected $_autoConfig = array('classes' => 'merge', 'responseTypes');
+
+   /**
+    * Array of closures that return various pieces of information about an HTTP response.
+    *
+    * @var array
+    */
+   protected $_responseTypes = array();

    /**
     * Indicates whether `Service` can connect to the HTTP endpoint for which it is configured.
@@ -90,6 +97,11 @@ class Service extends \lithium\core\Object {
        } catch(ClassNotFoundException $e) {
            $this->connection = null;
        }
+       $this->_responseTypes += array(
+           'headers' => function($response) { return $response->headers; },
+           'body' => function($response) { return $response->body(); },
+           'code' => function($response) { return $response->status['code']; }
+       );
    }

    /**
@@ -192,7 +204,10 @@ class Service extends \lithium\core\Object {
            $this->connection->close();
        }
        $this->last = (object) compact('request', 'response');
-       return ($options['return'] == 'body' && $response) ? $response->body() : $response;
+       $handlers = $this->_responseTypes;
+       $handler = isset($handlers[$options['return']]) ? $handlers[$options['return']] : null
+
+       return $handler ? $handler($response) : $response;
    }

    /**
@ppadron

Nice! Do you mind if I add this to the same commit or do you want to keep them separated?

@nateabele
Owner

Go for it. Write a test and squash it down into one commit, then do a PR against the dev branch. Thanks!

@nateabele nateabele closed this issue from a commit
@ppadron ppadron Implementing return handlers in net\http\Service and adding support f…
…or $path and $data to HEAD requests. Closes #640.
1ccc1b1
@nateabele nateabele closed this in 1ccc1b1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.